From 8f37636d91d801e757a959a9c68996c08cf9f460 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 17 Jan 2019 19:42:59 +0100 Subject: [PATCH 01/10] test: fix indenting off by one --- src/libsystemd/sd-bus/test-bus-address.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libsystemd/sd-bus/test-bus-address.c b/src/libsystemd/sd-bus/test-bus-address.c index db5ff72ef4..c58c52a778 100644 --- a/src/libsystemd/sd-bus/test-bus-address.c +++ b/src/libsystemd/sd-bus/test-bus-address.c @@ -40,8 +40,8 @@ static void test_bus_set_address_system_remote(char **args) { -EINVAL, NULL); test_one_address(b, "user@host", 0, "unixexec:path=ssh,argv1=-xT,argv2=--,argv3=user%40host,argv4=systemd-stdio-bridge"); - test_one_address(b, "user@host@host", - -EINVAL, NULL); + test_one_address(b, "user@host@host", + -EINVAL, NULL); test_one_address(b, "[::1]", 0, "unixexec:path=ssh,argv1=-xT,argv2=--,argv3=%3a%3a1,argv4=systemd-stdio-bridge"); test_one_address(b, "user@[::1]", From 2fe9a10d7695c4c3a748969a0d1662c624e50e5e Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 17 Jan 2019 21:06:30 +0100 Subject: [PATCH 02/10] sd-bus: initialize mutex after we allocated the wqueue That way the mutex doesn't have to be destroyed when we exit early due to OOM. --- src/libsystemd/sd-bus/sd-bus.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libsystemd/sd-bus/sd-bus.c b/src/libsystemd/sd-bus/sd-bus.c index 9eeb3c448c..0df3366d80 100644 --- a/src/libsystemd/sd-bus/sd-bus.c +++ b/src/libsystemd/sd-bus/sd-bus.c @@ -248,12 +248,12 @@ _public_ int sd_bus_new(sd_bus **ret) { .close_on_exit = true, }; - assert_se(pthread_mutex_init(&b->memfd_cache_mutex, NULL) == 0); - /* We guarantee that wqueue always has space for at least one entry */ if (!GREEDY_REALLOC(b->wqueue, b->wqueue_allocated, 1)) return -ENOMEM; + assert_se(pthread_mutex_init(&b->memfd_cache_mutex, NULL) == 0); + *ret = TAKE_PTR(b); return 0; } From 143d4e045a798ccc87889b2a8a60d7fbe44be441 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 17 Jan 2019 18:13:03 +0100 Subject: [PATCH 03/10] sd-bus: make rqueue/wqueue sizes of type size_t Let's do this like we usually do and size arrays with size_t. We already do this for the "allocated" counter correctly, and externally we expose the queue sizes as uint64_t anyway, hence there's really no point in usigned "unsigned" internally. --- src/libsystemd/sd-bus/bus-internal.h | 4 ++-- src/libsystemd/sd-bus/sd-bus.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libsystemd/sd-bus/bus-internal.h b/src/libsystemd/sd-bus/bus-internal.h index a8d61bf72a..04864d4df0 100644 --- a/src/libsystemd/sd-bus/bus-internal.h +++ b/src/libsystemd/sd-bus/bus-internal.h @@ -219,11 +219,11 @@ struct sd_bus { size_t rbuffer_size; sd_bus_message **rqueue; - unsigned rqueue_size; + size_t rqueue_size; size_t rqueue_allocated; sd_bus_message **wqueue; - unsigned wqueue_size; + size_t wqueue_size; size_t windex; size_t wqueue_allocated; diff --git a/src/libsystemd/sd-bus/sd-bus.c b/src/libsystemd/sd-bus/sd-bus.c index 0df3366d80..f000c6250b 100644 --- a/src/libsystemd/sd-bus/sd-bus.c +++ b/src/libsystemd/sd-bus/sd-bus.c @@ -2125,7 +2125,7 @@ _public_ int sd_bus_call( _cleanup_(sd_bus_message_unrefp) sd_bus_message *m = sd_bus_message_ref(_m); usec_t timeout; uint64_t cookie; - unsigned i; + size_t i; int r; bus_assert_return(m, -EINVAL, error); From e593b6a87a335267e5f7238b14683b7f840a01a3 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 17 Jan 2019 18:14:17 +0100 Subject: [PATCH 04/10] sd-bus: reorder bus ref and bus message ref handling Let's always place handling of these references together, so that all reference counting during allocation is at a single place. --- src/libsystemd/sd-bus/bus-message.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/libsystemd/sd-bus/bus-message.c b/src/libsystemd/sd-bus/bus-message.c index bb7e09c945..0d1d14f041 100644 --- a/src/libsystemd/sd-bus/bus-message.c +++ b/src/libsystemd/sd-bus/bus-message.c @@ -459,7 +459,6 @@ int bus_message_from_header( if (!m) return -ENOMEM; - m->n_ref = 1; m->sealed = true; m->header = header; m->header_accessible = header_accessible; @@ -513,7 +512,9 @@ int bus_message_from_header( m->creds.mask |= SD_BUS_CREDS_SELINUX_CONTEXT; } + m->n_ref = 1; m->bus = sd_bus_ref(bus); + *ret = TAKE_PTR(m); return 0; @@ -585,13 +586,13 @@ _public_ int sd_bus_message_new( return -ENOMEM; t->n_ref = 1; + t->bus = sd_bus_ref(bus); t->header = (struct bus_header*) ((uint8_t*) t + ALIGN(sizeof(struct sd_bus_message))); t->header->endian = BUS_NATIVE_ENDIAN; t->header->type = type; t->header->version = bus->message_version; t->allow_fds = bus->can_fds || !IN_SET(bus->state, BUS_HELLO, BUS_RUNNING); t->root_container.need_offsets = BUS_MESSAGE_IS_GVARIANT(t); - t->bus = sd_bus_ref(bus); if (bus->allow_interactive_authorization) t->header->flags |= BUS_MESSAGE_ALLOW_INTERACTIVE_AUTHORIZATION; From c0bc4ec5cc17ac61773d1e9362b0ffa8382c1ff1 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 17 Jan 2019 18:15:37 +0100 Subject: [PATCH 05/10] sd-bus: make sure dispatch_rqueue() initializes return parameter on all types of success Let's make sure our own code follows coding style and initializes all return values on all types of success (and leaves it uninitialized in all types of failure). --- src/libsystemd/sd-bus/sd-bus.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/libsystemd/sd-bus/sd-bus.c b/src/libsystemd/sd-bus/sd-bus.c index f000c6250b..933a5c594e 100644 --- a/src/libsystemd/sd-bus/sd-bus.c +++ b/src/libsystemd/sd-bus/sd-bus.c @@ -1865,8 +1865,10 @@ static int dispatch_rqueue(sd_bus *bus, bool hint_priority, int64_t priority, sd r = bus_read_message(bus, hint_priority, priority); if (r < 0) return r; - if (r == 0) + if (r == 0) { + *m = NULL; return ret; + } ret = 1; } From 39feb2ce417e54cf9746e64b5dfd610cef6ac440 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 17 Jan 2019 18:18:18 +0100 Subject: [PATCH 06/10] sd-bus: drop two inappropriate empty lines --- src/libsystemd/sd-bus/sd-bus.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/libsystemd/sd-bus/sd-bus.c b/src/libsystemd/sd-bus/sd-bus.c index 933a5c594e..c20fb664c5 100644 --- a/src/libsystemd/sd-bus/sd-bus.c +++ b/src/libsystemd/sd-bus/sd-bus.c @@ -2675,7 +2675,6 @@ static int process_builtin(sd_bus *bus, sd_bus_message *m) { SD_BUS_ERROR_UNKNOWN_METHOD, "Unknown method '%s' on interface '%s'.", m->member, m->interface); } - if (r < 0) return r; @@ -2799,7 +2798,6 @@ static int process_running(sd_bus *bus, bool hint_priority, int64_t priority, sd return r; *ret = TAKE_PTR(m); - return 1; } From b41812d1e308de03c879cfca490105216d528c4b Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 17 Jan 2019 21:07:42 +0100 Subject: [PATCH 07/10] sd-bus: always go through sd_bus_unref() to free messages Don't try to be smart, don't bypass the ref counting logic if there's no real reason to. This matters if we want to tweak the ref counting logic later. --- src/libsystemd/sd-bus/bus-message.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/libsystemd/sd-bus/bus-message.c b/src/libsystemd/sd-bus/bus-message.c index 0d1d14f041..0c3b292af8 100644 --- a/src/libsystemd/sd-bus/bus-message.c +++ b/src/libsystemd/sd-bus/bus-message.c @@ -136,8 +136,6 @@ static sd_bus_message* message_free(sd_bus_message *m) { return mfree(m); } -DEFINE_TRIVIAL_CLEANUP_FUNC(sd_bus_message*, message_free); - static void *message_extend_fields(sd_bus_message *m, size_t align, size_t sz, bool add_offset) { void *op, *np; size_t old_size, new_size, start; @@ -529,7 +527,7 @@ int bus_message_from_malloc( const char *label, sd_bus_message **ret) { - _cleanup_(message_freep) sd_bus_message *m = NULL; + _cleanup_(sd_bus_message_unrefp) sd_bus_message *m = NULL; size_t sz; int r; @@ -648,7 +646,7 @@ _public_ int sd_bus_message_new_method_call( const char *interface, const char *member) { - _cleanup_(message_freep) sd_bus_message *t = NULL; + _cleanup_(sd_bus_message_unrefp) sd_bus_message *t = NULL; int r; assert_return(bus, -ENOTCONN); @@ -693,7 +691,7 @@ static int message_new_reply( uint8_t type, sd_bus_message **m) { - _cleanup_(message_freep) sd_bus_message *t = NULL; + _cleanup_(sd_bus_message_unrefp) sd_bus_message *t = NULL; uint64_t cookie; int r; @@ -744,7 +742,7 @@ _public_ int sd_bus_message_new_method_error( sd_bus_message **m, const sd_bus_error *e) { - _cleanup_(message_freep) sd_bus_message *t = NULL; + _cleanup_(sd_bus_message_unrefp) sd_bus_message *t = NULL; int r; assert_return(sd_bus_error_is_set(e), -EINVAL); @@ -847,7 +845,7 @@ int bus_message_new_synthetic_error( const sd_bus_error *e, sd_bus_message **m) { - _cleanup_(message_freep) sd_bus_message *t = NULL; + _cleanup_(sd_bus_message_unrefp) sd_bus_message *t = NULL; int r; assert(bus); From 1b3f9dd759ca0ea215e7b89f8ce66d1b724497b9 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 17 Jan 2019 18:18:54 +0100 Subject: [PATCH 08/10] bus-message: introduce two kinds of references to bus messages Before this commit bus messages had a single reference count: when it reached zero the message would be freed. This simple approach meant a cyclic dependency was typically seen: a message that was enqueued in a bus connection object would reference the bus connection object but also itself be referenced by the bus connection object. So far out strategy to avoid cases like this was: make sure to process the bus connection regularly so that messages don#t stay queued, and at exit flush/close the connection so that the message queued would be emptied, and thus the cyclic dependencies resolved. Im many cases this isn't done properly however. With this change, let's address the issue more systematically: let's break the reference cycle. Specifically, there are now two types of references to a bus message: 1. A regular one, which keeps both the message and the bus object it is associated with pinned. 2. A "queue" reference, which is weaker: it pins the message, but not the bus object it is associated with. The idea is then that regular user handling uses regular references, but when a message is enqueued on its connection, then this takes a "queue" reference instead. This then means that a queued message doesn't imply the connection itself remains pinned, only regular references to the connection or a message associated with it do. Thus, if we end up in the situation where a user allocates a bus and a message and enqueues the latter in the former and drops all refs to both, then this will detect this case and free both. Note that this scheme isn't perfect, it only covers references between messages and the busses they are associated with. If OTOH a bus message is enqueued on a different bus than it is associated with cyclic deps cannot be recognized with this simple algorithm, and thus if you enqueue a message associated with a bus A on a bus B, and another message associated with bus B on a bus A, a cyclic ref will be in effect and not be discovered. However, given that this is an exotic case (though one that happens, consider systemd-bus-stdio-bridge), it should be OK not to cover with this, and people have to explicit flush all queues on exit in that case. Note that this commit only establishes the separate reference counters per message. A follow-up commit will start making use of this from the bus connection object. --- src/libsystemd/sd-bus/bus-message.c | 78 ++++++++++++++++++++++++++++- src/libsystemd/sd-bus/bus-message.h | 14 +++++- 2 files changed, 89 insertions(+), 3 deletions(-) diff --git a/src/libsystemd/sd-bus/bus-message.c b/src/libsystemd/sd-bus/bus-message.c index 0c3b292af8..700c57d362 100644 --- a/src/libsystemd/sd-bus/bus-message.c +++ b/src/libsystemd/sd-bus/bus-message.c @@ -118,7 +118,8 @@ static sd_bus_message* message_free(sd_bus_message *m) { message_reset_parts(m); - sd_bus_unref(m->bus); + /* Note that we don't unref m->bus here. That's already done by sd_bus_message_unref() as each user + * reference to the bus message also is considered a reference to the bus connection itself. */ if (m->free_fds) { close_many(m->fds, m->n_fds); @@ -889,7 +890,80 @@ int bus_message_new_synthetic_error( return 0; } -DEFINE_PUBLIC_TRIVIAL_REF_UNREF_FUNC(sd_bus_message, sd_bus_message, message_free); + +_public_ sd_bus_message* sd_bus_message_ref(sd_bus_message *m) { + if (!m) + return NULL; + + /* We are fine if this message so far was either explicitly reffed or not reffed but queued into at + * least one bus connection object. */ + assert(m->n_ref > 0 || m->n_queued > 0); + + m->n_ref++; + + /* Each user reference to a bus message shall also be considered a ref on the bus */ + sd_bus_ref(m->bus); + return m; +} + +_public_ sd_bus_message* sd_bus_message_unref(sd_bus_message *m) { + if (!m) + return NULL; + + assert(m->n_ref > 0); + + sd_bus_unref(m->bus); /* Each regular ref is also a ref on the bus connection. Let's hence drop it + * here. Note we have to do this before decrementing our own n_ref here, since + * otherwise, if this message is currently queued sd_bus_unref() might call + * bus_message_unref_queued() for this which might then destroy the message + * while we are still processing it. */ + m->n_ref--; + + if (m->n_ref > 0 || m->n_queued > 0) + return NULL; + + /* Unset the bus field if neither the user has a reference nor this message is queued. We are careful + * to reset the field only after the last reference to the bus is dropped, after all we might keep + * multiple references to the bus, once for each reference kept on outselves. */ + m->bus = NULL; + + return message_free(m); +} + +sd_bus_message* bus_message_ref_queued(sd_bus_message *m, sd_bus *bus) { + if (!m) + return NULL; + + /* If this is a different bus than the message is associated with, then implicitly turn this into a + * regular reference. This means that you can create a memory leak by enqueuing a message generated + * on one bus onto another at the same time as enqueueing a message from the second one on the first, + * as we'll not detect the cyclic references there. */ + if (bus != m->bus) + return sd_bus_message_ref(m); + + assert(m->n_ref > 0 || m->n_queued > 0); + m->n_queued++; + + return m; +} + +sd_bus_message* bus_message_unref_queued(sd_bus_message *m, sd_bus *bus) { + if (!m) + return NULL; + + if (bus != m->bus) + return sd_bus_message_unref(m); + + assert(m->n_queued > 0); + m->n_queued--; + + if (m->n_ref > 0 || m->n_queued > 0) + return NULL; + + m->bus = NULL; + + return message_free(m); +} _public_ int sd_bus_message_get_type(sd_bus_message *m, uint8_t *type) { assert_return(m, -EINVAL); diff --git a/src/libsystemd/sd-bus/bus-message.h b/src/libsystemd/sd-bus/bus-message.h index 0115437d26..a7c4f81c4b 100644 --- a/src/libsystemd/sd-bus/bus-message.h +++ b/src/libsystemd/sd-bus/bus-message.h @@ -48,7 +48,16 @@ struct bus_body_part { }; struct sd_bus_message { - unsigned n_ref; + /* Caveat: a message can be referenced in two different ways: the main (user-facing) way will also + * pin the bus connection object the message is associated with. The secondary way ("queued") is used + * when a message is in the read or write queues of the bus connection object, which will not pin the + * bus connection object. This is necessary so that we don't have to have a pair of cyclic references + * between a message that is queued and its connection: as soon as a message is only referenced by + * the connection (by means of being queued) and the connection itself has no other references it + * will be freed. */ + + unsigned n_ref; /* Counter of references that pin the connection */ + unsigned n_queued; /* Counter of references that do not pin the connection */ sd_bus *bus; @@ -211,3 +220,6 @@ int bus_message_remarshal(sd_bus *bus, sd_bus_message **m); void bus_message_set_sender_driver(sd_bus *bus, sd_bus_message *m); void bus_message_set_sender_local(sd_bus *bus, sd_bus_message *m); + +sd_bus_message* bus_message_ref_queued(sd_bus_message *m, sd_bus *bus); +sd_bus_message* bus_message_unref_queued(sd_bus_message *m, sd_bus *bus); From c1757a70eac0382c4837a3833d683919f6a48ed7 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 17 Jan 2019 18:31:59 +0100 Subject: [PATCH 09/10] sd-bus: use "queue" message references for managing r/w message queues in connection objects Let's make use of the new concept the previous commit added. See: #4846 --- src/libsystemd/sd-bus/bus-socket.c | 6 ++- src/libsystemd/sd-bus/sd-bus.c | 60 ++++++++++++++---------------- 2 files changed, 32 insertions(+), 34 deletions(-) diff --git a/src/libsystemd/sd-bus/bus-socket.c b/src/libsystemd/sd-bus/bus-socket.c index 441b4a816f..df9b2631fd 100644 --- a/src/libsystemd/sd-bus/bus-socket.c +++ b/src/libsystemd/sd-bus/bus-socket.c @@ -1110,8 +1110,10 @@ static int bus_socket_make_message(sd_bus *bus, size_t size) { bus->fds = NULL; bus->n_fds = 0; - if (t) - bus->rqueue[bus->rqueue_size++] = t; + if (t) { + bus->rqueue[bus->rqueue_size++] = bus_message_ref_queued(t, bus); + sd_bus_message_unref(t); + } return 1; } diff --git a/src/libsystemd/sd-bus/sd-bus.c b/src/libsystemd/sd-bus/sd-bus.c index c20fb664c5..69f1519976 100644 --- a/src/libsystemd/sd-bus/sd-bus.c +++ b/src/libsystemd/sd-bus/sd-bus.c @@ -146,13 +146,13 @@ static void bus_reset_queues(sd_bus *b) { assert(b); while (b->rqueue_size > 0) - sd_bus_message_unref(b->rqueue[--b->rqueue_size]); + bus_message_unref_queued(b->rqueue[--b->rqueue_size], b); b->rqueue = mfree(b->rqueue); b->rqueue_allocated = 0; while (b->wqueue_size > 0) - sd_bus_message_unref(b->wqueue[--b->wqueue_size]); + bus_message_unref_queued(b->wqueue[--b->wqueue_size], b); b->wqueue = mfree(b->wqueue); b->wqueue_allocated = 0; @@ -493,7 +493,7 @@ static int synthesize_connected_signal(sd_bus *bus) { /* Insert at the very front */ memmove(bus->rqueue + 1, bus->rqueue, sizeof(sd_bus_message*) * bus->rqueue_size); - bus->rqueue[0] = TAKE_PTR(m); + bus->rqueue[0] = bus_message_ref_queued(m, bus); bus->rqueue_size++; return 0; @@ -1811,7 +1811,7 @@ static int dispatch_wqueue(sd_bus *bus) { * anyway. */ bus->wqueue_size--; - sd_bus_message_unref(bus->wqueue[0]); + bus_message_unref_queued(bus->wqueue[0], bus); memmove(bus->wqueue, bus->wqueue + 1, sizeof(sd_bus_message*) * bus->wqueue_size); bus->windex = 0; @@ -1840,6 +1840,15 @@ int bus_rqueue_make_room(sd_bus *bus) { return 0; } +static void rqueue_drop_one(sd_bus *bus, size_t i) { + assert(bus); + assert(i < bus->rqueue_size); + + bus_message_unref_queued(bus->rqueue[i], bus); + memmove(bus->rqueue + i, bus->rqueue + i + 1, sizeof(sd_bus_message*) * (bus->rqueue_size - i - 1)); + bus->rqueue_size--; +} + static int dispatch_rqueue(sd_bus *bus, bool hint_priority, int64_t priority, sd_bus_message **m) { int r, ret = 0; @@ -1854,10 +1863,8 @@ static int dispatch_rqueue(sd_bus *bus, bool hint_priority, int64_t priority, sd for (;;) { if (bus->rqueue_size > 0) { /* Dispatch a queued message */ - - *m = bus->rqueue[0]; - bus->rqueue_size--; - memmove(bus->rqueue, bus->rqueue + 1, sizeof(sd_bus_message*) * bus->rqueue_size); + *m = sd_bus_message_ref(bus->rqueue[0]); + rqueue_drop_one(bus, 0); return 1; } @@ -1935,7 +1942,7 @@ _public_ int sd_bus_send(sd_bus *bus, sd_bus_message *_m, uint64_t *cookie) { * of the wqueue array is always allocated so * that we always can remember how much was * written. */ - bus->wqueue[0] = sd_bus_message_ref(m); + bus->wqueue[0] = bus_message_ref_queued(m, bus); bus->wqueue_size = 1; bus->windex = idx; } @@ -1949,7 +1956,7 @@ _public_ int sd_bus_send(sd_bus *bus, sd_bus_message *_m, uint64_t *cookie) { if (!GREEDY_REALLOC(bus->wqueue, bus->wqueue_allocated, bus->wqueue_size + 1)) return -ENOMEM; - bus->wqueue[bus->wqueue_size++] = sd_bus_message_ref(m); + bus->wqueue[bus->wqueue_size++] = bus_message_ref_queued(m, bus); } finish: @@ -2169,37 +2176,30 @@ _public_ int sd_bus_call( usec_t left; while (i < bus->rqueue_size) { - sd_bus_message *incoming = NULL; + _cleanup_(sd_bus_message_unrefp) sd_bus_message *incoming = NULL; - incoming = bus->rqueue[i]; + incoming = sd_bus_message_ref(bus->rqueue[i]); if (incoming->reply_cookie == cookie) { /* Found a match! */ - memmove(bus->rqueue + i, bus->rqueue + i + 1, sizeof(sd_bus_message*) * (bus->rqueue_size - i - 1)); - bus->rqueue_size--; + rqueue_drop_one(bus, i); log_debug_bus_message(incoming); if (incoming->header->type == SD_BUS_MESSAGE_METHOD_RETURN) { if (incoming->n_fds <= 0 || bus->accept_fd) { if (reply) - *reply = incoming; - else - sd_bus_message_unref(incoming); + *reply = TAKE_PTR(incoming); return 1; } - r = sd_bus_error_setf(error, SD_BUS_ERROR_INCONSISTENT_MESSAGE, "Reply message contained file descriptors which I couldn't accept. Sorry."); - sd_bus_message_unref(incoming); - return r; + return sd_bus_error_setf(error, SD_BUS_ERROR_INCONSISTENT_MESSAGE, "Reply message contained file descriptors which I couldn't accept. Sorry."); - } else if (incoming->header->type == SD_BUS_MESSAGE_METHOD_ERROR) { - r = sd_bus_error_copy(error, &incoming->error); - sd_bus_message_unref(incoming); - return r; - } else { + } else if (incoming->header->type == SD_BUS_MESSAGE_METHOD_ERROR) + return sd_bus_error_copy(error, &incoming->error); + else { r = -EIO; goto fail; } @@ -2209,15 +2209,11 @@ _public_ int sd_bus_call( incoming->sender && streq(bus->unique_name, incoming->sender)) { - memmove(bus->rqueue + i, bus->rqueue + i + 1, sizeof(sd_bus_message*) * (bus->rqueue_size - i - 1)); - bus->rqueue_size--; + rqueue_drop_one(bus, i); - /* Our own message? Somebody is trying - * to send its own client a message, - * let's not dead-lock, let's fail - * immediately. */ + /* Our own message? Somebody is trying to send its own client a message, + * let's not dead-lock, let's fail immediately. */ - sd_bus_message_unref(incoming); r = -ELOOP; goto fail; } From 4b70aedc4a595feaad0c43b8f10dc6782ef0546c Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 17 Jan 2019 19:45:12 +0100 Subject: [PATCH 10/10] test: add test for new sd-bus refcnt logic --- .../sd-bus/test-bus-queue-ref-cycle.c | 44 +++++++++++++++++++ src/test/meson.build | 4 ++ 2 files changed, 48 insertions(+) create mode 100644 src/libsystemd/sd-bus/test-bus-queue-ref-cycle.c diff --git a/src/libsystemd/sd-bus/test-bus-queue-ref-cycle.c b/src/libsystemd/sd-bus/test-bus-queue-ref-cycle.c new file mode 100644 index 0000000000..70901c30b0 --- /dev/null +++ b/src/libsystemd/sd-bus/test-bus-queue-ref-cycle.c @@ -0,0 +1,44 @@ +/* SPDX-License-Identifier: LGPL-2.1+ */ + +#include "main-func.h" +#include "sd-bus.h" +#include "tests.h" + +static int run(int argc, char *argv[]) { + sd_bus_message *m = NULL; + sd_bus *bus = NULL; + int r; + + /* This test will result in a memory leak in <= v240, but not on v241. Hence to be really useful it + * should be run through a leak tracker such as valgrind. */ + + r = sd_bus_open_system(&bus); + if (r < 0) + return log_tests_skipped("Failed to connect to bus"); + + /* Create a message and enqueue it (this shouldn't send it though as the connection setup is not complete yet) */ + assert_se(sd_bus_message_new_method_call(bus, &m, "foo.bar", "/foo", "quux.quux", "waldo") >= 0); + assert_se(sd_bus_send(bus, m, NULL) >= 0); + + /* Let's now unref the message first and the bus second. */ + m = sd_bus_message_unref(m); + bus = sd_bus_unref(bus); + + /* We should have a memory leak now on <= v240. Let's do this again, but destory in the opposite + * order. On v240 that too should be a leak. */ + + r = sd_bus_open_system(&bus); + if (r < 0) + return log_tests_skipped("Failed to connect to bus"); + + assert_se(sd_bus_message_new_method_call(bus, &m, "foo.bar", "/foo", "quux.quux", "waldo") >= 0); + assert_se(sd_bus_send(bus, m, NULL) >= 0); + + /* Let's now unref things in the opposite order */ + bus = sd_bus_unref(bus); + m = sd_bus_message_unref(m); + + return 0; +} + +DEFINE_MAIN_FUNCTION(run); diff --git a/src/test/meson.build b/src/test/meson.build index 86d8a30afa..c53b9653f9 100644 --- a/src/test/meson.build +++ b/src/test/meson.build @@ -867,6 +867,10 @@ tests += [ [], [threads]], + [['src/libsystemd/sd-bus/test-bus-queue-ref-cycle.c'], + [], + [threads]], + [['src/libsystemd/sd-bus/test-bus-watch-bind.c'], [], [threads], '', 'timeout=120'],