sd-bus: add symbol to tell linker that new vtable functions are used

In 856ad2a86bd9b3e264a090fcf4b0d05bfaa91030 sd_bus_add_object_vtable() and
sd_bus_add_fallback_vtable() were changed to take an updated sd_bus_vtable[]
array with additional 'features' and 'names' fields in the union.

The commit tried to check whether the old or the new table format is used, by
looking at the vtable[0].x.start.element_size field, on the assumption that the
added fields caused the structure size to grow. Unfortunately, this assumption
was false, and on arm32 (at least), the structure size is unchanged.

In libsystemd we use symbol versioning and a major.minor.patch semantic
versioning of the library name (major equals the number in the so-name).  When
systemd-242 was released, the minor number was (correctly) bumped, but this is
not enough, because no new symbols were added or symbol versions changed. This
means that programs compiled with the new systemd headers and library could be
successfully linked to older versions of the library. For example rpm only
looks at the so-name and the list of versioned symbols, completely ignoring the
major.minor numbers in the library name. But the older library does not
understand the new vtable format, and would return -EINVAL after failing the
size check (on those architectures where the structure size did change, i.e.
all 64 bit architectures).

To force new libsystemd (with the functions that take the updated
sd_bus_vtable[] format) to be used, let's pull in a dummy symbol from the table
definition. This is a bit wasteful, because a dummy pointer has to be stored,
but the effect is negligible. In particular, the pointer doesn't even change
the size of the structure because if fits in an unused area in the union.

The number stored in the new unsigned integer is not checked anywhere. If the
symbol exists, we already know we have the new version of the library, so an
additional check would not tell us anything.

An alternative would be to make sd_bus_add_{object,fallback}_vtable() versioned
symbols, using .symver linker annotations. We would provide
sd_bus_add_{object,fallback}_vtable@LIBSYSTEMD_221 (for backwards
compatibility) and e.g. sd_bus_add_{object,fallback}_vtable@@LIBSYSTEMD_242
(the default) with the new implementation. This would work too, but is more
work. We would have to version at least those two functions. And it turns out
that the .symver linker instructions have to located in the same compilation
unit as the function being annotated. We first compile libsystemd.a, and then
link it into libsystemd.so and various other targets, including
libsystemd-shared.so, and the nss modules. If the .symver annotations were
placed next to the function definitions (in bus-object.c), they would influence
all targets that link libsystemd.a, and cause problems, because those functions
should not be exported there. To export them only in libsystemd.so, compilation
would have to be rearranged, so that the functions exported in libsystemd.so
would not be present in libsystemd.a, but a separate compilation unit containg
them and the .symver annotations would be linked solely into libsystemd.so.
This is certainly possible, but more work than the approach in this patch.

856ad2a86bd9b3e264a090fcf4b0d05bfaa91030 has one more issue: it relies on the
undefined fields in sd_bus_vtable[] array to be zeros. But the structure
contains a union, and fields of the union do not have to be zero-initalized by
the compiler. This means that potentially, we could have garbarge values there,
for example when reading the old vtable format definition from the new function
implementation. In practice this should not be an issue at all, because vtable
definitions are static data and are placed in the ro-data section, which is
fully initalized, so we know that those undefined areas will be zero. Things
would be different if somebody defined the vtable array on the heap or on the
stack. Let's just document that they should zero-intialize the unused areas
in this case.

The symbol checking code had to be updated because otherwise gcc warns about a
cast from unsigned to a pointer.

4 files changed, 30 insertions, 6 deletions

diff --git a/src/libsystemd/libsystemd.sym b/src/libsystemd/libsystemd.sym
index a6748ceb20..a9ab0605ce 100644
--- a/src/libsystemd/libsystemd.sym
+++ b/src/libsystemd/libsystemd.sym
@@ -676,3 +676,8 @@ LIBSYSTEMD_241 {
 global:
         sd_bus_close_unref;
 } LIBSYSTEMD_240;
+
+LIBSYSTEMD_243 {
+global:
+        sd_bus_object_vtable_format;
+} LIBSYSTEMD_241;
diff --git a/src/libsystemd/sd-bus/bus-objects.c b/src/libsystemd/sd-bus/bus-objects.c
index d9fc25605a..ce2cb94bde 100644
--- a/src/libsystemd/sd-bus/bus-objects.c
+++ b/src/libsystemd/sd-bus/bus-objects.c
@@ -1701,7 +1701,8 @@ struct sd_bus_vtable_original {
 #define VTABLE_ELEMENT_SIZE sizeof(struct sd_bus_vtable)
 
 static int vtable_features(const sd_bus_vtable *vtable) {
-        if (vtable[0].x.start.element_size == VTABLE_ELEMENT_SIZE_ORIGINAL)
+        if (vtable[0].x.start.element_size < VTABLE_ELEMENT_SIZE ||
+            !vtable[0].x.start.vtable_format_reference)
                 return 0;
         return vtable[0].x.start.features;
 }
@@ -1928,6 +1929,9 @@ fail:
         return r;
 }
 
+/* This symbol exists solely to tell the linker that the "new" vtable format is used. */
+_public_ const unsigned sd_bus_object_vtable_format = 242;
+
 _public_ int sd_bus_add_object_vtable(
                 sd_bus *bus,
                 sd_bus_slot **slot,
diff --git a/src/systemd/sd-bus-vtable.h b/src/systemd/sd-bus-vtable.h
index 8a73ef0503..e3804e203c 100644
--- a/src/systemd/sd-bus-vtable.h
+++ b/src/systemd/sd-bus-vtable.h
@@ -52,6 +52,15 @@ enum {
         _SD_BUS_VTABLE_PARAM_NAMES     = 1 << 0,
 };
 
+extern const unsigned sd_bus_object_vtable_format;
+
+/* Note: unused areas in the sd_bus_vtable[] array must be initalized to 0. The stucture contains an embedded
+ * union, and the compiler is NOT required to initalize the unused areas of the union when the rest of the
+ * structure is initalized. Normally the array is defined as read-only data, in which case the linker places
+ * it in the BSS section, which is always fully initalized, so this is not a concern. But if the array is
+ * created on the stack or on the heap, care must be taken to initalize the unused areas, for examply by
+ * first memsetting the whole region to zero before filling the data in. */
+
 struct sd_bus_vtable {
         /* Please do not initialize this structure directly, use the
          * macros below instead */
@@ -62,6 +71,7 @@ struct sd_bus_vtable {
                 struct {
                         size_t element_size;
                         uint64_t features;
+                        const unsigned *vtable_format_reference;
                 } start;
                 struct {
                         const char *member;
@@ -93,7 +103,8 @@ struct sd_bus_vtable {
                 .x = {                                                  \
                     .start = {                                          \
                         .element_size = sizeof(sd_bus_vtable),          \
-                        .features = _SD_BUS_VTABLE_PARAM_NAMES          \
+                        .features = _SD_BUS_VTABLE_PARAM_NAMES,         \
+                        .vtable_format_reference = &sd_bus_object_vtable_format, \
                     },                                                  \
                 },                                                      \
         }
diff --git a/src/test/generate-sym-test.py b/src/test/generate-sym-test.py
index 357cce8e44..2510809452 100755
--- a/src/test/generate-sym-test.py
+++ b/src/test/generate-sym-test.py
@@ -6,18 +6,22 @@ for header in sys.argv[2:]:
     print('#include "{}"'.format(header.split('/')[-1]))
 
 print('''
-void* functions[] = {''')
+const void* symbols[] = {''')
 
 for line in open(sys.argv[1]):
     match = re.search('^ +([a-zA-Z0-9_]+);', line)
     if match:
-        print('    {},'.format(match.group(1)))
+        s = match.group(1)
+        if s == 'sd_bus_object_vtable_format':
+            print(f'    &{s},')
+        else:
+            print(f'    {s},')
 
 print('''};
 
 int main(void) {
     unsigned i;
-    for (i = 0; i < sizeof(functions)/sizeof(void*); i++)
-         printf("%p\\n", functions[i]);
+    for (i = 0; i < sizeof(symbols)/sizeof(void*); i++)
+         printf("%p\\n", symbols[i]);
     return 0;
 }''')