From 3ef091125ceb159374d2d0d4dbad84d90b252852 Mon Sep 17 00:00:00 2001

From: Ivan Shapovalov <intelfx100@gmail.com>

Date: Sat, 7 Mar 2015 08:44:52 -0500

Subject: [PATCH] core: do not spawn jobs or touch other units during

 coldplugging

Because the order of coldplugging is not defined, we can reference a

not-yet-coldplugged unit and read its state while it has not yet been

set to a meaningful value.

This way, already active units may get started again.

We fix this by deferring such actions until all units have been at

least somehow coldplugged.

Fixes https://bugs.freedesktop.org/show_bug.cgi?id=88401

(cherry picked from commit 6e392c9c45643d106673c6643ac8bf4e65da13c1)

---

 src/core/automount.c |  2 +-

 src/core/busname.c   |  2 +-

 src/core/device.c    |  2 +-

 src/core/manager.c   | 35 +++++++++++++++++++++++++++++++++--

 src/core/mount.c     |  2 +-

 src/core/path.c      | 14 ++++++++++----

 src/core/scope.c     |  2 +-

 src/core/service.c   |  2 +-

 src/core/slice.c     |  2 +-

 src/core/snapshot.c  |  2 +-

 src/core/socket.c    |  2 +-

 src/core/swap.c      |  2 +-

 src/core/target.c    |  2 +-

 src/core/timer.c     | 14 ++++++++++----

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

 src/core/unit.h      | 12 +++++++++---

 16 files changed, 89 insertions(+), 33 deletions(-)

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

index 9f6bd84b21..e4c79415d1 100644

--- a/src/core/automount.c

+++ b/src/core/automount.c

@@ -235,7 +235,7 @@ static void automount_set_state(Automount *a, AutomountState state) {

         unit_notify(UNIT(a), state_translation_table[old_state], state_translation_table[state], true);

 }

-static int automount_coldplug(Unit *u) {

+static int automount_coldplug(Unit *u, Hashmap *deferred_work) {

         Automount *a = AUTOMOUNT(u);

         int r;

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

index 1d77292f9b..43d7607a30 100644

--- a/src/core/busname.c

+++ b/src/core/busname.c

@@ -335,7 +335,7 @@ static void busname_set_state(BusName *n, BusNameState state) {

         unit_notify(UNIT(n), state_translation_table[old_state], state_translation_table[state], true);

 }

-static int busname_coldplug(Unit *u) {

+static int busname_coldplug(Unit *u, Hashmap *deferred_work) {

         BusName *n = BUSNAME(u);

         int r;

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

index 1cc103c290..4ff8827219 100644

--- a/src/core/device.c

+++ b/src/core/device.c

@@ -142,7 +142,7 @@ static void device_set_state(Device *d, DeviceState state) {

         unit_notify(UNIT(d), state_translation_table[old_state], state_translation_table[state], true);

 }

-static int device_coldplug(Unit *u) {

+static int device_coldplug(Unit *u, Hashmap *deferred_work) {

         Device *d = DEVICE(u);

         assert(d);

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

index bc9b7ec620..203a6a0a1a 100644

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

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

@@ -983,7 +983,28 @@ static int manager_coldplug(Manager *m) {

         Unit *u;

         char *k;

-        assert(m);

+        /*

+         * Some unit types tend to spawn jobs or check other units' state

+         * during coldplug. This is wrong because it is undefined whether the

+         * units in question have been already coldplugged (i. e. their state

+         * restored). This way, we can easily re-start an already started unit

+         * or otherwise make a wrong decision based on the unit's state.

+         *

+         * Solve this by providing a way for coldplug functions to defer

+         * such actions until after all units have been coldplugged.

+         *

+         * We store Unit* -> int(*)(Unit*).

+         *

+         * https://bugs.freedesktop.org/show_bug.cgi?id=88401

+         */

+        _cleanup_hashmap_free_ Hashmap *deferred_work = NULL;

+        int(*proc)(Unit*);

+

+        assert(m);

+

+        deferred_work = hashmap_new(&trivial_hash_ops);

+        if (!deferred_work)

+                return -ENOMEM;

         /* Then, let's set up their initial state. */

         HASHMAP_FOREACH_KEY(u, k, m->units, i) {

@@ -993,7 +1014,17 @@ static int manager_coldplug(Manager *m) {

                 if (u->id != k)

                         continue;

-                q = unit_coldplug(u);

+                q = unit_coldplug(u, deferred_work);

+                if (q < 0)

+                        r = q;

+        }

+

+        /* After coldplugging and setting up initial state of the units,

+         * let's perform operations which spawn jobs or query units' state. */

+        HASHMAP_FOREACH_KEY(proc, u, deferred_work, i) {

+                int q;

+

+                q = proc(u);

                 if (q < 0)

                         r = q;

         }

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

index c971330af2..3ae0eb4621 100644

--- a/src/core/mount.c

+++ b/src/core/mount.c

@@ -617,7 +617,7 @@ static void mount_set_state(Mount *m, MountState state) {

         m->reload_result = MOUNT_SUCCESS;

 }

-static int mount_coldplug(Unit *u) {

+static int mount_coldplug(Unit *u, Hashmap *deferred_work) {

         Mount *m = MOUNT(u);

         MountState new_state = MOUNT_DEAD;

         int r;

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

index e5ea79fec7..51e36fa8be 100644

--- a/src/core/path.c

+++ b/src/core/path.c

@@ -440,7 +440,12 @@ static void path_set_state(Path *p, PathState state) {

 static void path_enter_waiting(Path *p, bool initial, bool recheck);

-static int path_coldplug(Unit *u) {

+static int path_enter_waiting_coldplug(Unit *u) {

+        path_enter_waiting(PATH(u), true, true);

+        return 0;

+}

+

+static int path_coldplug(Unit *u, Hashmap *deferred_work) {

         Path *p = PATH(u);

         assert(p);

@@ -449,9 +454,10 @@ static int path_coldplug(Unit *u) {

         if (p->deserialized_state != p->state) {

                 if (p->deserialized_state == PATH_WAITING ||

-                    p->deserialized_state == PATH_RUNNING)

-                        path_enter_waiting(p, true, true);

-                else

+                    p->deserialized_state == PATH_RUNNING) {

+                        hashmap_put(deferred_work, u, &path_enter_waiting_coldplug);

+                        path_set_state(p, PATH_WAITING);

+                } else

                         path_set_state(p, p->deserialized_state);

         }

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

index b41db7872c..ae6614fbf0 100644

--- a/src/core/scope.c

+++ b/src/core/scope.c

@@ -173,7 +173,7 @@ static int scope_load(Unit *u) {

         return scope_verify(s);

 }

-static int scope_coldplug(Unit *u) {

+static int scope_coldplug(Unit *u, Hashmap *deferred_work) {

         Scope *s = SCOPE(u);

         int r;

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

index 15e29be149..7781b4e626 100644

--- a/src/core/service.c

+++ b/src/core/service.c

@@ -879,7 +879,7 @@ static void service_set_state(Service *s, ServiceState state) {

         s->reload_result = SERVICE_SUCCESS;

 }

-static int service_coldplug(Unit *u) {

+static int service_coldplug(Unit *u, Hashmap *deferred_work) {

         Service *s = SERVICE(u);

         int r;

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

index ae9819d015..61ff9d3314 100644

--- a/src/core/slice.c

+++ b/src/core/slice.c

@@ -153,7 +153,7 @@ static int slice_load(Unit *u) {

         return slice_verify(s);

 }

-static int slice_coldplug(Unit *u) {

+static int slice_coldplug(Unit *u, Hashmap *deferred_work) {

         Slice *t = SLICE(u);

         assert(t);

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

index b70c3beb60..b1d8448771 100644

--- a/src/core/snapshot.c

+++ b/src/core/snapshot.c

@@ -75,7 +75,7 @@ static int snapshot_load(Unit *u) {

         return 0;

 }

-static int snapshot_coldplug(Unit *u) {

+static int snapshot_coldplug(Unit *u, Hashmap *deferred_work) {

         Snapshot *s = SNAPSHOT(u);

         assert(s);

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

index 88aae4815b..760de0203d 100644

--- a/src/core/socket.c

+++ b/src/core/socket.c

@@ -1326,7 +1326,7 @@ static void socket_set_state(Socket *s, SocketState state) {

         unit_notify(UNIT(s), state_translation_table[old_state], state_translation_table[state], true);

 }

-static int socket_coldplug(Unit *u) {

+static int socket_coldplug(Unit *u, Hashmap *deferred_work) {

         Socket *s = SOCKET(u);

         int r;

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

index 5c19af5d91..369abf0f53 100644

--- a/src/core/swap.c

+++ b/src/core/swap.c

@@ -513,7 +513,7 @@ static void swap_set_state(Swap *s, SwapState state) {

                         job_add_to_run_queue(UNIT(other)->job);

 }

-static int swap_coldplug(Unit *u) {

+static int swap_coldplug(Unit *u, Hashmap *deferred_work) {

         Swap *s = SWAP(u);

         SwapState new_state = SWAP_DEAD;

         int r;

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

index 33fb66bc3f..2411a8e758 100644

--- a/src/core/target.c

+++ b/src/core/target.c

@@ -107,7 +107,7 @@ static int target_load(Unit *u) {

         return 0;

 }

-static int target_coldplug(Unit *u) {

+static int target_coldplug(Unit *u, Hashmap *deferred_work) {

         Target *t = TARGET(u);

         assert(t);

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

index 45744c7de5..48cf9c16a8 100644

--- a/src/core/timer.c

+++ b/src/core/timer.c

@@ -268,7 +268,12 @@ static void timer_set_state(Timer *t, TimerState state) {

 static void timer_enter_waiting(Timer *t, bool initial);

-static int timer_coldplug(Unit *u) {

+static int timer_enter_waiting_coldplug(Unit *u) {

+        timer_enter_waiting(TIMER(u), false);

+        return 0;

+}

+

+static int timer_coldplug(Unit *u, Hashmap *deferred_work) {

         Timer *t = TIMER(u);

         assert(t);

@@ -276,9 +281,10 @@ static int timer_coldplug(Unit *u) {

         if (t->deserialized_state != t->state) {

-                if (t->deserialized_state == TIMER_WAITING)

-                        timer_enter_waiting(t, false);

-                else

+                if (t->deserialized_state == TIMER_WAITING) {

+                        hashmap_put(deferred_work, u, &timer_enter_waiting_coldplug);

+                        timer_set_state(t, TIMER_WAITING);

+                } else

                         timer_set_state(t, t->deserialized_state);

         }

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

index a6558ee23b..565455bd63 100644

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

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

@@ -2859,27 +2859,34 @@ int unit_add_node_link(Unit *u, const char *what, bool wants) {

         return 0;

 }

-int unit_coldplug(Unit *u) {

+static int unit_add_deserialized_job_coldplug(Unit *u) {

+        int r;

+

+        r = manager_add_job(u->manager, u->deserialized_job, u, JOB_IGNORE_REQUIREMENTS, false, NULL, NULL);

+        if (r < 0)

+                return r;

+

+        u->deserialized_job = _JOB_TYPE_INVALID;

+

+        return 0;

+}

+

+int unit_coldplug(Unit *u, Hashmap *deferred_work) {

         int r;

         assert(u);

         if (UNIT_VTABLE(u)->coldplug)

-                if ((r = UNIT_VTABLE(u)->coldplug(u)) < 0)

+                if ((r = UNIT_VTABLE(u)->coldplug(u, deferred_work)) < 0)

                         return r;

         if (u->job) {

                 r = job_coldplug(u->job);

                 if (r < 0)

                         return r;

-        } else if (u->deserialized_job >= 0) {

+        } else if (u->deserialized_job >= 0)

                 /* legacy */

-                r = manager_add_job(u->manager, u->deserialized_job, u, JOB_IGNORE_REQUIREMENTS, false, NULL, NULL);

-                if (r < 0)

-                        return r;

-

-                u->deserialized_job = _JOB_TYPE_INVALID;

-        }

+                hashmap_put(deferred_work, u, &unit_add_deserialized_job_coldplug);

         return 0;

 }

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

index 291bc77a76..7ebc489c80 100644

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

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

@@ -307,8 +307,14 @@ struct UnitVTable {

         int (*load)(Unit *u);

         /* If a lot of units got created via enumerate(), this is

-         * where to actually set the state and call unit_notify(). */

-        int (*coldplug)(Unit *u);

+         * where to actually set the state and call unit_notify().

+         *

+         * This must not reference other units (maybe implicitly through spawning

+         * jobs), because it is possible that they are not yet coldplugged.

+         * Such actions must be deferred until the end of coldplug bу adding

+         * a "Unit* -> int(*)(Unit*)" entry into the hashmap.

+         */

+        int (*coldplug)(Unit *u, Hashmap *deferred_work);

         void (*dump)(Unit *u, FILE *f, const char *prefix);

@@ -544,7 +550,7 @@ int unit_deserialize(Unit *u, FILE *f, FDSet *fds);

 int unit_add_node_link(Unit *u, const char *what, bool wants);

-int unit_coldplug(Unit *u);

+int unit_coldplug(Unit *u, Hashmap *deferred_work);

 void unit_status_printf(Unit *u, const char *status, const char *unit_status_msg_format) _printf_(3, 0);