From 374c356979ba7222fa7e09005824fe6996b0e91e Mon Sep 17 00:00:00 2001 From: David Herrmann Date: Sat, 22 Mar 2014 19:31:31 +0100 Subject: [PATCH] sd-bus: mark sd_bus_unref() as broken regarding self-refs If you allocate a message with bus==NULL and then unref the main bus, it will free your message underneath and your program will go boom! To fix that, we really need to figure out what the semantics for self-references (m->bus) should be and when/where/what accesses are actually allowed. Same is true for the pseudo-thread-safety we employ.. --- TODO | 3 +++ src/libsystemd/sd-bus/sd-bus.c | 18 ++++++++++++++++++ 2 files changed, 21 insertions(+) diff --git a/TODO b/TODO index b894e235e..03e72336c 100644 --- a/TODO +++ b/TODO @@ -16,6 +16,9 @@ Bugfixes: * systemctl --root=container/ set-default ... is totally borked. +* sd_bus_unref() is broken regarding self-references and "pseudo thread-safety". + See the comment in sd_bus_unref() for more.. + External: * Fedora: when installing fedora with yum --installroot /var/run is a directory, not a symlink diff --git a/src/libsystemd/sd-bus/sd-bus.c b/src/libsystemd/sd-bus/sd-bus.c index 984611fd4..bbe61a6a8 100644 --- a/src/libsystemd/sd-bus/sd-bus.c +++ b/src/libsystemd/sd-bus/sd-bus.c @@ -1394,6 +1394,24 @@ _public_ sd_bus *sd_bus_unref(sd_bus *bus) { if (!bus) return NULL; + /* TODO/FIXME: It's naive to think REFCNT_GET() is thread-safe in any + * way but exclusive REFCNT_DEC(). The current logic _must_ lock around + * REFCNT_GET() until REFCNT_DEC() or two threads might end up in + * parallel in bus_reset_queues(). But locking would totally break the + * recursion we introduce by bus_reset_queues()... + * (Imagine one thread in sd_bus_message_unref() setting n_ref to 0 and + * thus calling into sd_bus_unref(). If at the same time the real + * thread calls sd_bus_unref(), both end up with "q == true" and will + * call into bus_reset_queues(). + * If we require the main bus to be alive until all dispatch threads + * are done, there is no need to do ref-counts at all. So in both ways, + * the REFCNT thing is humbug.) + * + * On a second note: messages are *not* required to have ->bus set nor + * does it have to be _this_ bus that they're assigned to. This whole + * ref-cnt checking breaks apart if a message is not assigned to us. + * (which is _very_ easy to trigger with the current API). */ + if (REFCNT_GET(bus->n_ref) == bus->rqueue_size + bus->wqueue_size + 1) { bool q = true; -- 2.30.2