[PATCH] xkb: Don't swap strings length in the input buffer

[Peter Hutterer]

On Thu, Jan 15, 2015 at 11:17:57AM +0100, Olivier Fourdan wrote:
> XkbSetGeometry request embeds lots of strings which have their length
> encoded on a CARD16 which needs to be swapped when the server and client
> have different endianess.
> 
> The function _GetCountedString() that parses these strings from the requests
> does the swapping of the length in the input buffer directly.
> 
> However, ProcXkbSetGeometry() may call _XkbSetGeometry() more than once
> (if there is more than one keyboard), thus causing on swapped clients the
> same data to be swapped twice in memory, further causing a server crash
> because the strings lengths on the second time are way off bounds.
> 
> To allow _GetCountedString() to run reliably more than once with swapped
> clients, do not swap the data in the buffer, use a variable instead.
> 
> Signed-off-by: Olivier Fourdan <ofourdan at redhat.com>
> ---
>  xkb/xkb.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/xkb/xkb.c b/xkb/xkb.c
> index 15c7f34..f8c2635 100644
> --- a/xkb/xkb.c
> +++ b/xkb/xkb.c
> @@ -4965,10 +4965,10 @@ _GetCountedString(char **wire_inout, Bool swap)
>  
>      wire = *wire_inout;
>      plen = (CARD16 *) wire;
> +    len = *plen;
>      if (swap) {
> -        swaps(plen);
> +        swaps(&len);
>      }
> -    len = *plen;
>      str = malloc(len + 1);
>      if (str) {
>          memcpy(str, &wire[2], len);
> -- 
> 2.1.0

you could drop plen and go
   len = *(CARD16*)wire;
   ...

either way, Reviewed-by: Peter Hutterer <peter.hutterer at who-t.net>

Cheers,
   Peter