[PATCH] libX11: check size of GetReqExtra after XFlush

[Kees Cook]

Hi,

Thanks for the feedback!

On Mon, Jul 18, 2011 at 10:06:24AM -0700, Jamey Sharp wrote:
> I have a few comments, I guess.
> 
> - The indentation in the GetReqExtra definition didn't work out. Looks
>   like it should be tabs and tabs only, there.

Oh, hrm, sorry about that. I will fix it up.

> - This patch turns a buffer overrun into a null-pointer dereference if
>   there's any caller that hasn't been updated. (While you've hit all the
>   cases I can find in X.Org sources, keep in mind that there are
>   closed-source Xlib extensions out there.) I think that's an
>   improvement, but it's worth calling out in the commit message.

True, I will improve the commit message.

> - I guess this new behavior should be documented in
>   specs/libX11/AppC.xml.

Okay, noted.

> - Your mention of 16K in the commit message confused me because I wasn't
>   sure where you were getting that limit from. Note that Xlib's buffer
>   size is actually runtime-configurable, down to a minimum of 2K. Could
>   you explain it in terms of Xlib's output buffer size, please?

That was, perhaps, a bit obscure. I will rephrase this in terms of the Xlib
buffer size.

> - Changing the implementation of GetReqExtra and the rest of Xlibint.h
>   is only acceptable if code compiled using the old implementation is
>   ABI compatible with code compiled using the new implementation. I
>   believe you're good on that count, but I wanted to point it out.

Right -- I tried to be careful with this.

> On the whole, I believe this is an improvement. With the above
> corrections, I'd be happy to commit this.

Thanks, I'll send a v2 shortly.

-Kees

-- 
Kees Cook
Ubuntu Security Team