sd-bus: drop half-registered vtable members on failure

add_object_vtable_internal() inserts BusVTableMember entries into the
global vtable_methods/vtable_properties sets as it walks the vtable, but
only sets node_vtable.node (which gates the disconnect-time cleanup)
after the loop. A failure mid-loop thus left the already-inserted
members behind, dangling once the slot's interface string and the node's
path were freed, so a later registration reading those keys hit a
use-after-free. Remove this slot's members on the fail path.

The test asserts the failed registration leaves no member behind.

Follow-up for 19befb2d5fc087f96e40ddc432b2cc9385666209

diff --git a/src/libsystemd/sd-bus/bus-objects.c b/src/libsystemd/sd-bus/bus-objects.c
index 795cc688376557adfa45862c0a7991e12afb4e8a..aa371cceca8fed558554de425a5b7afabbd09d5b 100644 (file)
--- a/src/libsystemd/sd-bus/bus-objects.c
+++ b/src/libsystemd/sd-bus/bus-objects.c
@@ -2099,6 +2099,37 @@ static int add_object_vtable_internal(
         return 0;
 
 fail:
+        /* Remove members we already inserted into the global tables before failing (identified by
+         * parent), so they don't dangle once the slot and node are freed below. */
+        if (s && s->node_vtable.interface)
+                for (v = bus_vtable_next(vtable, vtable); v->type != _SD_BUS_VTABLE_END; v = bus_vtable_next(vtable, v)) {
+                        BusVTableMember key, *x;
+                        Set *set;
+
+                        if (v->type == _SD_BUS_VTABLE_METHOD) {
+                                set = bus->vtable_methods;
+                                key.member = v->x.method.member;
+                        } else if (IN_SET(v->type, _SD_BUS_VTABLE_PROPERTY, _SD_BUS_VTABLE_WRITABLE_PROPERTY)) {
+                                set = bus->vtable_properties;
+                                key.member = v->x.property.member;
+                        } else
+                                continue;
+
+                        /* The entry that failed validation may carry a NULL member, which was never
+                         * inserted and would crash the hashmap lookup below. Skip it. */
+                        if (isempty(key.member))
+                                continue;
+
+                        key.path = n->path;
+                        key.interface = s->node_vtable.interface;
+
+                        x = set_get(set, &key);
+                        if (x && x->parent == &s->node_vtable) {
+                                assert_se(set_remove(set, &key) == x);
+                                free(x);
+                        }
+                }
+
         sd_bus_slot_unref(s);
         bus_node_gc(bus, n);
 

diff --git a/src/libsystemd/sd-bus/test-bus-objects.c b/src/libsystemd/sd-bus/test-bus-objects.c
index ac33086a6f374d340ccf72aa55ea2044c09a3842..5d8e772cb441eb92ad15ed481bd573035212ead0 100644 (file)
--- a/src/libsystemd/sd-bus/test-bus-objects.c
+++ b/src/libsystemd/sd-bus/test-bus-objects.c
@@ -7,6 +7,7 @@
 #include "bus-internal.h"
 #include "bus-message.h"
 #include "log.h"
+#include "set.h"
 #include "strv.h"
 #include "tests.h"
 
@@ -566,6 +567,25 @@ static int client(void *p) {
         return 0;
 }
 
+static const sd_bus_vtable partial_vtable[] = {
+        SD_BUS_VTABLE_START(0),
+        SD_BUS_METHOD("Foo", NULL, NULL, NULL, 0),
+        SD_BUS_METHOD("has space", NULL, NULL, NULL, 0), /* invalid member name, registration fails here */
+        SD_BUS_VTABLE_END
+};
+
+static void test_vtable_partial_failure(void) {
+        _cleanup_(sd_bus_flush_close_unrefp) sd_bus *bus = NULL;
+
+        ASSERT_OK(sd_bus_new(&bus));
+
+        /* Registration fails at the invalid member, after "Foo" was already inserted into the global vtable
+         * member tables. Check that the fail path removes it again. */
+        ASSERT_ERROR(sd_bus_add_object_vtable(bus, NULL, "/x", "org.test.iface", partial_vtable, NULL), EINVAL);
+
+        ASSERT_TRUE(set_isempty(bus->vtable_methods));
+}
+
 int main(int argc, char *argv[]) {
         _cleanup_(sd_event_unrefp) sd_event *e = NULL;
         _cleanup_(sd_future_unrefp) sd_future *f_server = NULL, *f_client = NULL;
@@ -573,6 +593,8 @@ int main(int argc, char *argv[]) {
 
         test_setup_logging(LOG_DEBUG);
 
+        test_vtable_partial_failure();
+
         c.automatic_integer_property = 4711;
         ASSERT_NOT_NULL(c.automatic_string_property = strdup("dudeldu"));