[RFC] glx: fix DRI2 memory leak

[Shuang He]

Michel Dänzer wrote:
> On Mon, 2009-03-30 at 13:04 +0800, Shuang He wrote:
>   
>> Shuang He wrote: 
>>     
>>> Jesse Barnes wrote: 
>>>       
>>>> On Fri, 27 Mar 2009 13:20:25 -0400
>>>> Kristian Høgsberg <krh at bitplanet.net> wrote:
>>>>         
>>>>> 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?
>>>>   
>>>>         
>>> 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
>>>       
>> 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: [...]
>>     
>
> The real problem was pointed out by Pierre Willenbrock: If
> glxPriv->pDraw is a pixmap, DrawableGone() destroys it, then later
> DRI2DestroyDrawable dereferences it... The patch below seems to work
> here - at least it hasn't crashed in a couple of hours, not sure about
> the leak yet.
>
> Note that unless I'm missing something, it might still be possible for
> this to occur with windows if the underlying DrawableRec is freed before
> this code is reached.
>
> Also, I don't really understand the logic behind clearing glxPriv->pDraw
> and ->drawId here. In particular, I'm not sure DrawableGone will ever be
> called with glxPriv->refCount > 1. If it won't, this change makes the
> assignments unreachable. And if it will, we'll again leak because the
> cleanup code won't be able to get to the underlying DrawableRec?
>
>
> diff --git a/glx/glxext.c b/glx/glxext.c
> index c882372..e74d00e 100644
> --- a/glx/glxext.c
> +++ b/glx/glxext.c
> @@ -119,17 +119,25 @@ static int ContextGone(__GLXcontext* cx, XID id)
>  static Bool DrawableGone(__GLXdrawable *glxPriv, XID xid)
>  {
>      ScreenPtr pScreen = glxPriv->pDraw->pScreen;
> +    PixmapPtr pPixmap = NULL;
> +    int refcount;
>  
>      switch (glxPriv->type) {
>  	case GLX_DRAWABLE_PIXMAP:
>  	case GLX_DRAWABLE_PBUFFER:
> -	    (*pScreen->DestroyPixmap)((PixmapPtr) glxPriv->pDraw);
> +	    pPixmap = (PixmapPtr) glxPriv->pDraw;
>  	    break;
>      }
>  
> -    glxPriv->pDraw = NULL;
> -    glxPriv->drawId = 0;
> +    refcount = glxPriv->refCount;
>      __glXUnrefDrawable(glxPriv);
> +    if (refcount > 1) {
> +	glxPriv->pDraw = NULL;
> +	glxPriv->drawId = 0;
> +    }
> +    
> +    if (pPixmap)
> +	(*pScreen->DestroyPixmap)(pPixmap);
>  
>      return True;
>  }
>
>
>   
This works for me. I don't see the memory leaks or X crashes.

Thanks
    --Shuang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.x.org/archives/xorg/attachments/20090402/6e2e1339/attachment.html>