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 < [email protected]> wrote: > On 09/ 3/13 11:05 PM, Peter Hutterer wrote: > >> From 8057fc810fb2815ea4c069ac9c1bc77709afb778 Mon Sep 17 00:00:00 2001 > >> From: Thomas Klausner <[email protected]> > >> 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 <[email protected]> > >> > >> Signed-off-by: Thomas Klausner <[email protected]> > >> --- > >> 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- [email protected] > Oracle Solaris Engineering - http://blogs.oracle.com/alanc > _______________________________________________ > [email protected]: X.Org development > Archives: http://lists.x.org/archives/xorg-devel > Info: http://lists.x.org/mailman/listinfo/xorg-devel > -- Jasper
_______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
