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..

diff --git a/TODO b/TODO
index b894e235ed89f381d16b36d0114ac9aaf944041c..03e72336c9fdc3577b012ceb2586e5ddc43fa8a5 100644 (file)
--- a/TODO
+++ b/TODO
@@ -16,6 +16,9 @@ Bugfixes:
 
 * systemctl --root=container/ set-default ... is totally borked.
 
 
 * 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
 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 984611fd4d9c92a959ca59015eb4a443806d5b4d..bbe61a6a8f8ae2c8870b0059aeffcae47c0fe407 100644 (file)
--- 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;
 
         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;
 
         if (REFCNT_GET(bus->n_ref) == bus->rqueue_size + bus->wqueue_size + 1) {
                 bool q = true;