On Wed, Jul 4, 2012 at 5:03 AM, Michel Dänzer <mic...@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 <mic...@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 <mic...@daenzer.net> >> > >> wrote: >> > >> > From: Michel Dänzer <michel.daen...@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 _______________________________________________ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel