cgroup: rework how we validate/escape cgroups

Let's clean up validation/escaping of cgroup names. i.e. split out code
that tests if name needs escaping. Return proper error codes, and extend
test a bit.

6 files changed, 104 insertions, 76 deletions

diff --git a/src/basic/cgroup-util.c b/src/basic/cgroup-util.c
index 90877c9fa1..21e2255dae 100644
--- a/src/basic/cgroup-util.c
+++ b/src/basic/cgroup-util.c
@@ -1554,52 +1554,64 @@ int cg_pid_get_user_slice(pid_t pid, char **slice) {
         return cg_path_get_user_slice(cgroup, slice);
 }
 
-char *cg_escape(const char *p) {
-        bool need_prefix = false;
-
-        /* This implements very minimal escaping for names to be used
-         * as file names in the cgroup tree: any name which might
-         * conflict with a kernel name or is prefixed with '_' is
-         * prefixed with a '_'. That way, when reading cgroup names it
-         * is sufficient to remove a single prefixing underscore if
-         * there is one. */
-
-        /* The return value of this function (unlike cg_unescape())
-         * needs free()! */
-
-        if (IN_SET(p[0], 0, '_', '.') ||
-            STR_IN_SET(p, "notify_on_release", "release_agent", "tasks") ||
-            startswith(p, "cgroup."))
-                need_prefix = true;
-        else {
-                const char *dot;
+bool cg_needs_escape(const char *p) {
 
-                dot = strrchr(p, '.');
-                if (dot) {
-                        CGroupController c;
-                        size_t l = dot - p;
+        /* Checks if the specified path is a valid cgroup name by our rules, or if it must be escaped. Note
+         * that we consider escaped cgroup names invalid here, as they need to be escaped a second time if
+         * they shall be used. Also note that various names cannot be made valid by escaping even if we
+         * return true here (because too long, or contain the forbidden character "/"). */
 
-                        for (c = 0; c < _CGROUP_CONTROLLER_MAX; c++) {
-                                const char *n;
+        if (!filename_is_valid(p))
+                return true;
 
-                                n = cgroup_controller_to_string(c);
+        if (IN_SET(p[0], '_', '.'))
+                return true;
 
-                                if (l != strlen(n))
-                                        continue;
+        if (STR_IN_SET(p, "notify_on_release", "release_agent", "tasks"))
+                return true;
 
-                                if (memcmp(p, n, l) != 0)
-                                        continue;
+        if (startswith(p, "cgroup."))
+                return true;
 
-                                need_prefix = true;
-                                break;
-                        }
-                }
+        for (CGroupController c = 0; c < _CGROUP_CONTROLLER_MAX; c++) {
+                const char *q;
+
+                q = startswith(p, cgroup_controller_to_string(c));
+                if (!q)
+                        continue;
+
+                if (q[0] == '.')
+                        return true;
         }
 
-        if (need_prefix)
-                return strjoin("_", p);
+        return false;
+}
+
+int cg_escape(const char *p, char **ret) {
+        _cleanup_free_ char *n = NULL;
+
+        /* This implements very minimal escaping for names to be used as file names in the cgroup tree: any
+         * name which might conflict with a kernel name or is prefixed with '_' is prefixed with a '_'. That
+         * way, when reading cgroup names it is sufficient to remove a single prefixing underscore if there
+         * is one. */
+
+        /* The return value of this function (unlike cg_unescape()) needs free()! */
 
-        return strdup(p);
+        if (cg_needs_escape(p)) {
+                n = strjoin("_", p);
+                if (!n)
+                        return -ENOMEM;
+
+                if (!filename_is_valid(n)) /* became invalid due to the prefixing? Or contained things like a slash that cannot be fixed by prefixing? */
+                        return -EINVAL;
+        } else {
+                n = strdup(p);
+                if (!n)
+                        return -ENOMEM;
+        }
+
+        *ret = TAKE_PTR(n);
+        return 0;
 }
 
 char *cg_unescape(const char *p) {
@@ -1698,9 +1710,9 @@ int cg_slice_to_path(const char *unit, char **ret) {
                 if (!unit_name_is_valid(n, UNIT_NAME_PLAIN))
                         return -EINVAL;
 
-                escaped = cg_escape(n);
-                if (!escaped)
-                        return -ENOMEM;
+                r = cg_escape(n, &escaped);
+                if (r < 0)
+                        return r;
 
                 if (!strextend(&s, escaped, "/"))
                         return -ENOMEM;
@@ -1708,15 +1720,14 @@ int cg_slice_to_path(const char *unit, char **ret) {
                 dash = strchr(dash+1, '-');
         }
 
-        e = cg_escape(unit);
-        if (!e)
-                return -ENOMEM;
+        r = cg_escape(unit, &e);
+        if (r < 0)
+                return r;
 
         if (!strextend(&s, e))
                 return -ENOMEM;
 
         *ret = TAKE_PTR(s);
-
         return 0;
 }
 
diff --git a/src/basic/cgroup-util.h b/src/basic/cgroup-util.h
index 5de000c4ce..9b30ae0396 100644
--- a/src/basic/cgroup-util.h
+++ b/src/basic/cgroup-util.h
@@ -279,7 +279,8 @@ int cg_pid_get_user_slice(pid_t pid, char **slice);
 
 int cg_path_decode_unit(const char *cgroup, char **unit);
 
-char *cg_escape(const char *p);
+bool cg_needs_escape(const char *p);
+int cg_escape(const char *p, char **ret);
 char *cg_unescape(const char *p) _pure_;
 
 bool cg_controller_is_valid(const char *p);
diff --git a/src/core/cgroup.c b/src/core/cgroup.c
index 8b178ea29f..cd48183f7a 100644
--- a/src/core/cgroup.c
+++ b/src/core/cgroup.c
@@ -2087,28 +2087,37 @@ static const char *migrate_callback(CGroupMask mask, void *userdata) {
         return strempty(unit_get_realized_cgroup_path(userdata, mask));
 }
 
-char *unit_default_cgroup_path(const Unit *u) {
-        _cleanup_free_ char *escaped = NULL, *slice_path = NULL;
-        Unit *slice;
+int unit_default_cgroup_path(const Unit *u, char **ret) {
+        _cleanup_free_ char *p = NULL;
         int r;
 
         assert(u);
+        assert(ret);
 
         if (unit_has_name(u, SPECIAL_ROOT_SLICE))
-                return strdup(u->manager->cgroup_root);
+                p = strdup(u->manager->cgroup_root);
+        else {
+                _cleanup_free_ char *escaped = NULL, *slice_path = NULL;
+                Unit *slice;
 
-        slice = UNIT_GET_SLICE(u);
-        if (slice && !unit_has_name(slice, SPECIAL_ROOT_SLICE)) {
-                r = cg_slice_to_path(slice->id, &slice_path);
+                slice = UNIT_GET_SLICE(u);
+                if (slice && !unit_has_name(slice, SPECIAL_ROOT_SLICE)) {
+                        r = cg_slice_to_path(slice->id, &slice_path);
+                        if (r < 0)
+                                return r;
+                }
+
+                r = cg_escape(u->id, &escaped);
                 if (r < 0)
-                        return NULL;
-        }
+                        return r;
 
-        escaped = cg_escape(u->id);
-        if (!escaped)
-                return NULL;
+                p = path_join(empty_to_root(u->manager->cgroup_root), slice_path, escaped);
+        }
+        if (!p)
+                return -ENOMEM;
 
-        return path_join(empty_to_root(u->manager->cgroup_root), slice_path, escaped);
+        *ret = TAKE_PTR(p);
+        return 0;
 }
 
 int unit_set_cgroup_path(Unit *u, const char *path) {
@@ -2264,9 +2273,9 @@ int unit_pick_cgroup_path(Unit *u) {
         if (!UNIT_HAS_CGROUP_CONTEXT(u))
                 return -EINVAL;
 
-        path = unit_default_cgroup_path(u);
-        if (!path)
-                return log_oom();
+        r = unit_default_cgroup_path(u, &path);
+        if (r < 0)
+                return log_unit_error_errno(u, r, "Failed to generate default cgroup path: %m");
 
         r = unit_set_cgroup_path(u, path);
         if (r == -EEXIST)
diff --git a/src/core/cgroup.h b/src/core/cgroup.h
index 8e1f093901..d445ea1e8d 100644
--- a/src/core/cgroup.h
+++ b/src/core/cgroup.h
@@ -290,7 +290,7 @@ void unit_invalidate_cgroup_members_masks(Unit *u);
 void unit_add_family_to_cgroup_realize_queue(Unit *u);
 
 const char *unit_get_realized_cgroup_path(Unit *u, CGroupMask mask);
-char *unit_default_cgroup_path(const Unit *u);
+int unit_default_cgroup_path(const Unit *u, char **ret);
 int unit_set_cgroup_path(Unit *u, const char *path);
 int unit_pick_cgroup_path(Unit *u);
 
diff --git a/src/core/unit-printf.c b/src/core/unit-printf.c
index 1b267d4fdd..3977082cc1 100644
--- a/src/core/unit-printf.c
+++ b/src/core/unit-printf.c
@@ -86,19 +86,21 @@ static void bad_specifier(const Unit *u, char specifier) {
 
 static int specifier_cgroup(char specifier, const void *data, const char *root, const void *userdata, char **ret) {
         const Unit *u = ASSERT_PTR(userdata);
-        char *n;
 
         bad_specifier(u, specifier);
 
-        if (u->cgroup_path)
+        if (u->cgroup_path) {
+                char *n;
+
                 n = strdup(u->cgroup_path);
-        else
-                n = unit_default_cgroup_path(u);
-        if (!n)
-                return -ENOMEM;
+                if (!n)
+                        return -ENOMEM;
 
-        *ret = n;
-        return 0;
+                *ret = n;
+                return 0;
+        }
+
+        return unit_default_cgroup_path(u, ret);
 }
 
 static int specifier_cgroup_root(char specifier, const void *data, const char *root, const void *userdata, char **ret) {
@@ -126,7 +128,7 @@ static int specifier_cgroup_slice(char specifier, const void *data, const char *
                 if (slice->cgroup_path)
                         n = strdup(slice->cgroup_path);
                 else
-                        n = unit_default_cgroup_path(slice);
+                        return unit_default_cgroup_path(slice, ret);
         } else
                 n = strdup(u->manager->cgroup_root);
         if (!n)
diff --git a/src/test/test-cgroup-util.c b/src/test/test-cgroup-util.c
index cdf911926c..4f430a1df2 100644
--- a/src/test/test-cgroup-util.c
+++ b/src/test/test-cgroup-util.c
@@ -235,14 +235,19 @@ TEST(proc) {
         }
 }
 
-static void test_escape_one(const char *s, const char *r) {
-        _cleanup_free_ char *b;
+static void test_escape_one(const char *s, const char *expected) {
+        _cleanup_free_ char *b = NULL;
 
-        b = cg_escape(s);
-        assert_se(b);
-        assert_se(streq(b, r));
+        assert_se(s);
+        assert_se(expected);
+
+        assert_se(cg_escape(s, &b) >= 0);
+        assert_se(streq(b, expected));
 
         assert_se(streq(cg_unescape(b), s));
+
+        assert_se(filename_is_valid(b));
+        assert_se(!cg_needs_escape(s) || b[0] == '_');
 }
 
 TEST(escape, .sd_booted = true) {