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

[Peter Hutterer]

On Wed, Sep 04, 2013 at 02:25:02PM -0400, Jasper St. Pierre wrote:
> Reviewed-by: Jasper St. Pierre <jstpierre at mecheye.net>

thanks for the review and the patch. merged into my tree, will be upstream
soon.

Cheers,
   Peter

> 
> 
> On Wed, Sep 4, 2013 at 2:09 PM, Thomas Klausner <wiz at netbsd.org> wrote:
> 
> > On Wed, Sep 04, 2013 at 11:13:14AM -0400, Jasper St. Pierre wrote:
> > > Yeah, this is how I would write it.
> > >
> > > Can you provide a patch, Alan?
> >
> > I tried to make the code match Alan's suggestion, attached.
> >
> > While there, I fixed a typo in the configure.ac script, also attached.
> >  Thomas
> >
> > >
> > > On Wed, Sep 4, 2013 at 2:30 AM, Alan Coopersmith <
> > > alan.coopersmith at oracle.com> wrote:
> > >
> > > > On 09/ 3/13 11:05 PM, Peter Hutterer wrote:
> > > > >>  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.
> > > >
> > > > I couldn't see it from the patch either, but a little more context than
> > > > the patch
> > > > includes helps:
> > > >
> > > >    2867     rc = dixLookupResourceByType((pointer *) &msk, stuff->mask,
> > > > RT_PIXMAP,
> > > >    2868                                  client, DixReadAccess);
> > > >    2869     if (rc != Success) {
> > > >    2870         if (stuff->mask != None) {
> > > >    2871             client->errorValue = stuff->mask;
> > > >    2872             return rc;
> > > >    2873         }
> > > >    2874     }
> > > >    2875     else if (src->drawable.width != msk->drawable.width
> > > >    2876              || src->drawable.height != msk->drawable.height
> > > >    2877              || src->drawable.depth != 1 ||
> > msk->drawable.depth !=
> > > > 1)
> > > >    2878         return BadMatch;
> > > >
> > > > So if stuff->mask is None, we do a resource lookup for a pixmap with
> > that
> > > > id,
> > > > which should fail, since it's None (aka 0).
> > > >
> > > > Thus we gp into the check at 2869, but don't match the if at 2870, so
> > don't
> > > > return.   Since 2875 is the else clause for the if we already went
> > into, we
> > > > skip over all those checks.
> > > >
> > > > The patch moves the src->drawable.depth check out of the else clause so
> > > > it's
> > > > always checked, not just in the (rc == Success) case for the mask
> > pixmap
> > > > lookup.
> > > > It could even be moved up above the lookup of the mask pixmap, since it
> > > > only
> > > > checks against the cursor image pixmap.
> > > >
> > > > I'm not sure if there's ever a reason to try to lookup a mask id of
> > None,
> > > > otherwise this might be easier to follow as:
> > > >
> > > >     /* Find and validate cursor image pixmap */
> > > >     rc = dixLookupResourceByType((pointer *) &src, stuff->source,
> > > > RT_PIXMAP,
> > > >                                  client, DixReadAccess);
> > > >     if (rc != Success) {
> > > >         client->errorValue = stuff->source;
> > > >         return rc;
> > > >     }
> > > >
> > > >     if (src->drawable.depth != 1)
> > > >        return (BadMatch);
> > > >
> > > >     /* Find and validate cursor mask pixmap, if one is provided */
> > > >     if (stuff->mask != None) {
> > > >         rc = dixLookupResourceByType((pointer *) &msk, stuff->mask,
> > > > RT_PIXMAP,
> > > >                                      client, DixReadAccess);
> > > >         if (rc != Success) {
> > > >             client->errorValue = stuff->mask;
> > > >             return rc;
> > > >         }
> > > >
> > > >         if (src->drawable.width != msk->drawable.width
> > > >              || src->drawable.height != msk->drawable.height
> > > >              || msk->drawable.depth != 1)
> > > >             return BadMatch;
> > > >     }
> > > >     else
> > > >         msk = NULL;
> > > >
> > > >
> > > > --
> > > >         -Alan Coopersmith-              alan.coopersmith at oracle.com
> > > >          Oracle Solaris Engineering - http://blogs.oracle.com/alanc
> > > > _______________________________________________
> > > > xorg-devel at lists.x.org: X.Org development
> > > > Archives: http://lists.x.org/archives/xorg-devel
> > > > Info: http://lists.x.org/mailman/listinfo/xorg-devel
> > > >
> > >
> > >
> > >
> > > --
> > >   Jasper
> >
> 
> 
> 
> -- 
>   Jasper