Created attachment 35338 [details] [review]
Fix XID confusion in DRI2

This only happens with a compositing manager, right?  Can you try applying the attached patch to the xserver and see if that fixes the problem?

Created attachment 35350 [details] [review]
fix the fix

Owen pointed out that my patch was broken and didn't actually do what it said it did.  This should work better.

The more recent patch worked well in light testing, but more stressful testing (
starting and killing an application under a compositing manager in a tight loop) revealed X server crashes along the lines of:

Program received signal SIGSEGV, Segmentation fault.
FreeResource (id=21023285, skipDeleteFuncType=0) at resource.c:546
546		    if (res->id == id)
(gdb) bt
#0  FreeResource (id=21023285, skipDeleteFuncType=0) at resource.c:546
#1  0x00000000004287ca in ProcFreePixmap (client=0x1143920) at dispatch.c:1502
#2  0x000000000042c32c in Dispatch () at dispatch.c:439
#3  0x00000000004219ca in main (argc=<value optimized out>, argv=0x7fff79de5a78, 
    envp=<value optimized out>) at main.c:286
(gdb) frame 1
#1  0x00000000004287ca in ProcFreePixmap (client=0x1143920) at dispatch.c:1502
1502		FreeResource(stuff->id, RT_NONE);

The problem here, as I see it, is the code in DRI2DrawableGone() is calling FreeResourceByType(), but the resource management code isn't reentrant - if the drawable XID and the fake DRI drawable ID end up in the same hash bucket, then if DRIDrawableGone deletes a resource, the "prev" pointer can end up as garbage when DRIDrawableGone returns and FreeResource continues, causing crashes.

I'm not sure I understand the function of the fake DRI drawable ID - is the point to avoid having a manual hook on client disconnect and piggyback on the normal resource cleanup code? It seems otherwise like we could just use the client ID rather than the faked-up resource ID for the purposes of maintaining the mapping.

(In reply to comment #3)
> The more recent patch worked well in light testing, but more stressful testing
> (
> starting and killing an application under a compositing manager in a tight
> loop) revealed X server crashes along the lines of:
> 
> Program received signal SIGSEGV, Segmentation fault.
> FreeResource (id=21023285, skipDeleteFuncType=0) at resource.c:546
> 546            if (res->id == id)
> (gdb) bt
> #0  FreeResource (id=21023285, skipDeleteFuncType=0) at resource.c:546
> #1  0x00000000004287ca in ProcFreePixmap (client=0x1143920) at dispatch.c:1502
> #2  0x000000000042c32c in Dispatch () at dispatch.c:439
> #3  0x00000000004219ca in main (argc=<value optimized out>,
> argv=0x7fff79de5a78, 
>     envp=<value optimized out>) at main.c:286
> (gdb) frame 1
> #1  0x00000000004287ca in ProcFreePixmap (client=0x1143920) at dispatch.c:1502
> 1502        FreeResource(stuff->id, RT_NONE);
> 
> The problem here, as I see it, is the code in DRI2DrawableGone() is calling
> FreeResourceByType(), but the resource management code isn't reentrant - if the
> drawable XID and the fake DRI drawable ID end up in the same hash bucket, then
> if DRIDrawableGone deletes a resource, the "prev" pointer can end up as garbage
> when DRIDrawableGone returns and FreeResource continues, causing crashes.

AFAIK, it's supposed to be reentrant.  If you look at FreeResource() in dix/resource.c, you can see that it compares the value of clientTable[cid].elements before and after the callback.  If something is added or deleted in the callback, the element count changes and FreeResources() restarts the search by setting prev = head.

However, I just looked at FreeResourceByType, to make sure it adjusted the element count when it deleted the resources... and it doesnt!  It's a little hard to believe that this kind of bug could have stayed around for so long in the server, but I guess the only consequence (aside from this kind of bug) is that the resource hash tables grow a little bigger than they need to.

> I'm not sure I understand the function of the fake DRI drawable ID - is the
> point to avoid having a manual hook on client disconnect and piggyback on the
> normal resource cleanup code? It seems otherwise like we could just use the
> client ID rather than the faked-up resource ID for the purposes of maintaining
> the mapping.

Yeah, it's just piggybacking on the resource cleanup that happens at client disconnect.  Pretty standard technique in the server.  We could subscribe to the ClientStateCallback instead and delete all the refs from a client when it disconnects, but it would be more work and the resource code was designed to handle this kind of case.

Created attachment 35363 [details] [review]
Fix the resource counting in FreeResourceByType()

This fixes the resource counting when deleting by type.  I still can't belive this bug was in there...

Created attachment 35364 [details] [review]
roll-up patch

Another update with the following changes:

  1) Rolls up the FreeResourceByType() fix
  2) Undoes the FreeResourceByType() change in glxcmds.c:DoDestroyDrawable()
  3) Plugs a leak of 'ref' in DRI2DrawableGone()

I applied the latest patch, and it seems to bring compiz back to life.

However, I noticed that I can crash the X in following way:

start gnome-screensaver-properties
switch between few screensavers.

X dies in DRI2WaitSwap, 

'if ((pPriv->swapsPending)'


I have nvidia geforce 8400M + nouveau + git tips of everything
(libdrm, mesa, xserver, nouveau ddx, ...)

With xserver-1.8 branch everything works just fine, including absence of above crash.

Maybe it was a coincidence, because now after few updates, everything works just fine

(In reply to comment #6)
> Created an attachment (id=35364) [details]
> roll-up patch
> 
> Another update with the following changes:
> 
>   1) Rolls up the FreeResourceByType() fix
>   2) Undoes the FreeResourceByType() change in glxcmds.c:DoDestroyDrawable()
>   3) Plugs a leak of 'ref' in DRI2DrawableGone()

Yes, this patch fixes on my side (on behalf of Xun - the original reporter, who is on vacation this week).

The patch series just go pulled into xserver master, bug should be fixed.  Please verify and close if it works.

Works just fine!

It works well. Verified.