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

[Adam Jackson]

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.  All you
can do in the client is optimize out sending some requests you know will
be no-ops.

But thanks for prompting me to re-read that code, it's less thread-safe
than I remembered.

- ajax
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20111219/2ed42122/attachment-0001.pgp>