sd-bus: re-check match_callbacks_modified in the argNhas value loop

The CAN_HASH branch of bus_match_run() that iterates the array values of
an argNhas= match dispatches a callback per value but, unlike the leaf
and manual-iteration branches, does not re-check bus->match_callbacks_modified
afterwards. A callback that adds or removes a match (freeing the node)
would then have the next hashmap_get(node->compare.children, ...) read
freed memory. Add the same check the sibling branches already have.

Follow-up for 198b158f4941b817f26f8eb0ff75809bf5436496

diff --git a/src/libsystemd/sd-bus/bus-match.c b/src/libsystemd/sd-bus/bus-match.c
index f2521d21b0426e4edb959265beb1d0ee7a833dd5..c365c5f3d844b64a7c75189c5911239fd52403ac 100644 (file)
--- a/src/libsystemd/sd-bus/bus-match.c
+++ b/src/libsystemd/sd-bus/bus-match.c
@@ -391,6 +391,9 @@ int bus_match_run(
                                         r = bus_match_run(bus, found, m);
                                         if (r != 0)
                                                 return r;
+
+                                        if (bus && bus->match_callbacks_modified)
+                                                return 0;
                                 }
                         }
 

diff --git a/src/libsystemd/sd-bus/test-bus-match.c b/src/libsystemd/sd-bus/test-bus-match.c
index 58ba71e21f6fed5f2cc1b6b31c2bc17e888f1494..31d7602c671df7f22ae773a0f5dbba735e1e183a 100644 (file)
--- a/src/libsystemd/sd-bus/test-bus-match.c
+++ b/src/libsystemd/sd-bus/test-bus-match.c
@@ -67,6 +67,63 @@ static void test_match_scope(const char *match, BusMatchScope scope) {
         assert_se(bus_match_get_scope(components, n_components) == scope);
 }
 
+static sd_bus *modify_bus;
+static BusMatchNode *modify_root;
+static sd_bus_slot *modify_slots;
+
+static int strv_uaf_modify_filter(sd_bus_message *m, void *userdata, sd_bus_error *ret_error) {
+        /* Remove both argNhas matches from within the callback (as bus_slot_disconnect() would), which
+         * frees the argNhas node that bus_match_run() is still iterating over the array values of. */
+        assert_se(bus_match_remove(modify_root, &modify_slots[0].match_callback) >= 0);
+        assert_se(bus_match_remove(modify_root, &modify_slots[1].match_callback) >= 0);
+        modify_bus->match_callbacks_modified = true;
+        return 0;
+}
+
+static int strv_uaf_noop_filter(sd_bus_message *m, void *userdata, sd_bus_error *ret_error) {
+        return 0;
+}
+
+static void add_cb_match(BusMatchNode *root, sd_bus_slot *slot, const char *match, sd_bus_message_handler_t cb) {
+        BusMatchComponent *components = NULL;
+        size_t n_components = 0;
+
+        CLEANUP_ARRAY(components, n_components, bus_match_parse_free);
+
+        assert_se(bus_match_parse(match, &components, &n_components) >= 0);
+        slot->n_ref = 1; /* bus_match_run() refs the slot around each callback */
+        slot->match_callback.callback = cb;
+        assert_se(bus_match_add(root, components, n_components, &slot->match_callback) >= 0);
+}
+
+/* Two argNhas= matches under the same node; the first callback frees that node. bus_match_run() must
+ * not dereference it for the next array value. */
+static void test_match_run_strv_modified(sd_bus *bus) {
+        BusMatchNode root = { .type = BUS_MATCH_ROOT };
+        _cleanup_(sd_bus_message_unrefp) sd_bus_message *m = NULL;
+        sd_bus_slot slots[2] = {};
+
+        modify_bus = bus;
+        modify_root = &root;
+        modify_slots = slots;
+
+        add_cb_match(&root, &slots[0], "arg0has='pi'", strv_uaf_modify_filter);
+        add_cb_match(&root, &slots[1], "arg0has='pa'", strv_uaf_noop_filter);
+
+        assert_se(sd_bus_message_new_signal(bus, &m, "/", "a.b", "c") >= 0);
+        assert_se(sd_bus_message_append(m, "as", 2, "pi", "pa") >= 0);
+        assert_se(sd_bus_message_seal(m, 1, 0) >= 0);
+
+        /* Let the leaf gating pass for our synthetic message/matches. */
+        m->read_counter = 1;
+        bus->iteration_counter = 1;
+        bus->match_callbacks_modified = false;
+
+        assert_se(bus_match_run(bus, &root, m) == 0);
+
+        bus_match_free(&root);
+}
+
 int main(int argc, char *argv[]) {
         BusMatchNode root = {
                 .type = BUS_MATCH_ROOT,
@@ -142,5 +199,7 @@ int main(int argc, char *argv[]) {
         test_match_scope("member='gurke',path='/org/freedesktop/DBus/Local'", BUS_MATCH_LOCAL);
         test_match_scope("arg2='piep',sender='org.freedesktop.DBus',member='waldo'", BUS_MATCH_DRIVER);
 
+        test_match_run_strv_modified(bus);
+
         return 0;
 }