[systemd-commits] src/libsystemd TODO

[David Herrmann]

 TODO                           |    3 +++
 src/libsystemd/sd-bus/sd-bus.c |   18 ++++++++++++++++++
 2 files changed, 21 insertions(+)

New commits:
commit 374c356979ba7222fa7e09005824fe6996b0e91e
Author: David Herrmann <dh.herrmann at gmail.com>
Date:   Sat Mar 22 19:31:31 2014 +0100

    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 b894e23..03e7233 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 984611f..bbe61a6 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;