[PATCH] glx: Pass GLX drawable ID to DRI2CreateDrawable().

[Michel Dänzer]

On Fre, 2012-06-29 at 19:09 +0200, Michel Dänzer wrote: 
> On Fre, 2012-06-29 at 12:58 -0400, Kristian Høgsberg wrote: 
> > On Fri, Jun 29, 2012 at 12:30 PM, Michel Dänzer <michel at daenzer.net> wrote:
> > > On Fre, 2012-06-29 at 12:20 -0400, Kristian Høgsberg wrote:
> > >> On Thu, Jun 28, 2012 at 7:39 AM, Michel Dänzer <michel at daenzer.net> wrote:
> > >> > From: Michel Dänzer <michel.daenzer at amd.com>
> > >> >
> > >> > Otherwise the DRI2Drawable may retain references to the destroyed
> > >> > __GLXDRIdrawable, leading to use after free in __glXDRIinvalidateBuffers().
> > >>
> > >> That looks wrong to me.  DRI2 isn't concerned with GLX drawables, only
> > >> X drawables.  If you're destroying the GLX drawable and want to not
> > >> get invalidate callbacks, you need to destroy the DRI2DrawableRef that
> > >> DRI2CreateDrawable creates.
> > >
> > > Which is what this patch does? :) (By means of
> > > glxcmds.c:DoDestroyDrawable -> FreeResource -> DRI2DrawableGone, where
> > > the ID matches ref->id, so it calls
> > > FreeResourceByType(ref->dri2_id, ...) as well)
> > >
> > > Can you explain why the non-GLX drawable ID needs to be passed to
> > > DRI2CreateDrawable?
> > 
> > The DRI2Drawable is created for the X drawable, not the GLX drawable.
> > When the X drawable goes away the DRI2 drawable needs to go away.
> 
> And it still does. When the X drawable goes away, so does the GLX
> drawable (via a similar Resource trick), so the above sequence comes
> into play.
> 
> 
> > It works the way it does, since a pixmap can have multiple XIDs and for
> > each XID, mutliple clients could call DRI2CreateClient.  We need to
> > keep the DRI2 drawable alive for each reference for each XID.  DRI2
> > automatically reclaims the DRI2Drawable when the underlying X drawable
> > is destroyed, but that will break if you pass in the GLX drawable XID.
> 
> I don't understand how that will break given the above. Can you
> elaborate?

I can implement the more complicated fix you suggested, but I'd like to
understand why it's necessary. It should be helpful if you could
describe a specific scenario that would break with the simple fix.


-- 
Earthling Michel Dänzer           |                   http://www.amd.com
Libre software enthusiast         |          Debian, X and DRI developer