[PATCH v2] Xi: fix modifier offset in XIPassiveGrab swapping function

[Daniel Stone]

Hi,

On 24 January 2014 23:14, Peter Hutterer <peter.hutterer at who-t.net> wrote:
> On 25/01/2014 08:48 , Alan Coopersmith wrote:
>>
>> On 01/24/14 12:05 AM, Peter Hutterer wrote:
>>>
>>> The request is followed by mask_len 4-byte units, then followed by the
>>> actual
>>> modifiers.
>>>
>>> Also fix up the swapping test, which had the same issue.
>>>
>>> Reported-by: Alan Coopersmith <alan.coopersmith at oracle.com>
>>> Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
>>> ---
>>> Changes to v1:
>>> - turns out the swapping test has the same bug, which explains why it
>>> didn't
>>>    trigger
>>>
>>>   Xi/xipassivegrab.c                      | 2 +-
>>>   test/xi2/protocol-xipassivegrabdevice.c | 9 ++++++++-
>>>   2 files changed, 9 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/Xi/xipassivegrab.c b/Xi/xipassivegrab.c
>>> index eccec0a..8aba977 100644
>>> --- a/Xi/xipassivegrab.c
>>> +++ b/Xi/xipassivegrab.c
>>> @@ -63,7 +63,7 @@ SProcXIPassiveGrabDevice(ClientPtr client)
>>>       swaps(&stuff->mask_len);
>>>       swaps(&stuff->num_modifiers);
>>>
>>> -    mods = (uint32_t *) &stuff[1];
>>> +    mods = (uint32_t *) &stuff[1] + stuff->mask_len;
>>>
>>>       for (i = 0; i < stuff->num_modifiers; i++, mods++) {
>>>           swapl(mods);
>>> diff --git a/test/xi2/protocol-xipassivegrabdevice.c
>>> b/test/xi2/protocol-xipassivegrabdevice.c
>>> index 1e2341e..31ef3d2 100644
>>> --- a/test/xi2/protocol-xipassivegrabdevice.c
>>> +++ b/test/xi2/protocol-xipassivegrabdevice.c
>>> @@ -137,6 +137,7 @@ request_XIPassiveGrabDevice(ClientPtr client,
>>> xXIPassiveGrabDeviceReq * req,
>>>   {
>>>       int rc;
>>>       int local_modifiers;
>>> +    int mask_len;
>>>
>>>       rc = ProcXIPassiveGrabDevice(&client_request);
>>>       assert(rc == error);
>>> @@ -153,10 +154,11 @@ request_XIPassiveGrabDevice(ClientPtr client,
>>> xXIPassiveGrabDeviceReq * req,
>>>       swaps(&req->deviceid);
>>>       local_modifiers = req->num_modifiers;
>>>       swaps(&req->num_modifiers);
>>> +    mask_len = req->mask_len;
>>>       swaps(&req->mask_len);
>>>
>>>       while (local_modifiers--) {
>>> -        CARD32 *mod = ((CARD32 *) (req + 1)) + local_modifiers;
>>> +        CARD32 *mod = ((CARD32 *) (req + 1) + mask_len) +
>>> local_modifiers;
>>
>>
>> Doesn't that need to be outside the parens that cast to (CARD32) so it
>> adds in CARD32-sized units instead of in xXIPassiveGrabDeviceReq sized
>> units?
>>
>> i.e. ((CARD32 *) (req + 1)) + mask_len + local_modifiers;
>
>
> Oh, right, that would make it more obvious. but the effect is the same,
> isn't it? as long as it's not inside the (req + 1) condition, the first arg
> is already cast and the + 1 is a 4-byte.
>
> so (int*)a + 1 + 1 is the same as ((int*)a + 1) + 1, at least that's what my
> quick test here showed.
>
> I'll fix it up locally though.

You've introduced a subtle parens change though: (int *)(a + 1) is not
the same as (int *) a + 1, or ((int *) a) + 1.

In the former case, a will be advanced by 1 of whatever type it's
declared to, then cast to int (at which point all pointer arithmetic
occurs on int *-sized units); in the latter two, a will be advanced by
1 int *-sized unit.

This demonstrates the difference between Alan's and your examples:

nightslugs:~% cat foo.c
#include <stdint.h>
#include <stdio.h>

int main(int argc, char *argv[])
{
uint8_t *byte = NULL;
uint32_t *word = NULL;

printf("%d %d\n", (uint32_t *) byte + 1 + 1, (uint32_t *) (byte + 1) + 1);
printf("%d %d\n", (uint32_t *) word + 1 + 1, (uint32_t *) (word + 1) + 1);

return 0;
}
nightslugs:~% ./foo
8 5
8 8

Cheers,
Daniel