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

Kenneth Graunke kenneth at whitecape.org
Sun Dec 29 00:54:10 PST 2013


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?

Thanks!
--Ken


More information about the Xcb mailing list