Bug 94136

Is this theoretical or actually observed? Just so we know what to set as priority.

Good question. I did observe it, but in my D-Bus bindings for Rust. Rust does not copy the padding.

I did a quick test with gcc (no optimisation flags), and it does copy the padding on assignment. It would be interesting to see what Clang/LLVM does (Rust uses LLVM as backend).

(In reply to David Henningsson from comment #0)
> According to the documentation, "DBusMessageIter contains no allocated
> memory; it need not be freed, and can be copied by assignment or memcpy()".

With hindsight, I think this was a design mistake: this would be much simpler if the iterator was a heap-allocated opaque object. (Putting it on the stack, not requiring freeing and not having any spare pointer members has apparently been really painful for people experimenting with GVariant serialization, as used in kdbus.)

It's possible to move from heap- to stack-allocated (become less opaque) over time, but it isn't possible to move from stack- to heap-allocated without either breaking ABI or introducing a second, parallel API and ABI for the heap-allocated version.

Created attachment 121762 [details]
standalone C program to print struct layouts

The layouts used here are the same as the real ones, except that I've wrapped the bitfields in a union with an ordinary dbus_uint32_t. Requires gcc for __builtin_offsetof().

On ILP32 (Linux i386):

placeholder:
0-3 dummy1
4-7 dummy2
8-11 dummy3
12-15 dummy4
16-19 dummy5
20-23 dummy6
24-27 dummy7
28-31 dummy8
32-35 dummy9
36-39 dummy10
40-43 dummy11
44-47 pad1
48-51 pad2
52-55 pad3
size: 56

reader:
0-3 message
4-7 bitfields.space
8-11 u.reader.bitfields.space
12-15 u.reader.type_str
16-19 u.reader.type_pos
20-23 u.reader.value_str
24-27 u.reader.value_pos
28-31 u.reader.klass
32-35 u.reader.u.array.start_pos
size: 40

writer:
0-3 message
4-7 bitfields.space
8-11 u.writer.bitfields.space
12-15 u.writer.type_str
16-19 u.writer.type_pos
20-23 u.writer.value_str
24-27 u.writer.value_pos
28-31 u.writer.u.array.start_pos
32-35 u.writer.u.array.len_pos
36-39 u.writer.u.array.element_type_pos
size: 40

On LP64 (Linux amd64; should also be valid for LLP64, i.e. 64-bit Windows, because we don't use long here anyway):

placeholder:
0-7 dummy1
8-15 dummy2
16-19 dummy3
20-23 dummy4
24-27 dummy5
28-31 dummy6
32-35 dummy7
36-39 dummy8
40-43 dummy9
44-47 dummy10
48-51 dummy11
52-55 pad1
56-59 pad2
64-71 pad3
size: 72

reader:
0-7 message
8-11 bitfields.space
16-19 u.reader.bitfields.space
24-31 u.reader.type_str
32-35 u.reader.type_pos
40-47 u.reader.value_str
48-51 u.reader.value_pos
56-63 u.reader.klass
64-67 u.reader.u.array.start_pos
size: 72

writer:
0-7 message
8-11 bitfields.space
16-19 u.writer.bitfields.space
24-31 u.writer.type_str
32-35 u.writer.type_pos
40-47 u.writer.value_str
48-51 u.writer.value_pos
52-55 u.writer.u.array.start_pos
56-59 u.writer.u.array.len_pos
60-63 u.writer.u.array.element_type_pos
size: 72

Slightly annotated version:

-------- = padding
 "       = field above, continued

On ILP32 (Linux i386):

placeholder:       reader:            writer:
0-3 dummy1         message            message
4-7 dummy2         bitfields          bitfields
8-11 dummy3        reader.bitfields   writer.bitfields
12-15 dummy4       reader.type_str    writer.type_str
16-19 dummy5       reader.type_pos    writer.type_pos
20-23 dummy6       reader.value_str   writer.value_str
24-27 dummy7       reader.value_pos   writer.value_pos
28-31 dummy8       reader.klass       array.start_pos
32-35 dummy9       array.start_pos    array.len_pos
36-39 dummy10      --------           array.element_type_pos
40-43 dummy11      --------           --------
44-47 pad1         --------           --------
48-51 pad2         --------           --------
52-55 pad3         --------           --------
size: 56

On LP64 (Linux amd64; should also be valid for LLP64, i.e. 64-bit Windows,
because we don't use long here anyway):

placeholder:       reader:
0-3   dummy1       message           message
4-7    "            "                 "
8-11  dummy2       bitfields         bitfields
12-15  "           --------          --------
16-19 dummy3       reader.bitfields  writer.bitfields
20-23 dummy4       --------          --------
24-27 dummy5       reader.type_str   writer.type_str
28-31 dummy6        "                 "
32-35 dummy7       reader.type_pos   writer.type_pos
36-39 dummy8       --------          --------
40-43 dummy9       reader.value_str  writer.value_str
44-47 dummy10       "                 "
48-51 dummy11      reader.value_pos  writer.value_pos
52-55 pad1         --------          array.start_pos
56-59 pad2         klass             array.len_pos
60-63 --------      "                array.element_type_pos
64-67 pad3         array.start_pos   --------
68-71  "           --------          --------
size: 72

Created attachment 121768 [details] [review]
[1/4] dbus-internals: add _DBUS_ALIGNOF

This is useful when making static assertions about our types'
properties.

Created attachment 121769 [details] [review]
[2/4] DBusMessage: assert the properties we need  DBusMessageIter to have

We already asserted that DBusMessageIter must be at least as large
as DBusMessageRealIter (so that casting DBusMessageIter * to
DBusMessageRealIter * does not result in overflowing the stack
variable). Also assert that it must have alignment requirements at
least as strict as those of DBusMessageRealIter * (so that casting
does not increase the required alignment).

Created attachment 121770 [details] [review]
[3/4] DBusMessageIter: eliminate padding on 64-bit platforms

Previously, 64-bit (LP64 or LLP64) platforms would have had 32 bits
of padding between pad2 and pad3. We want to guarantee that an ISO C
compiler will copy the entire struct when assigning between structs,
but padding is not guaranteed to be copied, so we want to ensure that
the struct is "packed".

Statically assert that the old ABI is compatible with the new ABI.

Created attachment 121771 [details] [review]
[4/4] Statically assert that the DBusMessageIter struct has no  padding

An optional follow-up would be to shuffle some struct members around so that there's some spare space at the end of these structs, but I haven't done that yet.

I developed these patches against the 1.10 stable-branch; I'd be inclined to apply them to that branch if other maintainers are OK with that. [3/4] is the only functional change, but I wouldn't be happy with applying it unless we also add the static assertions - if there's a platform where this does break ABI, we want to know about it, and we want compilation to fail there until it's fixed.

Comment on attachment 121770 [details] [review]
[3/4] DBusMessageIter: eliminate padding on 64-bit platforms

Theoretical rather than practical possibly, but don't these need to be == rather than >= ? 

I e, if someone compiles a program against the new headers, and then runs it on a machine with an old libdbus... it would be better if that scenario either succeeded or failed at linker level, rather than getting a SIGSEGV for not copying enough bytes?

(In reply to David Henningsson from comment #11)
> I e, if someone compiles a program against the new headers, and then runs it
> on a machine with an old libdbus... it would be better if that scenario
> either succeeded or failed at linker level, rather than getting a SIGSEGV
> for not copying enough bytes?

Perhaps. We don't normally consider compiling against a new library and then linking at runtime against an old library to be a supported action.

Distros (and Debian and its derivatives in particular) are good at detecting which symbols you use and generating minimal dependencies; that could either be solved by the dbus Debian maintainer (me) increasing the minimal version for dbus_message_iter_*() symbols, or by insisting that it's strictly equal.

Let's go with the "insist strictly equal" approach unless/until someone reports that it isn't true on their architecture, and fall back to ">=" and bumping the minimal version if we have to?

Comment on attachment 121768 [details] [review]
[1/4] dbus-internals: add _DBUS_ALIGNOF

Review of attachment 121768 [details] [review]:
-----------------------------------------------------------------

I thought this was going to trigger a warning, if not an error, with a strict C89 compiler, but I didn't get a peep from Clang, GCC or ICC.

So looks good to me.

Comment on attachment 121769 [details] [review]
[2/4] DBusMessage: assert the properties we need  DBusMessageIter to have

Review of attachment 121769 [details] [review]:
-----------------------------------------------------------------

Ship it.

Comment on attachment 121770 [details] [review]
[3/4] DBusMessageIter: eliminate padding on 64-bit platforms

Review of attachment 121770 [details] [review]:
-----------------------------------------------------------------

The assertions should be == to ensure we haven't broken anything at all. If the size of the new struct is smaller, then user code might copy too many bytes, which in an extreme condition could cause a segfault. If the align of the new struct is smaller, then we may break user structs that contain a DBusMessageIter by eliminating padding it used to have.

The change itself looks good. There are only two cases possible:
 a) sizeof(int) == sizeof(void*) (which implies they have the same alignof): no change in size, no change in alignment
 b) sizeof(int) < sizeof(void*), on all platforms I know of: alignof(int) < alignof(void*): there was a padding hole before pad3 and this change plugs it

This change would break a platform which had 64-bit pointers aligned at 32-bit boundaries. I don't know of any such platform, so this patch should be safe (the only case of alignof(T) < sizeof(T) that I know of is i386 when sizeof(T) > sizeof(void*), which is not the case here).

::: dbus/dbus-message.c
@@ +2053,5 @@
>    _DBUS_STATIC_ASSERT (_DBUS_ALIGNOF (DBusMessageRealIter) <=
>        _DBUS_ALIGNOF (DBusMessageIter));
> +  /* A failure of these two assertions would indicate that we've broken
> +   * ABI on this platform since 1.10.0. */
> +  _DBUS_STATIC_ASSERT (sizeof (DBusMessageIter_1_10_0) >=

Should be == exactly.

@@ +2055,5 @@
> +  /* A failure of these two assertions would indicate that we've broken
> +   * ABI on this platform since 1.10.0. */
> +  _DBUS_STATIC_ASSERT (sizeof (DBusMessageIter_1_10_0) >=
> +      sizeof (DBusMessageIter));
> +  _DBUS_STATIC_ASSERT (_DBUS_ALIGNOF (DBusMessageIter_1_10_0) >=

Ditto.

Comment on attachment 121771 [details] [review]
[4/4] Statically assert that the DBusMessageIter struct has no  padding

Review of attachment 121771 [details] [review]:
-----------------------------------------------------------------

Ship it!

(In reply to Thiago Macieira from comment #15)
> The assertions should be == to ensure we haven't broken anything at all.

Applied with that change; thanks for checking my reasoning on the assertions. Fixed in git for 1.10.8 and 1.11.2, which I'll hopefully release this week.

(In reply to David Henningsson from comment #0)
> So when you copy an DBusMessageIter to another

Another reason why this was only noticed in your language bindings is that in practice, this is rarely (never?) done in C users of libdbus: the conventional use is to put a DBusMessageIter on the stack or in a heap-allocated struct, use it for a while, and lose access to it when the stack pointer moves (if on the stack) or the enclosing struct is freed (if on the heap).