[RFC] glx: fix DRI2 memory leak

[Jesse Barnes]

On Wed, 01 Apr 2009 17:46:08 +0200
Michel Dänzer <michel at daenzer.net> 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;
>  }

Yeah that looks reasonable.  Hopefully Kristian will be ok with it too.

As for the potential for the drawable to be freed before we get here
I'm not sure.  It seems like this should be the only place where it gets
freed, but there's a lot of code to trace to be sure of that.

-- 
Jesse Barnes, Intel Open Source Technology Center