daandemeyer / rpms / systemd

Forked from rpms/systemd 2 years ago
Clone
5d2ee9
From bc2d7df4fc21e9e54413169d5aad21616314d65e Mon Sep 17 00:00:00 2001
5d2ee9
From: Lennart Poettering <lennart@poettering.net>
5d2ee9
Date: Thu, 17 Jan 2019 18:18:54 +0100
5d2ee9
Subject: [PATCH] bus-message: introduce two kinds of references to bus
5d2ee9
 messages
5d2ee9
5d2ee9
Before this commit bus messages had a single reference count: when it
5d2ee9
reached zero the message would be freed. This simple approach meant a
5d2ee9
cyclic dependency was typically seen: a message that was enqueued in a
5d2ee9
bus connection object would reference the bus connection object but also
5d2ee9
itself be referenced by the bus connection object. So far out strategy
5d2ee9
to avoid cases like this was: make sure to process the bus connection
5d2ee9
regularly so that messages don#t stay queued, and at exit flush/close
5d2ee9
the connection so that the message queued would be emptied, and thus the
5d2ee9
cyclic dependencies resolved. Im many cases this isn't done properly
5d2ee9
however.
5d2ee9
5d2ee9
With this change, let's address the issue more systematically: let's
5d2ee9
break the reference cycle. Specifically, there are now two types of
5d2ee9
references to a bus message:
5d2ee9
5d2ee9
1. A regular one, which keeps both the message and the bus object it is
5d2ee9
   associated with pinned.
5d2ee9
5d2ee9
2. A "queue" reference, which is weaker: it pins the message, but not
5d2ee9
   the bus object it is associated with.
5d2ee9
5d2ee9
The idea is then that regular user handling uses regular references, but
5d2ee9
when a message is enqueued on its connection, then this takes a "queue"
5d2ee9
reference instead. This then means that a queued message doesn't imply
5d2ee9
the connection itself remains pinned, only regular references to the
5d2ee9
connection or a message associated with it do. Thus, if we end up in the
5d2ee9
situation where a user allocates a bus and a message and enqueues the
5d2ee9
latter in the former and drops all refs to both, then this will detect
5d2ee9
this case and free both.
5d2ee9
5d2ee9
Note that this scheme isn't perfect, it only covers references between
5d2ee9
messages and the busses they are associated with. If OTOH a bus message
5d2ee9
is enqueued on a different bus than it is associated with cyclic deps
5d2ee9
cannot be recognized with this simple algorithm, and thus if you enqueue
5d2ee9
a message associated with a bus A on a bus B, and another message
5d2ee9
associated with bus B on a bus A, a cyclic ref will be in effect and not
5d2ee9
be discovered. However, given that this is an exotic case (though one
5d2ee9
that happens, consider systemd-bus-stdio-bridge), it should be OK not to
5d2ee9
cover with this, and people have to explicit flush all queues on exit in
5d2ee9
that case.
5d2ee9
5d2ee9
Note that this commit only establishes the separate reference counters
5d2ee9
per message. A follow-up commit will start making use of this from the
5d2ee9
bus connection object.
5d2ee9
5d2ee9
(cherry picked from commit 1b3f9dd759ca0ea215e7b89f8ce66d1b724497b9)
5d2ee9
Related: CVE-2020-1712
5d2ee9
---
5d2ee9
 src/libsystemd/sd-bus/bus-message.c | 60 ++++++++++++++++++++++++++---
5d2ee9
 src/libsystemd/sd-bus/bus-message.h | 14 ++++++-
5d2ee9
 2 files changed, 68 insertions(+), 6 deletions(-)
5d2ee9
5d2ee9
diff --git a/src/libsystemd/sd-bus/bus-message.c b/src/libsystemd/sd-bus/bus-message.c
5d2ee9
index 306b6d6816..7fe8929f82 100644
5d2ee9
--- a/src/libsystemd/sd-bus/bus-message.c
5d2ee9
+++ b/src/libsystemd/sd-bus/bus-message.c
5d2ee9
@@ -120,7 +120,8 @@ static sd_bus_message* message_free(sd_bus_message *m) {
5d2ee9
 
5d2ee9
         message_reset_parts(m);
5d2ee9
 
5d2ee9
-        sd_bus_unref(m->bus);
5d2ee9
+        /* Note that we don't unref m->bus here. That's already done by sd_bus_message_unref() as each user
5d2ee9
+         * reference to the bus message also is considered a reference to the bus connection itself. */
5d2ee9
 
5d2ee9
         if (m->free_fds) {
5d2ee9
                 close_many(m->fds, m->n_fds);
5d2ee9
@@ -893,27 +894,76 @@ int bus_message_new_synthetic_error(
5d2ee9
 }
5d2ee9
 
5d2ee9
 _public_ sd_bus_message* sd_bus_message_ref(sd_bus_message *m) {
5d2ee9
-
5d2ee9
         if (!m)
5d2ee9
                 return NULL;
5d2ee9
 
5d2ee9
-        assert(m->n_ref > 0);
5d2ee9
+        /* We are fine if this message so far was either explicitly reffed or not reffed but queued into at
5d2ee9
+         * least one bus connection object. */
5d2ee9
+        assert(m->n_ref > 0 || m->n_queued > 0);
5d2ee9
+
5d2ee9
         m->n_ref++;
5d2ee9
 
5d2ee9
+        /* Each user reference to a bus message shall also be considered a ref on the bus */
5d2ee9
+        sd_bus_ref(m->bus);
5d2ee9
         return m;
5d2ee9
 }
5d2ee9
 
5d2ee9
 _public_ sd_bus_message* sd_bus_message_unref(sd_bus_message *m) {
5d2ee9
-
5d2ee9
         if (!m)
5d2ee9
                 return NULL;
5d2ee9
 
5d2ee9
         assert(m->n_ref > 0);
5d2ee9
+
5d2ee9
+        sd_bus_unref(m->bus); /* Each regular ref is also a ref on the bus connection. Let's hence drop it
5d2ee9
+                               * here. Note we have to do this before decrementing our own n_ref here, since
5d2ee9
+                               * otherwise, if this message is currently queued sd_bus_unref() might call
5d2ee9
+                               * bus_message_unref_queued() for this which might then destroy the message
5d2ee9
+                               * while we are still processing it. */
5d2ee9
         m->n_ref--;
5d2ee9
 
5d2ee9
-        if (m->n_ref > 0)
5d2ee9
+        if (m->n_ref > 0 || m->n_queued > 0)
5d2ee9
                 return NULL;
5d2ee9
 
5d2ee9
+        /* Unset the bus field if neither the user has a reference nor this message is queued. We are careful
5d2ee9
+         * to reset the field only after the last reference to the bus is dropped, after all we might keep
5d2ee9
+         * multiple references to the bus, once for each reference kept on outselves. */
5d2ee9
+        m->bus = NULL;
5d2ee9
+
5d2ee9
+        return message_free(m);
5d2ee9
+}
5d2ee9
+
5d2ee9
+sd_bus_message* bus_message_ref_queued(sd_bus_message *m, sd_bus *bus) {
5d2ee9
+        if (!m)
5d2ee9
+                return NULL;
5d2ee9
+
5d2ee9
+        /* If this is a different bus than the message is associated with, then implicitly turn this into a
5d2ee9
+         * regular reference. This means that you can create a memory leak by enqueuing a message generated
5d2ee9
+         * on one bus onto another at the same time as enqueueing a message from the second one on the first,
5d2ee9
+         * as we'll not detect the cyclic references there. */
5d2ee9
+        if (bus != m->bus)
5d2ee9
+                return sd_bus_message_ref(m);
5d2ee9
+
5d2ee9
+        assert(m->n_ref > 0 || m->n_queued > 0);
5d2ee9
+        m->n_queued++;
5d2ee9
+
5d2ee9
+        return m;
5d2ee9
+}
5d2ee9
+
5d2ee9
+sd_bus_message* bus_message_unref_queued(sd_bus_message *m, sd_bus *bus) {
5d2ee9
+        if (!m)
5d2ee9
+                return NULL;
5d2ee9
+
5d2ee9
+        if (bus != m->bus)
5d2ee9
+                return sd_bus_message_unref(m);
5d2ee9
+
5d2ee9
+        assert(m->n_queued > 0);
5d2ee9
+        m->n_queued--;
5d2ee9
+
5d2ee9
+        if (m->n_ref > 0 || m->n_queued > 0)
5d2ee9
+                return NULL;
5d2ee9
+
5d2ee9
+        m->bus = NULL;
5d2ee9
+
5d2ee9
         return message_free(m);
5d2ee9
 }
5d2ee9
 
5d2ee9
diff --git a/src/libsystemd/sd-bus/bus-message.h b/src/libsystemd/sd-bus/bus-message.h
5d2ee9
index 97f6060e30..ded88005e2 100644
5d2ee9
--- a/src/libsystemd/sd-bus/bus-message.h
5d2ee9
+++ b/src/libsystemd/sd-bus/bus-message.h
5d2ee9
@@ -51,7 +51,16 @@ struct bus_body_part {
5d2ee9
 };
5d2ee9
 
5d2ee9
 struct sd_bus_message {
5d2ee9
-        unsigned n_ref;
5d2ee9
+        /* Caveat: a message can be referenced in two different ways: the main (user-facing) way will also
5d2ee9
+         * pin the bus connection object the message is associated with. The secondary way ("queued") is used
5d2ee9
+         * when a message is in the read or write queues of the bus connection object, which will not pin the
5d2ee9
+         * bus connection object. This is necessary so that we don't have to have a pair of cyclic references
5d2ee9
+         * between a message that is queued and its connection: as soon as a message is only referenced by
5d2ee9
+         * the connection (by means of being queued) and the connection itself has no other references it
5d2ee9
+         * will be freed. */
5d2ee9
+
5d2ee9
+        unsigned n_ref;     /* Counter of references that pin the connection */
5d2ee9
+        unsigned n_queued;  /* Counter of references that do not pin the connection */
5d2ee9
 
5d2ee9
         sd_bus *bus;
5d2ee9
 
5d2ee9
@@ -216,3 +225,6 @@ int bus_message_append_sender(sd_bus_message *m, const char *sender);
5d2ee9
 
5d2ee9
 void bus_message_set_sender_driver(sd_bus *bus, sd_bus_message *m);
5d2ee9
 void bus_message_set_sender_local(sd_bus *bus, sd_bus_message *m);
5d2ee9
+
5d2ee9
+sd_bus_message* bus_message_ref_queued(sd_bus_message *m, sd_bus *bus);
5d2ee9
+sd_bus_message* bus_message_unref_queued(sd_bus_message *m, sd_bus *bus);