Make systemctl --root look for files in the proper places

Running systemctl enable/disable/set-default/... with the --root
option under strace reveals that it accessed various files and
directories in the main fs, and not underneath the specified root.
This can lead to correct results only when the layout and
configuration in the container are identical, which often is not the
case. Fix this by adding the specified root to all file access
operations.

This patch does not handle some corner cases: symlinks which point
outside of the specified root might be interpreted differently than
they would be by the kernel if the specified root was the real root.
But systemctl does not create such symlinks by itself, and I think
this is enough of a corner case not to be worth the additional
complexity of reimplementing link chasing in systemd.

Also, simplify the code in a few places and remove an hypothetical
memory leak on error.

diff --git a/TODO b/TODO
index 3154fbd1654c9651d8e57d2429073f2edbfc4226..37a57130dda2585cee17f82a2eefdac8b3982eb2 100644 (file)
--- a/TODO
+++ b/TODO
@@ -16,8 +16,6 @@ Bugfixes:
 
   Cannot add dependency job for unit display-manager.service, ignoring: Unit display-manager.service failed to load: No such file or directory. See system logs and 'systemctl status display-manager.service' for details.
 
-* systemctl --root=container/ set-default ... is totally borked.
-
 * sd_bus_unref() is broken regarding self-references and "pseudo thread-safety".
   See the comment in sd_bus_unref() for more..
 

diff --git a/src/core/manager.c b/src/core/manager.c
index 5772f402b1f41885a579561157e5a8edf9c3838d..1e3e1273adadca0c1b105ac30315ec6bc10ae2bd 100644 (file)
--- a/src/core/manager.c
+++ b/src/core/manager.c
@@ -968,6 +968,7 @@ int manager_startup(Manager *m, FILE *serialization, FDSet *fds) {
 
         r = lookup_paths_init(
                         &m->lookup_paths, m->running_as, true,
+                        NULL,
                         m->generator_unit_path,
                         m->generator_unit_path_early,
                         m->generator_unit_path_late);
@@ -2374,6 +2375,7 @@ int manager_reload(Manager *m) {
 
         q = lookup_paths_init(
                         &m->lookup_paths, m->running_as, true,
+                        NULL,
                         m->generator_unit_path,
                         m->generator_unit_path_early,
                         m->generator_unit_path_late);

diff --git a/src/shared/install.c b/src/shared/install.c
index 2822e6188009c595c37482deddc8378a02766b91..487d0f660d40b6f13d3be1435e9d4a65a6dd187c 100644 (file)
--- a/src/shared/install.c
+++ b/src/shared/install.c
@@ -47,7 +47,9 @@ typedef struct {
 
 #define _cleanup_install_context_done_ _cleanup_(install_context_done)
 
-static int lookup_paths_init_from_scope(LookupPaths *paths, UnitFileScope scope) {
+static int lookup_paths_init_from_scope(LookupPaths *paths,
+                                        UnitFileScope scope,
+                                        const char *root_dir) {
         assert(paths);
         assert(scope >= 0);
         assert(scope < _UNIT_FILE_SCOPE_MAX);
@@ -57,6 +59,7 @@ static int lookup_paths_init_from_scope(LookupPaths *paths, UnitFileScope scope)
         return lookup_paths_init(paths,
                                  scope == UNIT_FILE_SYSTEM ? SYSTEMD_SYSTEM : SYSTEMD_USER,
                                  scope == UNIT_FILE_USER,
+                                 root_dir,
                                  NULL, NULL, NULL);
 }
 
@@ -701,7 +704,7 @@ int unit_file_link(
         assert(scope >= 0);
         assert(scope < _UNIT_FILE_SCOPE_MAX);
 
-        r = lookup_paths_init_from_scope(&paths, scope);
+        r = lookup_paths_init_from_scope(&paths, scope, root_dir);
         if (r < 0)
                 return r;
 
@@ -1473,7 +1476,7 @@ int unit_file_enable(
         assert(scope >= 0);
         assert(scope < _UNIT_FILE_SCOPE_MAX);
 
-        r = lookup_paths_init_from_scope(&paths, scope);
+        r = lookup_paths_init_from_scope(&paths, scope, root_dir);
         if (r < 0)
                 return r;
 
@@ -1513,7 +1516,7 @@ int unit_file_disable(
         assert(scope >= 0);
         assert(scope < _UNIT_FILE_SCOPE_MAX);
 
-        r = lookup_paths_init_from_scope(&paths, scope);
+        r = lookup_paths_init_from_scope(&paths, scope, root_dir);
         if (r < 0)
                 return r;
 
@@ -1577,7 +1580,7 @@ int unit_file_set_default(
         if (unit_name_to_type(file) != UNIT_TARGET)
                 return -EINVAL;
 
-        r = lookup_paths_init_from_scope(&paths, scope);
+        r = lookup_paths_init_from_scope(&paths, scope, root_dir);
         if (r < 0)
                 return r;
 
@@ -1617,7 +1620,7 @@ int unit_file_get_default(
         assert(scope < _UNIT_FILE_SCOPE_MAX);
         assert(name);
 
-        r = lookup_paths_init_from_scope(&paths, scope);
+        r = lookup_paths_init_from_scope(&paths, scope, root_dir);
         if (r < 0)
                 return r;
 
@@ -1675,12 +1678,13 @@ UnitFileState unit_file_get_state(
         if (!unit_name_is_valid(name, TEMPLATE_VALID))
                 return -EINVAL;
 
-        r = lookup_paths_init_from_scope(&paths, scope);
+        r = lookup_paths_init_from_scope(&paths, scope, root_dir);
         if (r < 0)
                 return r;
 
         STRV_FOREACH(i, paths.unit_path) {
                 struct stat st;
+                char *partial;
 
                 free(path);
                 path = NULL;
@@ -1689,10 +1693,14 @@ UnitFileState unit_file_get_state(
                         asprintf(&path, "%s/%s/%s", root_dir, *i, name);
                 else
                         asprintf(&path, "%s/%s", *i, name);
-
                 if (!path)
                         return -ENOMEM;
 
+                if (root_dir)
+                        partial = path + strlen(root_dir) + 1;
+                else
+                        partial = path;
+
                 /*
                  * Search for a unit file in our default paths, to
                  * be sure, that there are no broken symlinks.
@@ -1724,7 +1732,7 @@ UnitFileState unit_file_get_state(
                 else if (r > 0)
                         return state;
 
-                r = unit_file_can_install(&paths, root_dir, path, true);
+                r = unit_file_can_install(&paths, root_dir, partial, true);
                 if (r < 0 && errno != ENOENT)
                         return r;
                 else if (r > 0)
@@ -1832,7 +1840,7 @@ int unit_file_preset(
         assert(scope >= 0);
         assert(scope < _UNIT_FILE_SCOPE_MAX);
 
-        r = lookup_paths_init_from_scope(&paths, scope);
+        r = lookup_paths_init_from_scope(&paths, scope, root_dir);
         if (r < 0)
                 return r;
 
@@ -1902,7 +1910,7 @@ int unit_file_get_list(
         if (root_dir && scope != UNIT_FILE_SYSTEM)
                 return -EINVAL;
 
-        r = lookup_paths_init_from_scope(&paths, scope);
+        r = lookup_paths_init_from_scope(&paths, scope, root_dir);
         if (r < 0)
                 return r;
 

diff --git a/src/shared/path-lookup.c b/src/shared/path-lookup.c
index 63af43cdbb0de55542e54e54b88f0321a5d8bb70..c6b4ba1e22bff59117b7ad90723ab4108ef76226 100644 (file)
--- a/src/shared/path-lookup.c
+++ b/src/shared/path-lookup.c
@@ -198,6 +198,7 @@ int lookup_paths_init(
                 LookupPaths *p,
                 SystemdRunningAs running_as,
                 bool personal,
+                const char *root_dir,
                 const char *generator,
                 const char *generator_early,
                 const char *generator_late) {
@@ -275,11 +276,9 @@ int lookup_paths_init(
                 }
         }
 
-        if (!path_strv_canonicalize_absolute(p->unit_path, NULL))
+        if (!path_strv_canonicalize_absolute_uniq(p->unit_path, root_dir))
                 return -ENOMEM;
 
-        strv_uniq(p->unit_path);
-
         if (!strv_isempty(p->unit_path)) {
                 _cleanup_free_ char *t = strv_join(p->unit_path, "\n\t");
                 if (!t)
@@ -331,15 +330,12 @@ int lookup_paths_init(
                                 return -ENOMEM;
                 }
 
-                if (!path_strv_canonicalize_absolute(p->sysvinit_path, NULL))
+                if (!path_strv_canonicalize_absolute_uniq(p->sysvinit_path, root_dir))
                         return -ENOMEM;
 
-                if (!path_strv_canonicalize_absolute(p->sysvrcnd_path, NULL))
+                if (!path_strv_canonicalize_absolute_uniq(p->sysvrcnd_path, root_dir))
                         return -ENOMEM;
 
-                strv_uniq(p->sysvinit_path);
-                strv_uniq(p->sysvrcnd_path);
-
                 if (!strv_isempty(p->sysvinit_path)) {
                         _cleanup_free_ char *t = strv_join(p->sysvinit_path, "\n\t");
                         if (!t)

diff --git a/src/shared/path-lookup.h b/src/shared/path-lookup.h
index a3ef824a8603e42e59775bc3fdaa8d7ad5434b8e..847a52f20a28662235e292e4e9ff1d6211f037bb 100644 (file)
--- a/src/shared/path-lookup.h
+++ b/src/shared/path-lookup.h
@@ -43,5 +43,11 @@ SystemdRunningAs systemd_running_as_from_string(const char *s) _pure_;
 
 int user_config_home(char **config_home);
 
-int lookup_paths_init(LookupPaths *p, SystemdRunningAs running_as, bool personal, const char *generator, const char *generator_early, const char *generator_late);
+int lookup_paths_init(LookupPaths *p,
+                      SystemdRunningAs running_as,
+                      bool personal,
+                      const char *root_dir,
+                      const char *generator,
+                      const char *generator_early,
+                      const char *generator_late);
 void lookup_paths_free(LookupPaths *p);

diff --git a/src/shared/path-util.c b/src/shared/path-util.c
index 69fcb1660a134531830047795a3edd77d7ef6eb2..8bf9a3cf96c1cc809c01b192df6ed7cfd426a044 100644 (file)
--- a/src/shared/path-util.c
+++ b/src/shared/path-util.c
@@ -167,36 +167,63 @@ char **path_strv_canonicalize_absolute(char **l, const char *prefix) {
 
         STRV_FOREACH(s, l) {
                 char *t, *u;
+                _cleanup_free_ char *orig = NULL;
 
-                if (!path_is_absolute(*s))
+                if (!path_is_absolute(*s)) {
+                        free(*s);
                         continue;
+                }
 
                 if (prefix) {
-                        t = strappend(prefix, *s);
-                        free(*s);
-                        *s = NULL;
-
+                        orig = *s;
+                        t = strappend(prefix, orig);
                         if (!t) {
                                 enomem = true;
                                 continue;
                         }
-                } else {
+                } else
                         t = *s;
-                        *s = NULL;
-                }
 
                 errno = 0;
                 u = canonicalize_file_name(t);
                 if (!u) {
-                        if (errno == ENOENT)
-                                u = t;
-                        else {
+                        if (errno == ENOENT) {
+                                if (prefix) {
+                                        u = orig;
+                                        orig = NULL;
+                                        free(t);
+                                } else
+                                        u = t;
+                        } else {
                                 free(t);
                                 if (errno == ENOMEM || errno == 0)
                                         enomem = true;
 
                                 continue;
                         }
+                } else if (prefix) {
+                        char *x;
+
+                        free(t);
+                        x = path_startswith(u, prefix);
+                        if (x) {
+                                /* restore the slash if it was lost */
+                                if (!startswith(x, "/"))
+                                        *(--x) = '/';
+
+                                t = strdup(x);
+                                free(u);
+                                if (!t) {
+                                        enomem = true;
+                                        continue;
+                                }
+                                u = t;
+                        } else {
+                                /* canonicalized path goes outside of
+                                 * prefix, keep the original path instead */
+                                u = orig;
+                                orig = NULL;
+                        }
                 } else
                         free(t);
 

diff --git a/src/systemctl/systemctl.c b/src/systemctl/systemctl.c
index 91d8032945fbe5899287a859f528f9d890a347ba..a60a3019887fd821928960e180231ff0533ec030 100644 (file)
--- a/src/systemctl/systemctl.c
+++ b/src/systemctl/systemctl.c
@@ -5002,7 +5002,7 @@ static int enable_sysv_units(const char *verb, char **args) {
         /* Processes all SysV units, and reshuffles the array so that
          * afterwards only the native units remain */
 
-        r = lookup_paths_init(&paths, SYSTEMD_SYSTEM, false, NULL, NULL, NULL);
+        r = lookup_paths_init(&paths, SYSTEMD_SYSTEM, false, arg_root, NULL, NULL, NULL);
         if (r < 0)
                 return r;