[Xcb] XGE malloc bug - too small due to xcb struct padding

[Josh Triplett]

On Sun, Dec 29, 2013 at 12:54:10AM -0800, Kenneth Graunke wrote:
> Hello,
> 
> I believe I've found a bug in libxcb's read_packet() function where, for
> XGE events larger than 32-bytes, the size of the malloc'd buffer may be
> too small.
> 
> Specifically, I'm looking at the PresentCompleteNotify event:
> 
> (gdb) ptype xcb_present_complete_notify_event_t
> type = struct xcb_present_complete_notify_event_t {
>     uint8_t response_type;
>     uint8_t extension;
>     uint16_t sequence;
>     uint32_t length;
>     uint16_t event_type;
>     uint8_t kind;
>     uint8_t mode;
>     xcb_present_event_t event;
>     xcb_window_t window;
>     uint32_t serial;
>     uint64_t ust;
>     uint32_t full_sequence;
>     uint64_t msc;
> }
> 
> Note that XCB's structure doesn't match the X protocol due to the extra
> uint32_t full_sequence field, right at the 32 byte boundary.
> 
> When allocating the XCB struct, it looks like read_packet() adds an
> extra sizeof(uint32_t) to accommodate this extra field:
> 
>     bufsize = length + eventlength + nfd * sizeof(int)  +
>         (genrep.response_type == XCB_REPLY ? 0 : sizeof(uint32_t));
>                                                  ^^^^^^^^^^^^^^^^
> 
> However, this appears to be insufficient.  In this case, the
> PresentCompleteNotify event is 40 bytes on the wire, while the XCB
> struct is actually *48 bytes*, due to extra padding:
> 
> (gdb) p (int) &((xcb_present_complete_notify_event_t *) 0)->full_sequence
> $1 = 32
> (gdb) p (int) &((xcb_present_complete_notify_event_t *) 0)->msc
> $2 = 40
> 
> So we've allocated 44 bytes when we should have allocated 48 bytes.
> Reading or writing msc results in scribbling off the end of the buffer.
>  ('valgrind glxgears' on a 64-bit system with DRI3 reproduces this.)
> 
> I'm not sure how best to fix this.  The existing code bases the length
> entirely on the protocol description, but this extra padding doesn't
> exist on the wire...it's purely in XCB's structures.  Any ideas?

I don't think XCB is taking that padding into account when copying the
pieces of the structure around to accomodate the full_sequence field,
either.  The right answer is probably to mark the structure as packed.

If we ever have the opportunity to redesign these structures, then we
should likely move the full_sequence field to the beginning (and make it
64-bit).

- Josh Triplett