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

[Kristian Høgsberg]

On Wed, Jul 4, 2012 at 5:03 AM, Michel Dänzer <michel at daenzer.net> wrote:
> 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.

I'm not saying it wont work, just that I don't want to revisit all the
different destruction paths that are possible and see if what you
propose works.  This code is a nightmare, but I think that with the
way the DRI2 ref-counting and destruction code works now, it's been
pretty stable.  If you want to use it in a different way, go for it,
to me it just seems much easier and safer to just use the code the way
it was supposed to work instead of saving a bit of typing and then
have to think through all the glx/dri2 destruction paths again.

Kristian