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

2011-09-23 Thread Rami Ylimäki 

On 09/22/2011 07:34 PM, Jamey Sharp wrote:

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.


Originally, there was no good reason for extracting the code to a 
separate function. However, I'm currently suspecting that a certain XACE 
module 
(https://meego.gitorious.org/maemo-multimedia/xserver-policy-enforcement) might 
be suffering from the same problem. In this case I'd want to have a 
separate function and export it as well so that I wouldn't have to 
copy-paste its contents into the XACE module. If it turns out that XPE 
works fine, I can remove that function in the next patch revision.



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.


To be honest, I didn't know what LBX was when I wrote the patch and 
didn't take into account the effect of extensions on closing down 
clients. But it seems that LBX has been gone for 5 years now, so I can 
clarify this in the commit message of the next patch revision.


-- Rami

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

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

2011-09-22 Thread Rami Ylimäki 
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.ylim...@vincit.fi
Reviewed-by: Erkki Seppälä erkki.sepp...@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@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

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

2011-09-22 Thread 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.ylim...@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.ylim...@vincit.fi
 Reviewed-by: Erkki Seppälä erkki.sepp...@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@lists.x.org: X.Org development
 Archives: http://lists.x.org/archives/xorg-devel
 Info: http://lists.x.org/mailman/listinfo/xorg-devel
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel