diff --git a/src/basic/hashmap.c b/src/basic/hashmap.c index a2868e2455..89517b99f7 100644 --- a/src/basic/hashmap.c +++ b/src/basic/hashmap.c @@ -1330,6 +1330,9 @@ int set_ensure_consume(Set **s, const struct hash_ops *hash_ops, void *key) { if (r <= 0) { if (hash_ops && hash_ops->free_key) hash_ops->free_key(key); + else if (hash_ops && hash_ops->free_value) + /* Sets store their element in the key slot but may carry a value destructor. */ + hash_ops->free_value(key); else free(key); } diff --git a/src/libsystemd/sd-bus/bus-match.c b/src/libsystemd/sd-bus/bus-match.c index f2521d21b0..c365c5f3d8 100644 --- 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/bus-objects.c b/src/libsystemd/sd-bus/bus-objects.c index 795cc68837..aa371cceca 100644 --- 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-match.c b/src/libsystemd/sd-bus/test-bus-match.c index 58ba71e21f..31d7602c67 100644 --- 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; } diff --git a/src/libsystemd/sd-bus/test-bus-objects.c b/src/libsystemd/sd-bus/test-bus-objects.c index ac33086a6f..5d8e772cb4 100644 --- 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")); diff --git a/src/libsystemd/sd-device/device-monitor-private.h b/src/libsystemd/sd-device/device-monitor-private.h index 4582108df6..dc1af6c19c 100644 --- a/src/libsystemd/sd-device/device-monitor-private.h +++ b/src/libsystemd/sd-device/device-monitor-private.h @@ -15,3 +15,24 @@ int device_monitor_new_full(sd_device_monitor **ret, MonitorNetlinkGroup group, int device_monitor_get_address(sd_device_monitor *m, union sockaddr_union *ret); int device_monitor_allow_unicast_sender(sd_device_monitor *m, sd_device_monitor *sender); int device_monitor_send(sd_device_monitor *m, const union sockaddr_union *destination, sd_device *device); + +#define UDEV_MONITOR_MAGIC 0xfeedcafe + +typedef struct monitor_netlink_header { + /* "libudev" prefix to distinguish libudev and kernel messages */ + char prefix[8]; + /* Magic to protect against daemon <-> Library message format mismatch + * Used in the kernel from socket filter rules; needs to be stored in network order */ + unsigned magic; + /* Total length of header structure known to the sender */ + unsigned header_size; + /* Properties string buffer */ + unsigned properties_off; + unsigned properties_len; + /* Hashes of primary device properties strings, to let libudev subscribers + * use in-kernel socket filters; values need to be stored in network order */ + unsigned filter_subsystem_hash; + unsigned filter_devtype_hash; + unsigned filter_tag_bloom_hi; + unsigned filter_tag_bloom_lo; +} monitor_netlink_header; diff --git a/src/libsystemd/sd-device/device-monitor.c b/src/libsystemd/sd-device/device-monitor.c index 962329a865..9799d9b4da 100644 --- a/src/libsystemd/sd-device/device-monitor.c +++ b/src/libsystemd/sd-device/device-monitor.c @@ -67,27 +67,6 @@ struct sd_device_monitor { void *userdata; }; -#define UDEV_MONITOR_MAGIC 0xfeedcafe - -typedef struct monitor_netlink_header { - /* "libudev" prefix to distinguish libudev and kernel messages */ - char prefix[8]; - /* Magic to protect against daemon <-> Library message format mismatch - * Used in the kernel from socket filter rules; needs to be stored in network order */ - unsigned magic; - /* Total length of header structure known to the sender */ - unsigned header_size; - /* Properties string buffer */ - unsigned properties_off; - unsigned properties_len; - /* Hashes of primary device properties strings, to let libudev subscribers - * use in-kernel socket filters; values need to be stored in network order */ - unsigned filter_subsystem_hash; - unsigned filter_devtype_hash; - unsigned filter_tag_bloom_hi; - unsigned filter_tag_bloom_lo; -} monitor_netlink_header; - static int monitor_set_nl_address(sd_device_monitor *m) { union sockaddr_union snl; socklen_t addrlen; @@ -642,10 +621,10 @@ _public_ int sd_device_monitor_receive(sd_device_monitor *m, sd_device **ret) { "Invalid message signature (%x != %x).", message.nlh->magic, htobe32(UDEV_MONITOR_MAGIC)); - if (message.nlh->properties_off + 32 > (size_t) n) + if (message.nlh->properties_off > LESS_BY((size_t) n, 32u)) return log_monitor_errno(m, SYNTHETIC_ERRNO(EAGAIN), - "Invalid offset for properties (%u > %zi).", - message.nlh->properties_off + 32, n); + "Invalid properties offset (%u) for message of length %zi.", + message.nlh->properties_off, n); offset = message.nlh->properties_off; @@ -781,34 +760,57 @@ int device_monitor_send( return count; } -static void bpf_stmt(struct sock_filter *ins, unsigned *i, - unsigned short code, unsigned data) { +static int bpf_stmt_impl(struct sock_filter *ins, size_t *i, size_t n_ins, + unsigned short code, unsigned data) { + assert(ins); assert(i); + if (*i >= n_ins) + return -E2BIG; + ins[(*i)++] = (struct sock_filter) { .code = code, .k = data, }; + return 0; } -static void bpf_jmp(struct sock_filter *ins, unsigned *i, - unsigned short code, unsigned data, - unsigned short jt, unsigned short jf) { +#define bpf_stmt(ins, i, code, data) \ + bpf_stmt_impl((ins), (i), ELEMENTSOF(ins), (code), (data)) + +static int bpf_jmp_impl(struct sock_filter *ins, size_t *i, size_t n_ins, + unsigned short code, unsigned data, + unsigned jt, unsigned jf) { + assert(ins); assert(i); + if (*i >= n_ins) + return -E2BIG; + + /* The jump offsets are stored in single bytes (struct sock_filter.jt/.jf are __u8). A larger + * offset would be silently truncated and make the filter branch to the wrong instruction, i.e. + * drop events that should match. */ + if (jt > UINT8_MAX || jf > UINT8_MAX) + return -E2BIG; + ins[(*i)++] = (struct sock_filter) { .code = code, .jt = jt, .jf = jf, .k = data, }; + return 0; } +#define bpf_jmp(ins, i, code, data, jt, jf) \ + bpf_jmp_impl((ins), (i), ELEMENTSOF(ins), (code), (data), (jt), (jf)) + _public_ int sd_device_monitor_filter_update(sd_device_monitor *m) { struct sock_filter ins[512] = {}; struct sock_fprog filter; const char *subsystem, *devtype, *tag; - unsigned i = 0; + size_t i = 0; + int r; assert_return(m, -EINVAL); @@ -823,11 +825,11 @@ _public_ int sd_device_monitor_filter_update(sd_device_monitor *m) { } /* load magic in A */ - bpf_stmt(ins, &i, BPF_LD|BPF_W|BPF_ABS, offsetof(monitor_netlink_header, magic)); + r = bpf_stmt(ins, &i, BPF_LD|BPF_W|BPF_ABS, offsetof(monitor_netlink_header, magic)); /* jump if magic matches */ - bpf_jmp(ins, &i, BPF_JMP|BPF_JEQ|BPF_K, UDEV_MONITOR_MAGIC, 1, 0); + RET_GATHER(r, bpf_jmp(ins, &i, BPF_JMP|BPF_JEQ|BPF_K, UDEV_MONITOR_MAGIC, 1, 0)); /* wrong magic, pass packet */ - bpf_stmt(ins, &i, BPF_RET|BPF_K, 0xffffffff); + RET_GATHER(r, bpf_stmt(ins, &i, BPF_RET|BPF_K, 0xffffffff)); if (!set_isempty(m->tag_filter)) { int tag_matches = set_size(m->tag_filter); @@ -839,23 +841,23 @@ _public_ int sd_device_monitor_filter_update(sd_device_monitor *m) { uint32_t tag_bloom_lo = tag_bloom_bits & 0xffffffff; /* load device bloom bits in A */ - bpf_stmt(ins, &i, BPF_LD|BPF_W|BPF_ABS, offsetof(monitor_netlink_header, filter_tag_bloom_hi)); + RET_GATHER(r, bpf_stmt(ins, &i, BPF_LD|BPF_W|BPF_ABS, offsetof(monitor_netlink_header, filter_tag_bloom_hi))); /* clear bits (tag bits & bloom bits) */ - bpf_stmt(ins, &i, BPF_ALU|BPF_AND|BPF_K, tag_bloom_hi); + RET_GATHER(r, bpf_stmt(ins, &i, BPF_ALU|BPF_AND|BPF_K, tag_bloom_hi)); /* jump to next tag if it does not match */ - bpf_jmp(ins, &i, BPF_JMP|BPF_JEQ|BPF_K, tag_bloom_hi, 0, 3); + RET_GATHER(r, bpf_jmp(ins, &i, BPF_JMP|BPF_JEQ|BPF_K, tag_bloom_hi, 0, 3)); /* load device bloom bits in A */ - bpf_stmt(ins, &i, BPF_LD|BPF_W|BPF_ABS, offsetof(monitor_netlink_header, filter_tag_bloom_lo)); + RET_GATHER(r, bpf_stmt(ins, &i, BPF_LD|BPF_W|BPF_ABS, offsetof(monitor_netlink_header, filter_tag_bloom_lo))); /* clear bits (tag bits & bloom bits) */ - bpf_stmt(ins, &i, BPF_ALU|BPF_AND|BPF_K, tag_bloom_lo); + RET_GATHER(r, bpf_stmt(ins, &i, BPF_ALU|BPF_AND|BPF_K, tag_bloom_lo)); /* jump behind end of tag match block if tag matches */ tag_matches--; - bpf_jmp(ins, &i, BPF_JMP|BPF_JEQ|BPF_K, tag_bloom_lo, 1 + (tag_matches * 6), 0); + RET_GATHER(r, bpf_jmp(ins, &i, BPF_JMP|BPF_JEQ|BPF_K, tag_bloom_lo, 1 + (tag_matches * 6), 0)); } /* nothing matched, drop packet */ - bpf_stmt(ins, &i, BPF_RET|BPF_K, 0); + RET_GATHER(r, bpf_stmt(ins, &i, BPF_RET|BPF_K, 0)); } /* add all subsystem matches */ @@ -864,33 +866,33 @@ _public_ int sd_device_monitor_filter_update(sd_device_monitor *m) { uint32_t hash = string_hash32(subsystem); /* load device subsystem value in A */ - bpf_stmt(ins, &i, BPF_LD|BPF_W|BPF_ABS, offsetof(monitor_netlink_header, filter_subsystem_hash)); + RET_GATHER(r, bpf_stmt(ins, &i, BPF_LD|BPF_W|BPF_ABS, offsetof(monitor_netlink_header, filter_subsystem_hash))); if (!devtype) { /* jump if subsystem does not match */ - bpf_jmp(ins, &i, BPF_JMP|BPF_JEQ|BPF_K, hash, 0, 1); + RET_GATHER(r, bpf_jmp(ins, &i, BPF_JMP|BPF_JEQ|BPF_K, hash, 0, 1)); } else { /* jump if subsystem does not match */ - bpf_jmp(ins, &i, BPF_JMP|BPF_JEQ|BPF_K, hash, 0, 3); + RET_GATHER(r, bpf_jmp(ins, &i, BPF_JMP|BPF_JEQ|BPF_K, hash, 0, 3)); /* load device devtype value in A */ - bpf_stmt(ins, &i, BPF_LD|BPF_W|BPF_ABS, offsetof(monitor_netlink_header, filter_devtype_hash)); + RET_GATHER(r, bpf_stmt(ins, &i, BPF_LD|BPF_W|BPF_ABS, offsetof(monitor_netlink_header, filter_devtype_hash))); /* jump if value does not match */ hash = string_hash32(devtype); - bpf_jmp(ins, &i, BPF_JMP|BPF_JEQ|BPF_K, hash, 0, 1); + RET_GATHER(r, bpf_jmp(ins, &i, BPF_JMP|BPF_JEQ|BPF_K, hash, 0, 1)); } /* matched, pass packet */ - bpf_stmt(ins, &i, BPF_RET|BPF_K, 0xffffffff); - - if (i+1 >= ELEMENTSOF(ins)) - return -E2BIG; + RET_GATHER(r, bpf_stmt(ins, &i, BPF_RET|BPF_K, 0xffffffff)); } /* nothing matched, drop packet */ - bpf_stmt(ins, &i, BPF_RET|BPF_K, 0); + RET_GATHER(r, bpf_stmt(ins, &i, BPF_RET|BPF_K, 0)); } /* matched, pass packet */ - bpf_stmt(ins, &i, BPF_RET|BPF_K, 0xffffffff); + RET_GATHER(r, bpf_stmt(ins, &i, BPF_RET|BPF_K, 0xffffffff)); + + if (r < 0) + return r; /* install filter */ filter = (struct sock_fprog) { diff --git a/src/libsystemd/sd-device/test-sd-device-monitor.c b/src/libsystemd/sd-device/test-sd-device-monitor.c index 8d88f3eb67..c4034d27c7 100644 --- a/src/libsystemd/sd-device/test-sd-device-monitor.c +++ b/src/libsystemd/sd-device/test-sd-device-monitor.c @@ -9,10 +9,12 @@ #include "device-private.h" #include "device-util.h" #include "io-util.h" +#include "iovec-util.h" #include "mountpoint-util.h" #include "path-util.h" #include "socket-util.h" #include "stat-util.h" +#include "stdio-util.h" #include "string-util.h" #include "tests.h" #include "time-util.h" @@ -282,6 +284,21 @@ TEST(sd_device_monitor_filter_add_match_tag) { ASSERT_EQ(sd_event_loop(sd_device_monitor_get_event(monitor_client)), 100); } +TEST(sd_device_monitor_filter_update_bounds) { + _cleanup_(sd_device_monitor_unrefp) sd_device_monitor *m = NULL; + + ASSERT_OK(device_monitor_new_full(&m, MONITOR_GROUP_NONE, -EBADF)); + + /* Each tag emits 6 BPF instructions into a fixed 512-entry array, too many must be refused */ + for (unsigned u = 0; u < 200; u++) { + char t[32]; + xsprintf(t, "tag%u", u); + ASSERT_OK(sd_device_monitor_filter_add_match_tag(m, t)); + } + + ASSERT_ERROR(sd_device_monitor_filter_update(m), E2BIG); +} + TEST(sd_device_monitor_filter_add_match_sysattr) { _cleanup_(sd_device_monitor_unrefp) sd_device_monitor *monitor_server = NULL, *monitor_client = NULL; _cleanup_(sd_device_enumerator_unrefp) sd_device_enumerator *e = NULL; @@ -408,6 +425,31 @@ TEST(sd_device_monitor_receive) { ASSERT_STREQ(s, syspath); } +TEST(sd_device_monitor_receive_bad_properties_off) { + _cleanup_(sd_device_monitor_unrefp) sd_device_monitor *monitor_server = NULL, *monitor_client = NULL; + union sockaddr_union sa; + + prepare_monitor(&monitor_server, &monitor_client, &sa); + + monitor_netlink_header nlh = { + .prefix = "libudev", + .magic = htobe32(UDEV_MONITOR_MAGIC), + .header_size = sizeof nlh, + .properties_off = UINT32_MAX - 10, + }; + struct iovec iov = IOVEC_MAKE(&nlh, sizeof nlh); + struct msghdr smsg = { + .msg_iov = &iov, + .msg_iovlen = 1, + .msg_name = &sa, + .msg_namelen = sizeof(struct sockaddr_nl), + }; + ASSERT_OK_ERRNO(sendmsg(sd_device_monitor_get_fd(monitor_server), &smsg, 0)); + + _cleanup_(sd_device_unrefp) sd_device *dev = NULL; + ASSERT_ERROR(sd_device_monitor_receive(monitor_client, &dev), EAGAIN); +} + static int intro(void) { if (getuid() != 0) return log_tests_skipped("not root"); diff --git a/src/libsystemd/sd-netlink/sd-netlink.c b/src/libsystemd/sd-netlink/sd-netlink.c index 65d731a20b..8c2790e94b 100644 --- a/src/libsystemd/sd-netlink/sd-netlink.c +++ b/src/libsystemd/sd-netlink/sd-netlink.c @@ -548,7 +548,7 @@ int sd_netlink_call_async( uint64_t usec, const char *description) { - _cleanup_free_ sd_netlink_slot *slot = NULL; + _cleanup_(sd_netlink_slot_unrefp) sd_netlink_slot *slot = NULL; int r, k; assert_return(nl, -EINVAL); @@ -859,34 +859,39 @@ int netlink_add_match_internal( void *userdata, const char *description) { - _cleanup_free_ sd_netlink_slot *slot = NULL; + _cleanup_(sd_netlink_slot_unrefp) sd_netlink_slot *slot = NULL; int r; assert(groups); assert(n_groups > 0); - for (size_t i = 0; i < n_groups; i++) { - r = socket_broadcast_group_ref(nl, groups[i]); - if (r < 0) - return r; - } - r = netlink_slot_allocate(nl, !ret_slot, NETLINK_MATCH_CALLBACK, sizeof(struct match_callback), userdata, description, &slot); if (r < 0) return r; - slot->match_callback.groups = newdup(uint32_t, groups, n_groups); - if (!slot->match_callback.groups) - return -ENOMEM; - - slot->match_callback.n_groups = n_groups; slot->match_callback.callback = callback; slot->match_callback.type = type; slot->match_callback.cmd = cmd; + slot->match_callback.groups = newdup(uint32_t, groups, n_groups); + if (!slot->match_callback.groups) + return -ENOMEM; + + /* Link the callback before taking the broadcast group references, so that on a partial failure the + * cleanup path (sd_netlink_slot_unref() -> netlink_slot_disconnect()) can unlink it again and drop + * the references we already took. n_groups is bumped as each reference is acquired, so the disconnect + * only releases what we actually got. */ LIST_PREPEND(match_callbacks, nl->match_callbacks, &slot->match_callback); + for (size_t i = 0; i < n_groups; i++) { + r = socket_broadcast_group_ref(nl, groups[i]); + if (r < 0) + return r; + + slot->match_callback.n_groups = i + 1; + } + /* Set this at last. Otherwise, some failures in above call the destroy callback but some do not. */ slot->destroy_callback = destroy_callback; diff --git a/src/network/networkd-sriov.c b/src/network/networkd-sriov.c index 4664425979..d35e108a4a 100644 --- a/src/network/networkd-sriov.c +++ b/src/network/networkd-sriov.c @@ -98,11 +98,12 @@ int link_request_sr_iov_vfs(Link *link) { if (!sr_iov_has_config(sr_iov, attr)) continue; + /* The request takes a reference on the configuration. */ r = link_queue_request_safe( link, _REQUEST_TYPE_SRIOV_BASE + attr, sr_iov, - NULL, + sr_iov_unref, sr_iov_hash_func, sr_iov_compare_func, sr_iov_process_request, @@ -113,6 +114,8 @@ int link_request_sr_iov_vfs(Link *link) { return log_link_warning_errno(link, r, "Failed to request to set up %s for SR-IOV virtual function %"PRIu32": %m", sr_iov_attribute_to_string(attr), sr_iov->vf); + if (r > 0) + sr_iov_ref(sr_iov); } } diff --git a/src/network/networkd-state-file.c b/src/network/networkd-state-file.c index 3c505f39e6..f077ed5f4b 100644 --- a/src/network/networkd-state-file.c +++ b/src/network/networkd-state-file.c @@ -134,7 +134,7 @@ static int link_put_dns(Link *link, OrderedSet **s) { r = sd_dhcp_lease_get_dnr(link->dhcp_lease, &resolvers); if (r >= 0) { - struct in_addr_full **dot_servers; + struct in_addr_full **dot_servers = NULL; size_t n = 0; CLEANUP_ARRAY(dot_servers, n, in_addr_full_free_array); @@ -163,7 +163,7 @@ static int link_put_dns(Link *link, OrderedSet **s) { r = sd_dhcp6_lease_get_dnr(link->dhcp6_lease, &resolvers); if (r >= 0) { - struct in_addr_full **dot_servers; + struct in_addr_full **dot_servers = NULL; size_t n = 0; CLEANUP_ARRAY(dot_servers, n, in_addr_full_free_array); diff --git a/src/resolve/resolved-dns-browse-services.c b/src/resolve/resolved-dns-browse-services.c index 68dcbee349..6a48089a59 100644 --- a/src/resolve/resolved-dns-browse-services.c +++ b/src/resolve/resolved-dns-browse-services.c @@ -126,7 +126,7 @@ static int mdns_maintenance_query(sd_event_source *s, uint64_t usec, void *userd return log_error_errno(r, "Failed to create mDNS query for maintenance: %m"); q->complete = mdns_maintenance_query_complete; - q->varlink_request = sd_varlink_ref(service->service_browser->link); + q->service_browser_request = dns_service_browser_ref(service->service_browser); q->dnsservice_request = dnssd_discovered_service_ref(service); /* Schedule the next maintenance query based on the TTL */ diff --git a/src/shared/netif-sriov.c b/src/shared/netif-sriov.c index f5df7fb392..79fd86c4f6 100644 --- a/src/shared/netif-sriov.c +++ b/src/shared/netif-sriov.c @@ -16,24 +16,42 @@ #include "string-table.h" #include "string-util.h" +static SRIOV* sr_iov_detach_impl(SRIOV *sr_iov) { + assert(sr_iov); + + if (!sr_iov->sr_iov_by_section) + return NULL; + + assert(sr_iov->section); + ordered_hashmap_remove(sr_iov->sr_iov_by_section, sr_iov->section); + sr_iov->sr_iov_by_section = NULL; + + return sr_iov; +} + static SRIOV* sr_iov_free(SRIOV *sr_iov) { if (!sr_iov) return NULL; - if (sr_iov->sr_iov_by_section && sr_iov->section) - ordered_hashmap_remove(sr_iov->sr_iov_by_section, sr_iov->section); + sr_iov_detach_impl(sr_iov); config_section_free(sr_iov->section); return mfree(sr_iov); } -DEFINE_SECTION_CLEANUP_FUNCTIONS(SRIOV, sr_iov_free); +DEFINE_TRIVIAL_REF_UNREF_FUNC(SRIOV, sr_iov, sr_iov_free); +DEFINE_SECTION_CLEANUP_FUNCTIONS(SRIOV, sr_iov_unref); + +static void sr_iov_detach(SRIOV *sr_iov) { + assert(sr_iov); + sr_iov_unref(sr_iov_detach_impl(sr_iov)); +} DEFINE_PRIVATE_HASH_OPS_WITH_VALUE_DESTRUCTOR( sr_iov_hash_ops_by_section, ConfigSection, config_section_hash_func, config_section_compare_func, - SRIOV, sr_iov_free); + SRIOV, sr_iov_detach); static int sr_iov_new(SRIOV **ret) { SRIOV *sr_iov; @@ -45,6 +63,7 @@ static int sr_iov_new(SRIOV **ret) { return -ENOMEM; *sr_iov = (SRIOV) { + .n_ref = 1, .vf = UINT32_MAX, .vlan_proto = ETH_P_8021Q, .vf_spoof_check_setting = -1, @@ -60,7 +79,7 @@ static int sr_iov_new(SRIOV **ret) { static int sr_iov_new_static(OrderedHashmap **sr_iov_by_section, const char *filename, unsigned section_line, SRIOV **ret) { _cleanup_(config_section_freep) ConfigSection *n = NULL; - _cleanup_(sr_iov_freep) SRIOV *sr_iov = NULL; + _cleanup_(sr_iov_unrefp) SRIOV *sr_iov = NULL; SRIOV *existing = NULL; int r; @@ -368,7 +387,7 @@ int sr_iov_drop_invalid_sections(uint32_t num_vfs, OrderedHashmap *sr_iov_by_sec SRIOV *dup; if (sr_iov_section_verify(num_vfs, sr_iov) < 0) { - sr_iov_free(sr_iov); + sr_iov_detach(sr_iov); continue; } @@ -378,7 +397,7 @@ int sr_iov_drop_invalid_sections(uint32_t num_vfs, OrderedHashmap *sr_iov_by_sec "dropping the [SR-IOV] section specified at line %u.", dup->section->filename, sr_iov->section->line, dup->section->line, dup->section->line); - sr_iov_free(dup); + sr_iov_detach(dup); } r = set_ensure_put(&set, &sr_iov_hash_ops, sr_iov); @@ -402,7 +421,7 @@ int config_parse_sr_iov_uint32( void *data, void *userdata) { - _cleanup_(sr_iov_free_or_set_invalidp) SRIOV *sr_iov = NULL; + _cleanup_(sr_iov_unref_or_set_invalidp) SRIOV *sr_iov = NULL; OrderedHashmap **sr_iov_by_section = ASSERT_PTR(data); uint32_t k; int r; @@ -469,7 +488,7 @@ int config_parse_sr_iov_vlan_proto( void *data, void *userdata) { - _cleanup_(sr_iov_free_or_set_invalidp) SRIOV *sr_iov = NULL; + _cleanup_(sr_iov_unref_or_set_invalidp) SRIOV *sr_iov = NULL; OrderedHashmap **sr_iov_by_section = ASSERT_PTR(data); int r; @@ -507,7 +526,7 @@ int config_parse_sr_iov_link_state( void *data, void *userdata) { - _cleanup_(sr_iov_free_or_set_invalidp) SRIOV *sr_iov = NULL; + _cleanup_(sr_iov_unref_or_set_invalidp) SRIOV *sr_iov = NULL; OrderedHashmap **sr_iov_by_section = ASSERT_PTR(data); int r; @@ -558,7 +577,7 @@ int config_parse_sr_iov_boolean( void *data, void *userdata) { - _cleanup_(sr_iov_free_or_set_invalidp) SRIOV *sr_iov = NULL; + _cleanup_(sr_iov_unref_or_set_invalidp) SRIOV *sr_iov = NULL; OrderedHashmap **sr_iov_by_section = ASSERT_PTR(data); int r; @@ -615,7 +634,7 @@ int config_parse_sr_iov_mac( void *data, void *userdata) { - _cleanup_(sr_iov_free_or_set_invalidp) SRIOV *sr_iov = NULL; + _cleanup_(sr_iov_unref_or_set_invalidp) SRIOV *sr_iov = NULL; OrderedHashmap **sr_iov_by_section = ASSERT_PTR(data); int r; diff --git a/src/shared/netif-sriov.h b/src/shared/netif-sriov.h index cd2befc634..578a7ea579 100644 --- a/src/shared/netif-sriov.h +++ b/src/shared/netif-sriov.h @@ -27,6 +27,8 @@ typedef enum SRIOVLinkState { } SRIOVLinkState; typedef struct SRIOV { + unsigned n_ref; + ConfigSection *section; OrderedHashmap *sr_iov_by_section; @@ -43,6 +45,8 @@ typedef struct SRIOV { DECLARE_STRING_TABLE_LOOKUP_TO_STRING(sr_iov_attribute, SRIOVAttribute); +DECLARE_TRIVIAL_REF_UNREF_FUNC(SRIOV, sr_iov); + void sr_iov_hash_func(const SRIOV *sr_iov, struct siphash *state); int sr_iov_compare_func(const SRIOV *s1, const SRIOV *s2); bool sr_iov_has_config(SRIOV *sr_iov, SRIOVAttribute attr); diff --git a/src/test/test-set.c b/src/test/test-set.c index 4c1872d636..652b6837be 100644 --- a/src/test/test-set.c +++ b/src/test/test-set.c @@ -2,8 +2,10 @@ #include +#include "alloc-util.h" #include "random-util.h" #include "set.h" +#include "siphash24.h" #include "strv.h" #include "tests.h" @@ -51,6 +53,46 @@ TEST(set_free_with_hash_ops) { assert_se(items[3].seen == 0); } +static unsigned set_ensure_consume_freed = 0; + +typedef struct ValItem { + int key; +} ValItem; + +static void val_item_free(ValItem *i) { + set_ensure_consume_freed++; + free(i); +} + +static void val_item_hash_func(const ValItem *i, struct siphash *state) { + siphash24_compress_typesafe(i->key, state); +} + +static int val_item_compare_func(const ValItem *a, const ValItem *b) { + return CMP(a->key, b->key); +} + +DEFINE_PRIVATE_HASH_OPS_WITH_VALUE_DESTRUCTOR( + val_item_hash_ops, ValItem, val_item_hash_func, val_item_compare_func, + ValItem, val_item_free); + +TEST(set_ensure_consume_value_destructor) { + _cleanup_set_free_ Set *s = NULL; + ValItem *a, *b; + + ASSERT_NOT_NULL(a = new0(ValItem, 1)); + a->key = 1; + ASSERT_OK_POSITIVE(set_ensure_consume(&s, &val_item_hash_ops, a)); + + ASSERT_NOT_NULL(b = new0(ValItem, 1)); + b->key = 1; /* same key, different pointer -> duplicate */ + + set_ensure_consume_freed = 0; + /* The rejected duplicate must be released via the value destructor, not plain free(). */ + ASSERT_OK_ZERO(set_ensure_consume(&s, &val_item_hash_ops, b)); + ASSERT_EQ(set_ensure_consume_freed, 1u); +} + TEST(set_put) { _cleanup_set_free_ Set *m = NULL; diff --git a/src/udev/test-udev-format.c b/src/udev/test-udev-format.c index 1acd8db72b..b71e1e182a 100644 --- a/src/udev/test-udev-format.c +++ b/src/udev/test-udev-format.c @@ -1,9 +1,13 @@ /* SPDX-License-Identifier: LGPL-2.1-or-later */ +#include "sd-device.h" + +#include "device-private.h" #include "log.h" #include "mountpoint-util.h" #include "string-util.h" #include "tests.h" +#include "udev-event.h" #include "udev-format.h" static void test_udev_resolve_subsys_kernel_one(const char *str, bool read_value, int retval, const char *expected) { @@ -36,6 +40,26 @@ TEST(udev_resolve_subsys_kernel) { test_udev_resolve_subsys_kernel_one("[net/lo]/address", true, 0, "00:00:00:00:00:00"); } +TEST(udev_event_apply_format_links) { + _cleanup_(sd_device_unrefp) sd_device *dev = NULL; + _cleanup_(udev_event_unrefp) UdevEvent *event = NULL; + char dest[64]; + bool truncated = false; + + ASSERT_OK(sd_device_new_from_syspath(&dev, "/sys/class/net/lo")); + + for (unsigned u = 0; u < 32; u++) { + _cleanup_free_ char *l = NULL; + ASSERT_OK(asprintf(&l, "/dev/link-that-is-long-%u", u)); + ASSERT_OK(device_add_devlink(dev, l)); + } + + ASSERT_NOT_NULL((event = udev_event_new(dev, NULL, EVENT_TEST_SPAWN))); + + udev_event_apply_format(event, "$links", dest, sizeof dest, false, &truncated); + ASSERT_TRUE(truncated); +} + static int intro(void) { if (path_is_mount_point("/sys") <= 0) return log_tests_skipped("/sys is not mounted"); diff --git a/src/udev/test-udev-manager.c b/src/udev/test-udev-manager.c index d444b0bf00..7de5a37289 100644 --- a/src/udev/test-udev-manager.c +++ b/src/udev/test-udev-manager.c @@ -1,5 +1,6 @@ /* SPDX-License-Identifier: LGPL-2.1-or-later */ +#include "alloc-util.h" #include "tests.h" #include "udev-manager.h" @@ -16,4 +17,22 @@ TEST(devpath_conflict) { "/devices/pci0000:00/0000:00:1c.4/0000:3c:00.0/nvme/nvme0/nvme0n1/nvme0n1p1")); } +TEST(worker_free_detaches_event) { + /* worker_free() must clear the back-pointer of the event it is processing, otherwise the event is + * left referencing the freed worker. This happens e.g. on manager_free(), which frees the workers + * before the events still referencing them. */ + + Worker *worker = ASSERT_PTR(new0(Worker, 1)); + worker->pidref = PIDREF_NULL; + + _cleanup_free_ Event *event = ASSERT_PTR(new0(Event, 1)); + + worker->event = event; + event->worker = worker; + + worker_free(worker); + + ASSERT_NULL(event->worker); +} + DEFINE_TEST_MAIN(LOG_DEBUG); diff --git a/src/udev/udev-format.c b/src/udev/udev-format.c index d670ee129f..c37ea0406f 100644 --- a/src/udev/udev-format.c +++ b/src/udev/udev-format.c @@ -334,9 +334,9 @@ static ssize_t udev_event_subst_format( case FORMAT_SUBST_LINKS: FOREACH_DEVICE_DEVLINK(dev, link) { if (s == dest) - strpcpy_full(&s, l, link + STRLEN("/dev/"), &truncated); + l = strpcpy_full(&s, l, link + STRLEN("/dev/"), &truncated); else - strpcpyl_full(&s, l, &truncated, " ", link + STRLEN("/dev/"), NULL); + l = strpcpyl_full(&s, l, &truncated, " ", link + STRLEN("/dev/"), NULL); if (truncated) break; } diff --git a/src/udev/udev-manager.c b/src/udev/udev-manager.c index df13df6b50..fabbdbb88c 100644 --- a/src/udev/udev-manager.c +++ b/src/udev/udev-manager.c @@ -50,63 +50,6 @@ #define EVENT_REQUEUE_INTERVAL_USEC (200 * USEC_PER_MSEC) #define EVENT_REQUEUE_TIMEOUT_USEC (3 * USEC_PER_MINUTE) -typedef enum EventState { - EVENT_UNDEF, - EVENT_QUEUED, - EVENT_RUNNING, - EVENT_LOCKED, - EVENT_PROCESSED, -} EventState; - -typedef struct Event { - /* All events that have not been processed (state != EVENT_PROCESSED) are referenced by the Manager. - * Additionally, an event may be referenced by events blocked by this event. See event_find_blocker(). */ - unsigned n_ref; - - Manager *manager; - Worker *worker; - EventState state; - - sd_device *dev; - - sd_device_action_t action; - uint64_t seqnum; - const char *id; - const char *devpath; - const char *devpath_old; - const char *devnode; - - /* Used when the device is locked by another program. */ - usec_t requeue_next_usec; - usec_t requeue_timeout_usec; - unsigned locked_event_prioq_index; - char *whole_disk; - LIST_FIELDS(Event, same_disk); - - /* The last blocker for this event. This event must not be processed before the blocker is processed. */ - Event *blocker; - - LIST_FIELDS(Event, event); -} Event; - -typedef enum WorkerState { - WORKER_UNDEF, - WORKER_RUNNING, - WORKER_IDLE, - WORKER_KILLED, -} WorkerState; - -typedef struct Worker { - Manager *manager; - PidRef pidref; - sd_event_source *child_event_source; - sd_event_source *timeout_warning_event_source; - sd_event_source *timeout_kill_event_source; - union sockaddr_union address; - WorkerState state; - Event *event; -} Worker; - static void event_unset_whole_disk(Event *event) { Manager *manager = ASSERT_PTR(ASSERT_PTR(event)->manager); @@ -171,13 +114,18 @@ static Event* event_enter_processed(Event *event) { DEFINE_TRIVIAL_CLEANUP_FUNC(Event*, event_enter_processed); -static Worker* worker_free(Worker *worker) { +Worker* worker_free(Worker *worker) { if (!worker) return NULL; if (worker->manager) hashmap_remove(worker->manager->workers, &worker->pidref); + /* If an event is still attached, clear its back-pointer so it doesn't dangle (mirrors event_free(), + * which clears the worker's pointer). */ + if (worker->event) + worker->event->worker = NULL; + sd_event_source_disable_unref(worker->child_event_source); sd_event_source_unref(worker->timeout_warning_event_source); sd_event_source_unref(worker->timeout_kill_event_source); diff --git a/src/udev/udev-manager.h b/src/udev/udev-manager.h index c1b98c52d1..4e18dd5404 100644 --- a/src/udev/udev-manager.h +++ b/src/udev/udev-manager.h @@ -1,9 +1,12 @@ /* SPDX-License-Identifier: GPL-2.0-or-later */ #pragma once +#include "sd-device.h" #include "sd-event.h" #include "list.h" +#include "pidref.h" +#include "socket-util.h" #include "udev-config.h" #include "udev-forward.h" @@ -76,10 +79,69 @@ typedef struct Manager { bool exit; } Manager; +typedef enum EventState { + EVENT_UNDEF, + EVENT_QUEUED, + EVENT_RUNNING, + EVENT_LOCKED, + EVENT_PROCESSED, +} EventState; + +typedef struct Event { + /* All events that have not been processed (state != EVENT_PROCESSED) are referenced by the Manager. + * Additionally, an event may be referenced by events blocked by this event. See event_find_blocker(). */ + unsigned n_ref; + + Manager *manager; + Worker *worker; + EventState state; + + sd_device *dev; + + sd_device_action_t action; + uint64_t seqnum; + const char *id; + const char *devpath; + const char *devpath_old; + const char *devnode; + + /* Used when the device is locked by another program. */ + usec_t requeue_next_usec; + usec_t requeue_timeout_usec; + unsigned locked_event_prioq_index; + char *whole_disk; + LIST_FIELDS(Event, same_disk); + + /* The last blocker for this event. This event must not be processed before the blocker is processed. */ + Event *blocker; + + LIST_FIELDS(Event, event); +} Event; + +typedef enum WorkerState { + WORKER_UNDEF, + WORKER_RUNNING, + WORKER_IDLE, + WORKER_KILLED, +} WorkerState; + +typedef struct Worker { + Manager *manager; + PidRef pidref; + sd_event_source *child_event_source; + sd_event_source *timeout_warning_event_source; + sd_event_source *timeout_kill_event_source; + union sockaddr_union address; + WorkerState state; + Event *event; +} Worker; + Manager* manager_new(void); Manager* manager_free(Manager *manager); DEFINE_TRIVIAL_CLEANUP_FUNC(Manager*, manager_free); +Worker* worker_free(Worker *worker); + int manager_main(Manager *manager); void manager_reload(Manager *manager, bool force); void manager_revert(Manager *manager);