core: do not create symlink to private directory if parent already exists

The very basic functinality of StateDirectory= or friends is creating
specified directories. That should work if one entry is a subdirectory
of another. However, it does not when combined with DynamicUser=yes.

To support such case, this adds ExecDirectoryItem.only_create flag, and
if it is set PID1 only create private directory, and not create the symlink
to the private directory.

Fixes #24783.

diff --git a/src/core/dbus-execute.c b/src/core/dbus-execute.c
index 57036936f9a2f441c53633e53897974646dec827..fd73c7bcb77f03e31741876f641be39ca3de7c50 100644 (file)
--- a/src/core/dbus-execute.c
+++ b/src/core/dbus-execute.c
@@ -3324,6 +3324,7 @@ int bus_exec_context_set_transient_property(
                                         if (r < 0)
                                                 return log_oom();
                                 }
+                                exec_directory_sort(d);
 
                                 joined = unit_concat_strv(l, UNIT_ESCAPE_SPECIFIERS);
                                 if (!joined)
@@ -3787,6 +3788,8 @@ int bus_exec_context_set_transient_property(
                 if (r < 0)
                         return r;
 
+                exec_directory_sort(directory);
+
                 r = sd_bus_message_exit_container(message);
                 if (r < 0)
                         return r;

diff --git a/src/core/execute.c b/src/core/execute.c
index 955e2cf628805960cb930adfde7319b46a726598..1b75b49426c3651cdfa555f0704b9105beea7cc4 100644 (file)
--- a/src/core/execute.c
+++ b/src/core/execute.c
@@ -91,6 +91,7 @@
 #include "signal-util.h"
 #include "smack-util.h"
 #include "socket-util.h"
+#include "sort-util.h"
 #include "special.h"
 #include "stat-util.h"
 #include "string-table.h"
@@ -2412,12 +2413,24 @@ static int setup_exec_directory(
                                         goto fail;
                         }
 
-                        /* And link it up from the original place. Note that if a mount namespace is going to be
-                         * used, then this symlink remains on the host, and a new one for the child namespace will
-                         * be created later. */
-                        r = symlink_idempotent(pp, p, true);
-                        if (r < 0)
-                                goto fail;
+                        if (!context->directories[type].items[i].only_create) {
+                                /* And link it up from the original place.
+                                 * Notes
+                                 * 1) If a mount namespace is going to be used, then this symlink remains on
+                                 *    the host, and a new one for the child namespace will be created later.
+                                 * 2) It is not necessary to create this symlink when one of its parent
+                                 *    directories is specified and already created. E.g.
+                                 *        StateDirectory=foo foo/bar
+                                 *    In that case, the inode points to pp and p for "foo/bar" are the same:
+                                 *        pp = "/var/lib/private/foo/bar"
+                                 *        p = "/var/lib/foo/bar"
+                                 *    and, /var/lib/foo is a symlink to /var/lib/private/foo. So, not only
+                                 *    we do not need to create the symlink, but we cannot create the symlink.
+                                 *    See issue #24783. */
+                                r = symlink_idempotent(pp, p, true);
+                                if (r < 0)
+                                        goto fail;
+                        }
 
                 } else {
                         _cleanup_free_ char *target = NULL;
@@ -3289,7 +3302,8 @@ static int compile_bind_mounts(
                 if (!params->prefix[t])
                         continue;
 
-                n += context->directories[t].n_items;
+                for (size_t i = 0; i < context->directories[t].n_items; i++)
+                        n += !context->directories[t].items[i].only_create;
         }
 
         if (n <= 0) {
@@ -3358,6 +3372,11 @@ static int compile_bind_mounts(
                 for (size_t i = 0; i < context->directories[t].n_items; i++) {
                         char *s, *d;
 
+                        /* When one of the parent directories is in the list, we cannot create the symlink
+                         * for the child directory. See also the comments in setup_exec_directory(). */
+                        if (context->directories[t].items[i].only_create)
+                                continue;
+
                         if (exec_directory_is_private(context, t))
                                 s = path_join(params->prefix[t], "private", context->directories[t].items[i].path);
                         else
@@ -3437,7 +3456,9 @@ static int compile_symlinks(
                                         return r;
                         }
 
-                        if (!exec_directory_is_private(context, dt) || exec_context_with_rootfs(context))
+                        if (!exec_directory_is_private(context, dt) ||
+                            exec_context_with_rootfs(context) ||
+                            context->directories[dt].items[i].only_create)
                                 continue;
 
                         private_path = path_join(params->prefix[dt], "private", context->directories[dt].items[i].path);
@@ -7090,6 +7111,34 @@ int exec_directory_add(ExecDirectory *d, const char *path, const char *symlink)
         return 1; /* new item is added */
 }
 
+static int exec_directory_item_compare_func(const ExecDirectoryItem *a, const ExecDirectoryItem *b) {
+        assert(a);
+        assert(b);
+
+        return path_compare(a->path, b->path);
+}
+
+void exec_directory_sort(ExecDirectory *d) {
+        assert(d);
+
+        /* Sort the exec directories to make always parent directories processed at first in
+         * setup_exec_directory(), e.g., even if StateDirectory=foo/bar foo, we need to create foo at first,
+         * then foo/bar. Also, set .only_create flag if one of the parent directories is contained in the
+         * list. See also comments in setup_exec_directory() and issue #24783. */
+
+        if (d->n_items <= 1)
+                return;
+
+        typesafe_qsort(d->items, d->n_items, exec_directory_item_compare_func);
+
+        for (size_t i = 1; i < d->n_items; i++)
+                for (size_t j = 0; j < i; j++)
+                        if (path_startswith(d->items[i].path, d->items[j].path)) {
+                                d->items[i].only_create = true;
+                                break;
+                        }
+}
+
 DEFINE_HASH_OPS_WITH_VALUE_DESTRUCTOR(exec_set_credential_hash_ops, char, string_hash_func, string_compare_func, ExecSetCredential, exec_set_credential_free);
 DEFINE_HASH_OPS_WITH_VALUE_DESTRUCTOR(exec_load_credential_hash_ops, char, string_hash_func, string_compare_func, ExecLoadCredential, exec_load_credential_free);
 

diff --git a/src/core/execute.h b/src/core/execute.h
index fc6d36aa00d3c594be1bc09af8fd22b68a317a07..a2cf22806b2b8df40045760c24a8ccf8665f76ab 100644 (file)
--- a/src/core/execute.h
+++ b/src/core/execute.h
@@ -135,6 +135,7 @@ typedef enum ExecDirectoryType {
 typedef struct ExecDirectoryItem {
         char *path;
         char **symlinks;
+        bool only_create;
 } ExecDirectoryItem;
 
 typedef struct ExecDirectory {
@@ -493,6 +494,7 @@ DEFINE_TRIVIAL_CLEANUP_FUNC(ExecLoadCredential*, exec_load_credential_free);
 
 void exec_directory_done(ExecDirectory *d);
 int exec_directory_add(ExecDirectory *d, const char *path, const char *symlink);
+void exec_directory_sort(ExecDirectory *d);
 
 extern const struct hash_ops exec_set_credential_hash_ops;
 extern const struct hash_ops exec_load_credential_hash_ops;

diff --git a/src/core/unit.c b/src/core/unit.c
index d181d03b7a0a7dd434be92a1a7e3b686221b0546..d6bea2080f089f329eee16c06d44e64b7827401f 100644 (file)
--- a/src/core/unit.c
+++ b/src/core/unit.c
@@ -4131,6 +4131,9 @@ int unit_patch_contexts(Unit *u) {
                         ec->no_new_privileges = true;
                         ec->restrict_suid_sgid = true;
                 }
+
+                for (ExecDirectoryType dt = 0; dt < _EXEC_DIRECTORY_TYPE_MAX; dt++)
+                        exec_directory_sort(ec->directories + dt);
         }
 
         cc = unit_get_cgroup_context(u);