From 36226a9afe96bdffce9d0697be020f2ca9d7fe6f Mon Sep 17 00:00:00 2001

From: Michal Sekletar <msekletar@users.noreply.github.com>

Date: Fri, 23 Mar 2018 15:28:06 +0100

Subject: [PATCH] core: delay adding target dependencies until all units are

 loaded and aliases resolved (#8381)

Currently we add target dependencies while we are loading units. This

can create ordering loops even if configuration doesn't contain any

loop. Take for example following configuration,

$ systemctl get-default

multi-user.target

$ cat /etc/systemd/system/test.service

[Unit]

After=default.target

[Service]

ExecStart=/bin/true

[Install]

WantedBy=multi-user.target

If we encounter such unit file early during manager start-up (e.g. load

queue is dispatched while enumerating devices due to SYSTEMD_WANTS in

udev rules) we would add stub unit default.target and we order it Before

test.service. At the same time we add implicit Before to

multi-user.target. Later we merge two units and we create ordering cycle

in the process.

To fix the issue we will now never add any target dependencies until we

loaded all the unit files and resolved all the aliases.

(cherry picked from commit 19496554e23ea4861ce780430052dcf86a2ffcba)

Resolves: #1368856

---

 src/core/manager.c | 40 ++++++++++++++++++++++++++++++++++++++++

 src/core/manager.h |  3 +++

 src/core/unit.c    | 46 ++++++++++++++++------------------------------

 src/core/unit.h    |  5 +++++

 4 files changed, 64 insertions(+), 30 deletions(-)

diff --git a/src/core/manager.c b/src/core/manager.c

index 9c406bb5bf..0466e4bb8a 100644

--- a/src/core/manager.c

+++ b/src/core/manager.c

@@ -1373,6 +1373,41 @@ Unit *manager_get_unit(Manager *m, const char *name) {

         return hashmap_get(m->units, name);

 }

+static int manager_dispatch_target_deps_queue(Manager *m) {

+        Unit *u;

+        unsigned k;

+        int r = 0;

+

+        static const UnitDependency deps[] = {

+                UNIT_REQUIRED_BY,

+                UNIT_REQUIRED_BY_OVERRIDABLE,

+                UNIT_WANTED_BY,

+                UNIT_BOUND_BY

+        };

+

+        assert(m);

+

+        while ((u = m->target_deps_queue)) {

+                assert(u->in_target_deps_queue);

+

+                LIST_REMOVE(target_deps_queue, u->manager->target_deps_queue, u);

+                u->in_target_deps_queue = false;

+

+                for (k = 0; k < ELEMENTSOF(deps); k++) {

+                        Unit *target;

+                        Iterator i;

+

+                        SET_FOREACH(target, u->dependencies[deps[k]], i) {

+                                r = unit_add_default_target_dependency(u, target);

+                                if (r < 0)

+                                        return r;

+                        }

+                }

+        }

+

+        return r;

+}

+

 unsigned manager_dispatch_load_queue(Manager *m) {

         Unit *u;

         unsigned n = 0;

@@ -1396,6 +1431,11 @@ unsigned manager_dispatch_load_queue(Manager *m) {

         }

         m->dispatching_load_queue = false;

+

+        /* Dispatch the units waiting for their target dependencies to be added now, as all targets that we know about

+         * should be loaded and have aliases resolved */

+        (void) manager_dispatch_target_deps_queue(m);

+

         return n;

 }

diff --git a/src/core/manager.h b/src/core/manager.h

index 90d2d982e1..b0e4cad1fc 100644

--- a/src/core/manager.h

+++ b/src/core/manager.h

@@ -114,6 +114,9 @@ struct Manager {

         /* Units that should be realized */

         LIST_HEAD(Unit, cgroup_queue);

+        /* Target units whose default target dependencies haven't been set yet */

+        LIST_HEAD(Unit, target_deps_queue);

+

         sd_event *event;

         /* We use two hash tables here, since the same PID might be

diff --git a/src/core/unit.c b/src/core/unit.c

index 22d9beed76..cfddce34d4 100644

--- a/src/core/unit.c

+++ b/src/core/unit.c

@@ -519,6 +519,9 @@ void unit_free(Unit *u) {

                 u->manager->n_in_gc_queue--;

         }

+        if (u->in_target_deps_queue)

+                LIST_REMOVE(target_deps_queue, u->manager->target_deps_queue, u);

+

         if (u->in_cgroup_queue)

                 LIST_REMOVE(cgroup_queue, u->manager->cgroup_queue, u);

@@ -1065,6 +1068,18 @@ int unit_load_fragment_and_dropin_optional(Unit *u) {

         return 0;

 }

+void unit_add_to_target_deps_queue(Unit *u) {

+        Manager *m = u->manager;

+

+        assert(u);

+

+        if (u->in_target_deps_queue)

+                return;

+

+        LIST_PREPEND(target_deps_queue, m->target_deps_queue, u);

+        u->in_target_deps_queue = true;

+}

+

 int unit_add_default_target_dependency(Unit *u, Unit *target) {

         assert(u);

         assert(target);

@@ -1091,32 +1106,6 @@ int unit_add_default_target_dependency(Unit *u, Unit *target) {

         return unit_add_dependency(target, UNIT_AFTER, u, true);

 }

-static int unit_add_target_dependencies(Unit *u) {

-

-        static const UnitDependency deps[] = {

-                UNIT_REQUIRED_BY,

-                UNIT_REQUIRED_BY_OVERRIDABLE,

-                UNIT_WANTED_BY,

-                UNIT_BOUND_BY

-        };

-

-        Unit *target;

-        Iterator i;

-        unsigned k;

-        int r = 0;

-

-        assert(u);

-

-        for (k = 0; k < ELEMENTSOF(deps); k++)

-                SET_FOREACH(target, u->dependencies[deps[k]], i) {

-                        r = unit_add_default_target_dependency(u, target);

-                        if (r < 0)

-                                return r;

-                }

-

-        return r;

-}

-

 static int unit_add_slice_dependencies(Unit *u) {

         assert(u);

@@ -1217,10 +1206,7 @@ int unit_load(Unit *u) {

         }

         if (u->load_state == UNIT_LOADED) {

-

-                r = unit_add_target_dependencies(u);

-                if (r < 0)

-                        goto fail;

+                unit_add_to_target_deps_queue(u);

                 r = unit_add_slice_dependencies(u);

                 if (r < 0)

diff --git a/src/core/unit.h b/src/core/unit.h

index 480e2e95f1..dfec9cea01 100644

--- a/src/core/unit.h

+++ b/src/core/unit.h

@@ -162,6 +162,9 @@ struct Unit {

         /* CGroup realize members queue */

         LIST_FIELDS(Unit, cgroup_queue);

+        /* Target dependencies queue */

+        LIST_FIELDS(Unit, target_deps_queue);

+

         /* PIDs we keep an eye on. Note that a unit might have many

          * more, but these are the ones we care enough about to

          * process SIGCHLD for */

@@ -228,6 +231,7 @@ struct Unit {

         bool in_cleanup_queue:1;

         bool in_gc_queue:1;

         bool in_cgroup_queue:1;

+        bool in_target_deps_queue:1;

         bool sent_dbus_new_signal:1;

@@ -498,6 +502,7 @@ void unit_add_to_load_queue(Unit *u);

 void unit_add_to_dbus_queue(Unit *u);

 void unit_add_to_cleanup_queue(Unit *u);

 void unit_add_to_gc_queue(Unit *u);

+void unit_add_to_target_deps_queue(Unit *u);

 int unit_merge(Unit *u, Unit *other);

 int unit_merge_by_name(Unit *u, const char *other);