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

[Jasper St. Pierre]

Yeah, this is how I would write it.

Can you provide a patch, Alan?


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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.x.org/archives/xorg-devel/attachments/20130904/093b235f/attachment.html>