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

[Kenneth Graunke]

On 12/29/2013 01:03 AM, 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.

You're right, of course - expanding the allocation fixed the out of
bounds memory access, but the msc values were still bogus.

Using __attribute__((packed)) does indeed fix the problem, and makes
vblank synchronization actually work with DRI3/Present on a 64-bit system.

> 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

Yeah...that would be a lot nicer.