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

[Mark Kettenis]

> Date: Sun, 29 Dec 2013 01:03:31 -0800
> From: Josh Triplett <josh at joshtriplett.org>
> 
> 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.

That isn't the right answer.  The 64-bit fields will be misaligned and
accessing them on a strict alignment architecture will trigger a bus
error.

Looks like the logic for inserting the extra "full_sequence" field is
busted.  To support 64-bit fields in the "extended part" of the event,
we need 32 bits of padding after the "full_sequence" member.  That
padding also needs to be added to xcb_generic_event_t to make sure
that xcb_in.c:read_packet() puts the XGE event data in the right spot.