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

[Jasper St. Pierre]

Reviewed-by: Jasper St. Pierre <jstpierre at mecheye.net>


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