[systemd-commits] 2 commits - src/libsystemd
David Herrmann
dvdhrm at kemper.freedesktop.org
Sat Mar 22 10:10:28 PDT 2014
src/libsystemd/sd-bus/sd-bus.c | 11 ++++-
src/libsystemd/sd-rtnl/sd-rtnl.c | 81 +++++++++++++++++++++++++++++++--------
2 files changed, 76 insertions(+), 16 deletions(-)
New commits:
commit eb33a6f858c2cd39dcf9c2f39514c9f83ed040fe
Author: David Herrmann <dh.herrmann at gmail.com>
Date: Sat Mar 22 18:06:38 2014 +0100
sd-bus: add note about sd_bus_unref() recursion
In sd_bus_unref() we check for self-reference loops and destruct our
queues in case we're the only reference holders. However, we do _not_
modify our own ref-count, thus effectively causing the
message-destructions to enter with the same reference count as we did.
The only reason this doesn't cause an endless recursion (or trigger
assert(m->n_ref > 0) in sd_bus_message_unref()) is the fact that we
decrease queue-counters _before_ calling _unref(). That's not obvious at
all, so add a big fat note in bus_reset_queues() to everyone touching that
code.
diff --git a/src/libsystemd/sd-bus/sd-bus.c b/src/libsystemd/sd-bus/sd-bus.c
index 15c7677..984611f 100644
--- a/src/libsystemd/sd-bus/sd-bus.c
+++ b/src/libsystemd/sd-bus/sd-bus.c
@@ -111,6 +111,12 @@ static void bus_node_destroy(sd_bus *b, struct node *n) {
static void bus_reset_queues(sd_bus *b) {
assert(b);
+ /* NOTE: We _must_ decrement b->Xqueue_size before calling
+ * sd_bus_message_unref() for _each_ message. Otherwise the
+ * self-reference checks in sd_bus_unref() will fire for each message.
+ * We would thus recurse into sd_bus_message_unref() and trigger the
+ * assert(m->n_ref > 0) */
+
while (b->rqueue_size > 0)
sd_bus_message_unref(b->rqueue[--b->rqueue_size]);
@@ -1408,7 +1414,10 @@ _public_ sd_bus *sd_bus_unref(sd_bus *bus) {
/* We are the only holders on the messages, and the
* messages are the only holders on us, so let's drop
* the messages and thus implicitly also kill our own
- * last references */
+ * last references.
+ * bus_reset_queues() decrements the queue-size before
+ * calling into sd_bus_message_unref(). Thus, it
+ * protects us from recursion. */
if (q)
bus_reset_queues(bus);
commit 22fdeadcc06e95fe41ac4de872ec245c0887547f
Author: David Herrmann <dh.herrmann at gmail.com>
Date: Sat Mar 22 17:43:30 2014 +0100
sd-rtnl: fix self-reference leaks
Like sd-bus, sd-rtnl can have self-references through queued messages. In
particular, each queued message has the following self-ref loop:
rtnl->wqueue[i]->rtnl == rtnl
Same is true for "rqueue".
When sd_rtnl_unref() gets called, we must therefore make sure we correctly
consider each self-reference when deciding to destroy the object. For each
queued message, there _might_ be one ref. However, rtnl-messages can be
created _without_ a bus-reference, therefore we need to verify the
actually required ref-count.
Once we know exactly how many self-refs exist, and we verified none of the
queued messages has external references, we can destruct the object.
We must immediately drop our own reference, then flush all queues and
destroy the bus object. Otherwise, each sd_rtnl_message_unref() call would
recurse into the same destruction logic as they enter with the same
rtnl-refcnt.
Note: We really should verify _all_ queued messages have m->rtnl set to
the bus they're queued on. If that's given, we can change:
if (REFCNT_GET(rtnl->n_ref) <= refs)
to
if (REFCNT_GET(rtnl->n_ref) == refs)
and thus avoid recalculating the required refs for each message we
remove from the queue during destruction.
diff --git a/src/libsystemd/sd-rtnl/sd-rtnl.c b/src/libsystemd/sd-rtnl/sd-rtnl.c
index e5610b4..6042657 100644
--- a/src/libsystemd/sd-rtnl/sd-rtnl.c
+++ b/src/libsystemd/sd-rtnl/sd-rtnl.c
@@ -111,31 +111,82 @@ sd_rtnl *sd_rtnl_ref(sd_rtnl *rtnl) {
}
sd_rtnl *sd_rtnl_unref(sd_rtnl *rtnl) {
+ unsigned long refs;
- if (rtnl && REFCNT_DEC(rtnl->n_ref) <= 0) {
+ if (!rtnl)
+ return NULL;
+
+ /*
+ * If our ref-cnt is exactly the number of internally queued messages
+ * plus the ref-cnt to be dropped, then we know there's no external
+ * reference to us. Hence, we look through all queued messages and if
+ * they also have no external references, we're about to drop the last
+ * ref. Flush the queues so the REFCNT_DEC() below will drop to 0.
+ * We must be careful not to introduce inter-message references or this
+ * logic will fall apart..
+ */
+
+ refs = rtnl->rqueue_size + rtnl->wqueue_size + 1;
+
+ if (REFCNT_GET(rtnl->n_ref) <= refs) {
struct match_callback *f;
+ bool q = true;
unsigned i;
- for (i = 0; i < rtnl->rqueue_size; i++)
- sd_rtnl_message_unref(rtnl->rqueue[i]);
- free(rtnl->rqueue);
+ for (i = 0; i < rtnl->rqueue_size; i++) {
+ if (REFCNT_GET(rtnl->rqueue[i]->n_ref) > 1) {
+ q = false;
+ break;
+ } else if (rtnl->rqueue[i]->rtnl != rtnl)
+ --refs;
+ }
- for (i = 0; i < rtnl->wqueue_size; i++)
- sd_rtnl_message_unref(rtnl->wqueue[i]);
- free(rtnl->wqueue);
+ if (q) {
+ for (i = 0; i < rtnl->wqueue_size; i++) {
+ if (REFCNT_GET(rtnl->wqueue[i]->n_ref) > 1) {
+ q = false;
+ break;
+ } else if (rtnl->wqueue[i]->rtnl != rtnl)
+ --refs;
+ }
+ }
- hashmap_free_free(rtnl->reply_callbacks);
- prioq_free(rtnl->reply_callbacks_prioq);
+ if (q && REFCNT_GET(rtnl->n_ref) == refs) {
+ /* Drop our own ref early to avoid recursion from:
+ * sd_rtnl_message_unref()
+ * sd_rtnl_unref()
+ * These must enter sd_rtnl_unref() with a ref-cnt
+ * smaller than us. */
+ REFCNT_DEC(rtnl->n_ref);
- while ((f = rtnl->match_callbacks)) {
- LIST_REMOVE(match_callbacks, rtnl->match_callbacks, f);
- free(f);
- }
+ for (i = 0; i < rtnl->rqueue_size; i++)
+ sd_rtnl_message_unref(rtnl->rqueue[i]);
+ free(rtnl->rqueue);
- safe_close(rtnl->fd);
- free(rtnl);
+ for (i = 0; i < rtnl->wqueue_size; i++)
+ sd_rtnl_message_unref(rtnl->wqueue[i]);
+ free(rtnl->wqueue);
+
+ assert_se(REFCNT_GET(rtnl->n_ref) == 0);
+
+ hashmap_free_free(rtnl->reply_callbacks);
+ prioq_free(rtnl->reply_callbacks_prioq);
+
+ while ((f = rtnl->match_callbacks)) {
+ LIST_REMOVE(match_callbacks, rtnl->match_callbacks, f);
+ free(f);
+ }
+
+ safe_close(rtnl->fd);
+ free(rtnl);
+
+ return NULL;
+ }
}
+ assert_se(REFCNT_GET(rtnl->n_ref) > 0);
+ REFCNT_DEC(rtnl->n_ref);
+
return NULL;
}
More information about the systemd-commits
mailing list