Fix race between dbus addmatch and checking for service name change.

LP: #1896614

2 files changed, 185 insertions, 0 deletions

diff --git a/debian/patches/lp1896614-core-Avoid-race-when-starting-dbus-services.patch b/debian/patches/lp1896614-core-Avoid-race-when-starting-dbus-services.patch
new file mode 100644
index 0000000000..ebd9351d86
--- /dev/null
+++ b/debian/patches/lp1896614-core-Avoid-race-when-starting-dbus-services.patch
@@ -0,0 +1,184 @@
+From a5a8776ae5e4244b7f5acb2a1bfbe6e0b4d8a870 Mon Sep 17 00:00:00 2001
+From: Mattias Jernberg <mattiasj@axis.com>
+Date: Thu, 11 Jul 2019 18:13:46 +0200
+Subject: [PATCH] core: Avoid race when starting dbus services
+Origin: upstream, https://github.com/systemd/systemd/commit/a5a8776ae5e4244b7f5acb2a1bfbe6e0b4d8a870
+Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/systemd/+bug/1896614
+
+In high load scenarios it is possible for services to be started
+before the NameOwnerChanged signal is properly installed.
+
+Emulate a callback by also queuing a GetNameOwner when the match is
+installed.
+
+Fixes: #12956
+---
+ src/core/dbus.c    |  4 ++--
+ src/core/service.c | 13 +++++------
+ src/core/unit.c    | 56 ++++++++++++++++++++++++++++++++++++++++++++--
+ src/core/unit.h    |  3 ++-
+ 4 files changed, 63 insertions(+), 13 deletions(-)
+
+--- a/src/core/dbus.c
++++ b/src/core/dbus.c
+@@ -776,7 +776,7 @@
+                          * changed, so synthesize a name owner changed signal. */
+ 
+                         if (!streq_ptr(unique, s->bus_name_owner))
+-                                UNIT_VTABLE(u)->bus_name_owner_change(u, name, s->bus_name_owner, unique);
++                                UNIT_VTABLE(u)->bus_name_owner_change(u, s->bus_name_owner, unique);
+                 } else {
+                         /* So, the name we're watching is not on the bus.
+                          * This either means it simply hasn't appeared yet,
+@@ -785,7 +785,7 @@
+                          * and synthesize a name loss signal in this case. */
+ 
+                         if (s->bus_name_owner)
+-                                UNIT_VTABLE(u)->bus_name_owner_change(u, name, s->bus_name_owner, NULL);
++                                UNIT_VTABLE(u)->bus_name_owner_change(u, s->bus_name_owner, NULL);
+                 }
+         }
+ 
+--- a/src/core/service.c
++++ b/src/core/service.c
+@@ -3653,7 +3653,6 @@
+ 
+ static void service_bus_name_owner_change(
+                 Unit *u,
+-                const char *name,
+                 const char *old_owner,
+                 const char *new_owner) {
+ 
+@@ -3661,17 +3660,15 @@
+         int r;
+ 
+         assert(s);
+-        assert(name);
+ 
+-        assert(streq(s->bus_name, name));
+         assert(old_owner || new_owner);
+ 
+         if (old_owner && new_owner)
+-                log_unit_debug(u, "D-Bus name %s changed owner from %s to %s", name, old_owner, new_owner);
++                log_unit_debug(u, "D-Bus name %s changed owner from %s to %s", s->bus_name, old_owner, new_owner);
+         else if (old_owner)
+-                log_unit_debug(u, "D-Bus name %s no longer registered by %s", name, old_owner);
++                log_unit_debug(u, "D-Bus name %s no longer registered by %s", s->bus_name, old_owner);
+         else
+-                log_unit_debug(u, "D-Bus name %s now registered by %s", name, new_owner);
++                log_unit_debug(u, "D-Bus name %s now registered by %s", s->bus_name, new_owner);
+ 
+         s->bus_name_good = !!new_owner;
+ 
+@@ -3704,11 +3701,11 @@
+ 
+                 /* Try to acquire PID from bus service */
+ 
+-                r = sd_bus_get_name_creds(u->manager->api_bus, name, SD_BUS_CREDS_PID, &creds);
++                r = sd_bus_get_name_creds(u->manager->api_bus, s->bus_name, SD_BUS_CREDS_PID, &creds);
+                 if (r >= 0)
+                         r = sd_bus_creds_get_pid(creds, &pid);
+                 if (r >= 0) {
+-                        log_unit_debug(u, "D-Bus name %s is now owned by process " PID_FMT, name, pid);
++                        log_unit_debug(u, "D-Bus name %s is now owned by process " PID_FMT, s->bus_name, pid);
+ 
+                         service_set_main_pid(s, pid);
+                         unit_watch_pid(UNIT(s), pid);
+--- a/src/core/unit.c
++++ b/src/core/unit.c
+@@ -3091,7 +3091,46 @@
+         new_owner = empty_to_null(new_owner);
+ 
+         if (UNIT_VTABLE(u)->bus_name_owner_change)
+-                UNIT_VTABLE(u)->bus_name_owner_change(u, name, old_owner, new_owner);
++                UNIT_VTABLE(u)->bus_name_owner_change(u, old_owner, new_owner);
++
++        return 0;
++}
++
++static int get_name_owner_handler(sd_bus_message *message, void *userdata, sd_bus_error *error) {
++        const sd_bus_error *e;
++        const char *new_owner;
++        Unit *u = userdata;
++        int r;
++
++        assert(message);
++        assert(u);
++
++        u->get_name_owner_slot = sd_bus_slot_unref(u->get_name_owner_slot);
++
++        if (sd_bus_error_is_set(error)) {
++                log_error("Failed to get name owner from bus: %s", error->message);
++                return 0;
++        }
++
++        e = sd_bus_message_get_error(message);
++        if (sd_bus_error_has_name(e, "org.freedesktop.DBus.Error.NameHasNoOwner"))
++                return 0;
++
++        if (e) {
++                log_error("Unexpected error response from GetNameOwner: %s", e->message);
++                return 0;
++        }
++
++        r = sd_bus_message_read(message, "s", &new_owner);
++        if (r < 0) {
++                bus_log_parse_error(r);
++                return 0;
++        }
++
++        new_owner = empty_to_null(new_owner);
++
++        if (UNIT_VTABLE(u)->bus_name_owner_change)
++                UNIT_VTABLE(u)->bus_name_owner_change(u, NULL, new_owner);
+ 
+         return 0;
+ }
+@@ -3113,7 +3152,19 @@
+                          "member='NameOwnerChanged',"
+                          "arg0='", name, "'");
+ 
+-        return sd_bus_add_match_async(bus, &u->match_bus_slot, match, signal_name_owner_changed, NULL, u);
++        int r = sd_bus_add_match_async(bus, &u->match_bus_slot, match, signal_name_owner_changed, NULL, u);
++        if (r < 0)
++                return r;
++
++        return sd_bus_call_method_async(bus,
++                                        &u->get_name_owner_slot,
++                                        "org.freedesktop.DBus",
++                                        "/org/freedesktop/DBus",
++                                        "org.freedesktop.DBus",
++                                        "GetNameOwner",
++                                        get_name_owner_handler,
++                                        u,
++                                        "s", name);
+ }
+ 
+ int unit_watch_bus_name(Unit *u, const char *name) {
+@@ -3148,6 +3199,7 @@
+ 
+         (void) hashmap_remove_value(u->manager->watch_bus, name, u);
+         u->match_bus_slot = sd_bus_slot_unref(u->match_bus_slot);
++        u->get_name_owner_slot = sd_bus_slot_unref(u->get_name_owner_slot);
+ }
+ 
+ bool unit_can_serialize(Unit *u) {
+--- a/src/core/unit.h
++++ b/src/core/unit.h
+@@ -174,6 +174,7 @@
+ 
+         /* The slot used for watching NameOwnerChanged signals */
+         sd_bus_slot *match_bus_slot;
++        sd_bus_slot *get_name_owner_slot;
+ 
+         /* References to this unit from clients */
+         sd_bus_track *bus_track;
+@@ -512,7 +513,7 @@
+         void (*notify_message)(Unit *u, const struct ucred *ucred, char **tags, FDSet *fds);
+ 
+         /* Called whenever a name this Unit registered for comes or goes away. */
+-        void (*bus_name_owner_change)(Unit *u, const char *name, const char *old_owner, const char *new_owner);
++        void (*bus_name_owner_change)(Unit *u, const char *old_owner, const char *new_owner);
+ 
+         /* Called for each property that is being set */
+         int (*bus_set_property)(Unit *u, const char *name, sd_bus_message *message, UnitWriteFlags flags, sd_bus_error *error);
diff --git a/debian/patches/series b/debian/patches/series
index d061bd6bc0..50c0557464 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -222,3 +222,4 @@ lp1832754/0001-umount-Try-unmounting-even-if-remounting-read-only-f.patch
 lp1832754/0002-umount-Don-t-bother-remounting-api-and-ro-filesystem.patch
 lp1886115-pid1-fix-free-of-uninitialized-pointer-in-unit_fail_.patch
 lp1830746-bump-mlock-ulimit-to-64Mb.patch
+lp1896614-core-Avoid-race-when-starting-dbus-services.patch