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

Daniel Martin consume.noise at gmail.com
Sun Dec 29 04:35:41 PST 2013


On Sun, Dec 29, 2013 at 01:03:31AM -0800, Josh Triplett wrote:
> 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).

I'd rather drop the full_sequence then shifting it around. I had send
patches to do so .o0(Did I?), which got NAKed as it requires some nasty
coordination with XLib (the first thing XLib does with GE events: drop
the full_sequence). If we shift the field around we would run into the
same problem.

So, why not drop the field if we're going to touch the structures?


Cheers,
    Daniel Martin


More information about the Xcb mailing list