[Xcb] [PATCH] Force XCB event structures with 64-bit extended fields to be packed.

[Kenneth Graunke]

With the advent of the Present extension, some events (such as
PresentCompleteNotify) now use native 64-bit types on the wire.

For XGE events, we insert an extra "uint32_t full_sequence" field
immediately after the first 32 bytes of data.  Normally, this causes
the subsequent fields to be shifted over by 4 bytes, and the structure
to grow in size by 4 bytes.  Everything works fine.

However, if event contains 64-bit extended fields, this may result in
the compiler adding an extra 4 bytes of padding so that those fields
remain aligned on 64-bit boundaries.  This causes the structure to grow
by 8 bytes, not 4.  Unfortunately, XCB doesn't realize this, and
always believes that the length only increased by 4.  read_packet()
then fails to malloc enough memory to hold the event, and the event
processing code uses the wrong offsets.

To fix this, mark any event structures containing 64-bit extended
fields with __attribute__((__packed__)).

Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
Reviewed-by: Keith Packard <keithp at keithp.com>
---
 src/c_client.py | 12 +++++++++---
 src/xcb.h       |  2 ++
 2 files changed, 11 insertions(+), 3 deletions(-)

Keith noted that the compiler is required to generate code to do an
unaligned load, so straight reads from this should not cause problems.

On #xorg-devel, dalias pointed out that on some architectures, it isn't
safe to take the address of an unaligned field and pass it around as a
pointer.  Keith and Eric noted that this is really unlikely, given that
we're only applying this workaround to events, which should only ever be
read...

This preserves the existing ABI on x86 systems, and fixes amd64 systems.
Unlike other proposed fixes, it doesn't require a major .so bump, nor a
new release of Xlib.  Not being able to take a pointer to certain fields
in two of the new Present events is an irritating restriction.  But we've
only come up with one other solution that doesn't break ABI - handling
uint64_t as a pair of uint32_t with funky accessor functions.  That seems
even more irritating.  As I see it, this is awful, but strictly an
improvement over what we have today.

In the future, if we decide to bump the .so version and break ABI, I
agree with Daniel that we should clean up this mess by removing the
full_sequence field (or at least not putting it in the middle!).

Keith suggested only applying the "packed" attribute to structures that
required packing.  So I've added a bit of Python code to limit that.

Feedback welcome...thanks all.

diff --git a/src/c_client.py b/src/c_client.py
index 99fd307..ff27439 100644
--- a/src/c_client.py
+++ b/src/c_client.py
@@ -1762,7 +1762,7 @@ def c_simple(self, name):
         # Iterator
         _c_iterator(self, name)
 
-def _c_complex(self):
+def _c_complex(self, force_packed = False):
     '''
     Helper function for handling all structure types.
     Called for all structs, requests, replies, events, errors.
@@ -1772,7 +1772,7 @@ def _c_complex(self):
     _h('/**')
     _h(' * @brief %s', self.c_type)
     _h(' **/')
-    _h('typedef %s %s {', self.c_container, self.c_type)
+    _h('typedef %s%s %s {', self.c_container, ' XCB_PACKED' if force_packed else '', self.c_type)
 
     struct_fields = []
     maxtypelen = 0
@@ -2902,6 +2902,7 @@ def c_event(self, name):
     # events while generating the structure for them. Otherwise we would read
     # garbage (the internal full_sequence) when accessing normal event fields
     # there.
+    force_packed = False
     if hasattr(self, 'is_ge_event') and self.is_ge_event and self.name == name:
         event_size = 0
         for field in self.fields:
@@ -2911,6 +2912,11 @@ def c_event(self, name):
                 full_sequence = Field(tcard32, tcard32.name, 'full_sequence', False, True, True)
                 idx = self.fields.index(field)
                 self.fields.insert(idx + 1, full_sequence)
+
+                # If the event contains any 64-bit extended fields, they need
+                # to remain aligned on a 64-bit boundary.  Adding full_sequence
+                # would normally break that; force the struct to be packed.
+                force_packed = True in (f.type.size == 8 and f.type.is_simple for f in self.fields[(idx+1):])
                 break
 
     _c_type_setup(self, name, ('event',))
@@ -2920,7 +2926,7 @@ def c_event(self, name):
 
     if self.name == name:
         # Structure definition
-        _c_complex(self)
+        _c_complex(self, force_packed)
     else:
         # Typedef
         _h('')
diff --git a/src/xcb.h b/src/xcb.h
index e62c985..73c77a3 100644
--- a/src/xcb.h
+++ b/src/xcb.h
@@ -51,6 +51,8 @@ extern "C" {
  * @file xcb.h
  */
 
+#define XCB_PACKED __attribute__((__packed__))
+
 /**
  * @defgroup XCB_Core_API XCB Core API
  * @brief Core API of the XCB library.
-- 
1.8.5.2