Bug 20137

So as I understand it, this problem arises because C requires unions be aligned according to their largest member.  Thus, DBusBasicValue is going to have 64 bit alignment.  This would be fine, but in a number of cases we're type-punning from a pointer to a non-union type, which isn't guaranteed to have 64 alignment.

An example of this call chain is
    _dbus_header_cache_revalidate
which allocates on the stack:
    unsigned char field_code;
Then it passes the address of that stack variable to:
    _dbus_type_reader_read_basic (&sub, &field_code);
which calls _dbus_marshal_read_basic, type-punning it to a DBusBasicValue where we expect 6

A solid fix I think would be to make _dbus_marshal_read_basic actually take DBusBasicValue * instead of void *, thus ensuring callers have aligned it correctly.  The other bonus of doing this is that we *also* fix strict-aliasing issues, because we're accessing the memory through a union.

That's nontrivial though.  And actually investigating more, a lot of things call _dbus_type_reader_read_basic, and in fact we expose this through the API, where we expect application programmers to pass "fundamental" pointers through e.g. dbus_message_get_args_valist.

So we can't avoid type-punning, and that brings us back to this patch, which is probably then the only fix.  To be complete actually we should probably cut out the DBusBasicValue* middleman here and just cast to volatile fundamental pointers in all cases.

Created attachment 23284 [details] [review]
0001-Bug-20137-Fix-alignment-usage-when-demarshaling-ba.patch

Any comments appreciated, particularly from the original author of the earlier patch if possible.

The proposed patch looks pretty good; the only change I request is to make
the "volatile char *" in the DBUS_TYPE_BYTE case "volatile unsigned char *",
as the latter will generate better code (GCC 4.3.1-6 with -O2).

A few other observations:

1. the DBUS_TYPE_INT16 will now generate the correct code to handle a
   mis-aligned destination address,
2. the DBUS_TYPE_INT32 will still cause unaligned accesses if the destination
   address is mis-aligned
3. users of _dbus_marshal_read_basic() appear to have been aligning correctly
   for 16-bit and 32-bit destinations, as I've never observed unaligned accesses
   from dbus for anything, once the byte issue was fixed. 

Thanks for fixing this onxce and for all... :-)

--Jay++

Thanks for the review, I've changed it to use unsigned as you suggested, and pushed to master (commit 7de15965c263dccf22b08ffb5939f37f7043795d).

Regarding 2) the destination should be 4 byte aligned always; the protocol requires it.