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

[Peter Hutterer]

On Sat, Jan 25, 2014 at 12:30:05AM +0000, Daniel Stone wrote:
> 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

right, not disagreeing with any of the above. I'm either seeing more parens
or I'm otherwise confused. which isn't surprising given that I should be
looking at a beer glass, not at pointer casting at this time of the day.

if you look at the modified line after the patch:

    CARD32 *mod = ((CARD32 *) (req + 1) + mask_len) + local_modifiers;

we have (req + 1) points to the first byte after the request. that is cast to
a CARD32*, at which point we add mask_len and local_modifiers, both in
4-byte units. the outermost set of brackets is superfluous and the confusing
bit.

req + 1 is in parens, so we can reduce it to 'a', at which pointer we're
looking at the difference between
  (uint32_t*)a + 1 + 1 vs. ((uint32_t*)a + 1) + 1

Or, to add to your test:

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

$ ./foo
9 9

Cheers,
   Peter, off to blend in at the local RSL