[PATCH xserver v2] Fix id in error when resource does not exist

[Peter Harris]

On 2016-10-06 1:07 PM, Adam Jackson wrote:
> On Thu, 2016-09-29 at 11:17 -0400, Peter Harris wrote:
>> Always set client->errorValue before returning an error.
> 
> As sensible as this seems, I think it's wrong.  I ran across something
> similar a few years ago:
> 
> https://lists.freedesktop.org/archives/xorg-devel/2011-June/023462.html
> 
> BadMatch errors are specified as not filling in errorValue, so...
> 
>> @@ -1220,11 +1220,13 @@ dixLookupResourceByType(void **result, XID id, RESTYPE rtype,
>>              if (res->id == id && res->type == rtype)
>>                  break;
>>      }
>> +    if (client) {
>> +        client->errorValue = id;
>> +    }
>>      if (!res)
>>          return resourceTypes[rtype & TypeMask].errorValue;
>>  
>>      if (client) {
>> -        client->errorValue = id;
>>          cid = XaceHook(XACE_RESOURCE_ACCESS, client, id, res->type,
>>                         res->value, RT_NONE, NULL, mode);
>>          if (cid == BadValue)
> 
> ... imagine ChangeGC specifying more than one of {tile, stipple, font,
> clipmask pixmap}.  Whichever one is looked up last will be the
> errorValue thrown with the eventual BadMatch, which beyond being
> against the spec also means you might be misled about which resource is
> failing the match.

But that's already the case with the current code. When the lookup
succeeds, client->errorValue is set to whatever was looked up (by the
line removed in this patch). Yes, the next BadMatch is going to have
nonsense in a few bytes defined to be padding bytes, but that was
already the case.

This patch only moves the setting of client->errorValue above the
"Resource Not Found" return. So the error definitely[1] isn't BadMatch,
and we want the ID to be the thing that actually doesn't exist, instead
of the previous thing that does exist.

This patch only makes a difference[1] when the error value isn't BadMatch.

The thing that bit me was a BadGC with errorValue set to the drawable of
the offending request, which took some head-scratching to figure out.

Peter Harris

[1] Unless an extension calls SetResourceTypeErrorValue(resType,
BadMatch). Which would be crazy.
-- 
               Open Text Connectivity Solutions Group
Peter Harris                    http://connectivity.opentext.com/
Research and Development        Phone: +1 905 762 6001
pharris at opentext.com            Toll Free: 1 877 359 4866