[PATCH] dix: Prevent access to freed memory if a client kills itself.
Jamey Sharp
jamey at minilop.net
Thu Sep 22 09:34:46 PDT 2011
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
More information about the xorg-devel
mailing list