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

[Alan Coopersmith]

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