[Mesa-dev] [PATCH] glx: block attempt to swapbuffer on pixmap. (v2)

[Michel Dänzer]

On Mon, 2011-12-19 at 10:14 -0500, Adam Jackson wrote: 
> On Mon, 2011-12-19 at 09:40 +0000, Dave Airlie wrote:
> > On Wed, Dec 7, 2011 at 6:32 PM, Eric Anholt <eric at anholt.net> wrote:
> > > On Wed,  7 Dec 2011 10:24:09 +0000, Dave Airlie <airlied at gmail.com> wrote:
> > >> From: Dave Airlie <airlied at redhat.com>
> > >>
> > >> This keeps track of the creation process and stores a drawable type,
> > >> it then blocks DRI2 from getting called if the drawable is a pixmap.
> > >>
> > >> v2: check if we have a GLX drawable, which means we aren't a pbuffer,
> > >> avoid doing flush at all since its meant to be a no-op.
> > >
> > > I still think this is the wrong way to go.  As ajax pointed out, there's
> > > all sorts of races available from trying to guess client-side, and
> > > there's no way anybody's relying on the current ("print an error message
> > > in the xorg log") behavior of the DRI2 protocol for single-buffered, so
> > > we might as well resolve that DRI2 protocol should do what we want for
> > > GLX.
> > 
> > I've been thinking about the races, and I'm not sure they really
> > apply, like we would have all those races now with the current code,
> > 
> > Look at the patch, when the glx_drawable struct is created we record
> > the type of the pixmap, we then call GetGLXDrawable, which looks the
> > XID up in the hash,
> > so if the XID has been reused or raced, then we would have to expect
> > the hash to be incorrect, or else avoid using it in a lot of other
> > places.
> 
> True, but we could fix that by moving the UnlockDisplay() to after the
> DRI2 stanza (and fixing the called code to not try to take the lock
> itself).  There'd still be a race between the GLX and DRI2 requests, in
> that someone else could Destroy that which you just created, but that's
> a race against an actively malicious client; it would have to be
> spamming the server with Destroy requests, since there's no way it can
> know what XIDs you're going to pick.
> 
> In contrast, trying to track pixmapness like you're doing here races
> against at least two app programming techniques that GLX allows and apps
> actually use: multiple threads, and XID passing between applications.
> In the latter case you'll not have fixed anything: glx_draw will be NULL
> because we've not tracked this pixmap since its creation, so we'll carry
> on with the DRI request.
> 
> The server is the only thing that knows how to handle this scenario, and
> it _can_ get this request, so you have to fix it there anyway.

Actually no, the DRI2 code in the server can't know, as this is a
perfectly legitimate request for a GLXPbuffer.


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