[RFC] glx: fix DRI2 memory leak

[Shuang He]

Shuang He wrote:
> Jesse Barnes wrote:
>> On Fri, 27 Mar 2009 13:20:25 -0400
>> Kristian Høgsberg <krh at bitplanet.net> wrote:
>>
>>   
>>> On Thu, Mar 26, 2009 at 6:33 PM, Jesse Barnes
>>> <jbarnes at virtuousgeek.org> wrote:
>>>     
>>>> Ok, I think this is where the leak was:
>>>> __glXUnrefDrawable->DrawableGone->__glXUnrefDrawable.  This sequence
>>>> meant the glxPriv would stay around (since it was used), but
>>>> DrawableGone had generally already disposed of the pDrawable before
>>>> the call to Unref, so we never got into DRI2DestroyBuffers, thus
>>>> the leak.
>>>>
>>>> The loop seems broken to me.  It *looks* like DrawableGone should be
>>>> the real free routine, only called when a drawable really is free,
>>>> so I've fixed up the code to conform to that.  This means removing
>>>> the GLX priv frees from DRI and DRI2 routines and putting them here
>>>> and using the GLX unref routine everwhere (since it will only free
>>>> the resource when the refcount reaches 0).
>>>>
>>>> Any thoughts?  I'd appreciate some more testers too...  I'm not
>>>> quite sure about the generic DoDestroyDrawable change, but if the
>>>> refcount is always 1 there anyway it shouldn't make a difference
>>>> and seems more correct.
>>>>       
>>> The __GLXdrawable is a reference counted object, and the glx code
>>> references it in a couple of places: when there's an X resource
>>> pointing to it (a GLXWindow, GLXPixmap or GLXPbuffer) and when it's
>>> the current drawable or readable for a context.  The __GLXdrawable is
>>> allocated by the backend in use (dri, dri2 or swrast) and must be
>>> freed by the same backend (don't mix alloc and free across abstraction
>>> barriers).  We unref the __GLXdrawable when we either set a different
>>> drawable/readable and when the X resource goes away.
>>>
>>> The leak is (as you pointed out before) because we NULL the pDraw
>>> pointer before calling the backend destructor, which then can't unref
>>> the DRI2Drawable, which we then leak.  You had the right fix in the
>>> originial post, we just need to not touch glxPriv after
>>> __glXUnrefDrawable if it got destroyed.  I suggest this change to
>>> DrawableGone in irc:
>>>
>>>     refcount = glxPriv->refcount;
>>>     __glXUnrefDrawable(glxPriv);
>>>     if (refcount > 1) {
>>>         glxPriv->pDraw = NULL;
>>>         glxPriv->drawId = 0;
>>>     }
>>>     
>>
>> Yep, that seems to work too...  Magnus or Vasily can you guys confirm?
>>
>> Thanks,
>>   
> Memory leak problem seems resolved, but I still see X crash with:
> (gdb) bt
> #0  0xb7fd5424 in __kernel_vsyscall ()
> #1  0x03155660 in raise () from /lib/libc.so.6
> #2  0x03157028 in abort () from /lib/libc.so.6
> #3  0x031925bd in __libc_message () from /lib/libc.so.6
> #4  0x031987e4 in malloc_printerr () from /lib/libc.so.6
> #5  0x0319c441 in _int_realloc () from /lib/libc.so.6
> #6  0x0319d176 in realloc () from /lib/libc.so.6
> #7  0x08131002 in Xrealloc (ptr=0x6, amount=0) at utils.c:1133
> #8  0x0806d10b in dixAllocatePrivate (privates=0x91487c8, key=0xb7e90a3c)
>     at privates.c:129
> #9  0x0806d1cc in dixSetPrivate (privates=0x91487c8, key=0xb7e90a3c, val=0x0)
>     at privates.c:193
> #10 0xb7e8eca1 in DRI2DestroyDrawable (pDraw=0x91487b0) at dri2.c:218
> #11 0xb7eee668 in __glXDRIdrawableDestroy (drawable=0x9205ff0) at glxdri2.c:108
> #12 0xb7ee49bb in __glXUnrefDrawable (glxPriv=0x9205ff0) at glxutil.c:58
> #13 0xb7ee3183 in DrawableGone (glxPriv=0x9205ff0, xid=12583326)
>     at glxext.c:131
> #14 0x0806efdc in FreeResource (id=12583326, skipDeleteFuncType=0)
>     at resource.c:561
> #15 0xb7edffa6 in DoDestroyDrawable (cl=<value optimized out>,
>     glxdrawable=12583326, type=1) at glxcmds.c:1225
> #16 0xb7ee340a in __glXDispatch (client=0x8d79db8) at glxext.c:523
> #17 0x080874cf in Dispatch () at dispatch.c:437
> ---Type <return> to continue, or q <return> to quit---
> #18 0x0806c69d in main (argc=2, argv=0xbf9d2754, envp=Cannot access memory at
> address 0xbde
>
> Thanks
> 	--Shuang
> ) at main.c:397
>   
>

Just debug a bit, check out this series of calls in DRI2DestroyDrawable when X
crashed:
in (*ds->DestroyBuffers)(pDraw, pPriv->buffers, pPriv->bufferCount);
  Xfree: free(0x9eef330)
  Xfree: free(0x9eeef20)
  Xfree: free(0x9efdde0)
  Xfree: free(0x9efce08)
  Xfree: free(0x9eee8b0)
  Xrealloc: ptr = 0x9efaf20
  Xrealloc: amount = 384
  Xfree: free(0x9efcd18)
  Xfree: free(0x9ef8468)
  Xrealloc: ptr = 0x9efa278
  Xrealloc: amount = 384
  Xfree: free(0x9efcd18)
  Xfree: free(0x9ef9808)
  Xfree: free(0x9eeef38)
  Xfree: free(0x9efa648)
  Xfree: free(0x9efd788)
in dixSetPrivate(&pPixmap->devPrivates, dri2PixmapPrivateKey, NULL);
  Xrealloc: ptr = 0x9efce08
  Xrealloc: amount = 512

So dixSetPrivate is trying to realloc memory at 0x9efce08, which is alreay
freed  in DetroyBuffers. So maybe we should also do this:

diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c
index 0f2e24b..dddcfdc 100644
--- a/hw/xfree86/dri2/dri2.c
+++ b/hw/xfree86/dri2/dri2.c
@@ -204,9 +204,6 @@ DRI2DestroyDrawable(DrawablePtr pDraw)
     if (pPriv->refCount > 0)
        return;

-    (*ds->DestroyBuffers)(pDraw, pPriv->buffers, pPriv->bufferCount);
-    xfree(pPriv);
-
     if (pDraw->type == DRAWABLE_WINDOW)
     {
        pWin = (WindowPtr) pDraw;
@@ -217,6 +214,9 @@ DRI2DestroyDrawable(DrawablePtr pDraw)
        pPixmap = (PixmapPtr) pDraw;
        dixSetPrivate(&pPixmap->devPrivates, dri2PixmapPrivateKey, NULL);
     }
+
+    (*ds->DestroyBuffers)(pDraw, pPriv->buffers, pPriv->bufferCount);
+    xfree(pPriv);
 }

 Bool

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.x.org/archives/xorg/attachments/20090330/5cc307e8/attachment.html>