[PATCH] dix: Prevent access to freed memory if a client kills itself.

[Jamey Sharp]

This patch makes sense to me, but I have a couple of requests:

I'm not sure why you extracted out a separate function; I'm not
convinced that makes the code more clear, in this case.

More importantly, I'd like to see justification in the commit message
for deleting LBX support here. I seem to recall that support was
deleted from the rest of the server already, which would be excellent
justification, but please say so if that's true.

Jamey

On 9/22/11, Rami Ylimäki <rami.ylimaki at vincit.fi> wrote:
> The 'Dispatch' function accesses freed client structure if a client
> happens to kill itself in a request. For example, I have a test client
> that is used to check that it handles the XIO error correctly. The XIO
> error is generated by requesting the client to kill itself with
> XKillClient.
>
> Signed-off-by: Rami Ylimäki <rami.ylimaki at vincit.fi>
> Reviewed-by: Erkki Seppälä <erkki.seppala at vincit.fi>
> ---
>  dix/dispatch.c |   32 +++++++++++++++++---------------
>  1 files changed, 17 insertions(+), 15 deletions(-)
>
> diff --git a/dix/dispatch.c b/dix/dispatch.c
> index 43cb4d1..cc5ee09 100644
> --- a/dix/dispatch.c
> +++ b/dix/dispatch.c
> @@ -3226,6 +3226,20 @@ ProcChangeAccessControl(ClientPtr client)
>      return ChangeAccessControl(client, stuff->mode == EnableAccess);
>  }
>
> +/**
> + * Prevents a client from killing itself immediately.
> + */
> +static void CloseDownClientByClient(ClientPtr client, ClientPtr killclient)
> +{
> +    if (client == killclient)
> +    {
> +        MarkClientException(client);
> +        isItTimeToYield = TRUE;
> +    }
> +    else
> +        CloseDownClient(killclient);
> +}
> +
>  /*********************
>   * CloseDownRetainedResources
>   *
> @@ -3263,21 +3277,9 @@ ProcKillClient(ClientPtr client)
>      }
>
>      rc = dixLookupClient(&killclient, stuff->id, client, DixDestroyAccess);
> -    if (rc == Success) {
> -	CloseDownClient(killclient);
> -	/* if an LBX proxy gets killed, isItTimeToYield will be set */
> -	if (isItTimeToYield || (client == killclient))
> -	{
> -	    /* force yield and return Success, so that Dispatch()
> -	     * doesn't try to touch client
> -	     */
> -	    isItTimeToYield = TRUE;
> -	    return Success;
> -	}
> -	return Success;
> -    }
> -    else
> -	return rc;
> +    if (rc == Success)
> +	CloseDownClientByClient(client, killclient);
> +    return rc;
>  }
>
>  int
> --
> 1.7.1
>
> _______________________________________________
> xorg-devel at lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: http://lists.x.org/mailman/listinfo/xorg-devel