teknoraver / rpms / systemd

Forked from rpms/systemd 2 months ago
Clone

Blame SOURCES/0313-bus-message-introduce-two-kinds-of-references-to-bus.patch

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