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

Thomas Klausner wiz at netbsd.org
Wed Sep 4 11:09:18 PDT 2013


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
-------------- next part --------------
>From ac4558c7b5ffec3cd148e992332f3b13a2b9b35a Mon Sep 17 00:00:00 2001
From: Thomas Klausner <wiz at NetBSD.org>
Date: Wed, 4 Sep 2013 20:05:51 +0200
Subject: [PATCH:xserver 1/2] 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>, changed following
comments by Alan Coopersmith <alan.coopersmith at oracle.com>.

Signed-off-by: Thomas Klausner <wiz at NetBSD.org>
---
 dix/dispatch.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/dix/dispatch.c b/dix/dispatch.c
index 51d0de2..71fda48 100644
--- a/dix/dispatch.c
+++ b/dix/dispatch.c
@@ -2864,18 +2864,25 @@ ProcCreateCursor(ClientPtr client)
         return rc;
     }
 
-    rc = dixLookupResourceByType((pointer *) &msk, stuff->mask, RT_PIXMAP,
-                                 client, DixReadAccess);
-    if (rc != Success) {
-        if (stuff->mask != None) {
+    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
+            || src->drawable.depth != 1 || msk->drawable.depth != 1)
+            return BadMatch;
     }
-    else if (src->drawable.width != msk->drawable.width
-             || src->drawable.height != msk->drawable.height
-             || src->drawable.depth != 1 || msk->drawable.depth != 1)
-        return BadMatch;
+    else
+        msk = NULL;
 
     width = src->drawable.width;
     height = src->drawable.height;
-- 
1.8.4

-------------- next part --------------
>From 2bd145cb6aa6ab406bf1c8a0775d9d470675efa1 Mon Sep 17 00:00:00 2001
From: Thomas Klausner <wiz at NetBSD.org>
Date: Wed, 4 Sep 2013 20:06:07 +0200
Subject: [PATCH:xserver 2/2] Fix typo in configure warning.

Signed-off-by: Thomas Klausner <wiz at NetBSD.org>
---
 configure.ac | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/configure.ac b/configure.ac
index d27ca23..5159420 100644
--- a/configure.ac
+++ b/configure.ac
@@ -47,7 +47,7 @@ XORG_WITH_XSLTPROC
 XORG_ENABLE_UNIT_TESTS
 XORG_LD_WRAP([optional])
 
-m4_ifndef([XORG_FONT_MACROS_VERSION], [m4_fatal([must install fontutil 1.1 or later before running autoconf/autogen])])
+m4_ifndef([XORG_FONT_MACROS_VERSION], [m4_fatal([must install font-util 1.1 or later before running autoconf/autogen])])
 XORG_FONT_MACROS_VERSION(1.1)
 
 dnl this gets generated by autoheader, and thus contains all the defines.  we
-- 
1.8.4



More information about the xorg-devel mailing list