[PATCH xserver] was: Re: Two long-standing server bugs

[Thomas Klausner]

On Wed, Sep 04, 2013 at 04:05:51PM +1000, Peter Hutterer wrote:
> On Tue, Sep 03, 2013 at 09:47:28AM +0200, Thomas Klausner wrote:
> > On Sun, Sep 01, 2013 at 01:37:31PM -0400, der Mouse wrote:
> > > The first is that CreateCursor (Xlib call XCreatePixmapCursor) with a
> > > non-bitmap source pixmap and a None mask is, as I read it, supposed to
> > > error out with BadMatch, but doesn't.  This is the one I fixed.
> > > 
> > > The second is that pixmap cursors created with a None mask get
> > > extended, in some cases, to the right with additional pixels in the
> > > background colour, usually to a multiple of 32 pixels.  I don't know
> > > exactly when this happens; in my experience it correlates (almost?)
> > > perfectly with using video hardware that's behind a PCI bus, though I
> > > feel reasonably sure that it's more a question of which ddx device code
> > > is in use.  This is not, strictly, contrary to the spec, since cursors
> > > "may be transformed arbitrarily to meet display limitations", but,
> > > since it works correctly (in the cases I've seen) when the client
> > > specifies a mask pixmap full of 1s, there clearly is no actual need to
> > > mangle cursors specified with a mask of None this way.  So I think
> > > calling it a bug is fair.  This is the one I kinda-sorta fixed.
> > > 
> > > Here are my patches.  Pathnames are based on NetBSD's /usr/xsrc; I
> > > trust people here can adapt as necessary. :-)  Anyone is welcome to
> > > pick these up, either to be fed back into the main tree or for private
> > > use, as desired.  (In case an explicit statement to this effect is
> > > helpful, I release any intellectual property rights I may have in these
> > > patches into the public domain.)
> > > 
> > > First, the `missing BadMatch' bug.
> > > 
> > > --- a/external/mit/xorg-server/dist/dix/dispatch.c
> > > +++ b/external/mit/xorg-server/dist/dix/dispatch.c
> > > @@ -2863,9 +2863,10 @@ ProcCreateCursor (ClientPtr client)
> > >      }
> > >      else if (  src->drawable.width != msk->drawable.width
> > >  	    || src->drawable.height != msk->drawable.height
> > > -	    || src->drawable.depth != 1
> > >  	    || msk->drawable.depth != 1)
> > >  	return (BadMatch);
> > > +    if (src->drawable.depth != 1)
> > > +	return (BadMatch);
> > >  
> > >      width = src->drawable.width;
> > >      height = src->drawable.height;
> > 
> > I've adapted this patch to the current xserver git sources, attached.
> > Please review.
> >  Thomas
> 
> > From 8057fc810fb2815ea4c069ac9c1bc77709afb778 Mon Sep 17 00:00:00 2001
> > From: Thomas Klausner <wiz at NetBSD.org>
> > Date: Tue, 3 Sep 2013 09:44:18 +0200
> > Subject: [PATCH:xserver] Fix bug in cursor handling.
> > 
> > CreateCursor (Xlib call XCreatePixmapCursor) with a non-bitmap
> > source pixmap and a None mask is supposed to error out with BadMatch,
> > but didn't.
> > 
> > From der Mouse <mouse at Rodents-Montreal.ORG>
> > 
> > Signed-off-by: Thomas Klausner <wiz at NetBSD.org>
> > ---
> >  dix/dispatch.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/dix/dispatch.c b/dix/dispatch.c
> > index 51d0de2..ae3b97b 100644
> > --- a/dix/dispatch.c
> > +++ b/dix/dispatch.c
> > @@ -2874,9 +2874,12 @@ ProcCreateCursor(ClientPtr client)
> >      }
> >      else if (src->drawable.width != msk->drawable.width
> >               || src->drawable.height != msk->drawable.height
> > -             || src->drawable.depth != 1 || msk->drawable.depth != 1)
> > +             || msk->drawable.depth != 1)
> >          return BadMatch;
> >  
> > +    if (src->drawable.depth != 1)
> > +        return (BadMatch);
> > +
> 
> I'm staring hard at this and struggling to see the change. The previous code
> already returned BadMatch for a src depth of != 1. 

Yeah, it took me a long time too.

The difference is:

Before the change, src->drawable.depth is only
compared in the else-if case -- so when the 'if' evaluates to true, it
is not checked. 

After the change, src->drawable.depth is _always_ checked, independent
of the 'if' before.
 Thomas