Rework unit_name_mangle_with_suffix() to (very slightly) simplify the path

'systemctl status /../dev' now looks for 'dev.mount', not '-..-dev.service',
and 'systemctl status /../foo' looks for 'foo.mount', not '-..-foo.service'. I
think this much more useful. I think the escaping is not very useful, so I plan
to submit a later series which changes that behaviour. But I think this first
step here is already useful on its own.

Note that the patch is smaller than it seems: before, is_device_path() would
return true only for absolute paths, so moving of is_device_path() under the
path_is_absolute() conditional doesn't influence the logic.

diff --git a/man/systemd-escape.xml b/man/systemd-escape.xml
index 6a5f31a2e3a18903f9fc808a0abca6246ac75ee8..1d351c7f9bdbc7550ce007332204325e34da721a 100644 (file)
--- a/man/systemd-escape.xml
+++ b/man/systemd-escape.xml
@@ -90,10 +90,14 @@
         <term><option>--path</option></term>
         <term><option>-p</option></term>
 
-        <listitem><para>When escaping or unescaping a string, assume it refers to a file system path. This eliminates
-        leading, trailing or duplicate <literal>/</literal> characters and rejects <literal>.</literal> and
-        <literal>..</literal> path components. This is particularly useful for generating strings suitable for
-        unescaping with the <literal>%f</literal> specifier in unit files, see
+        <listitem><para>When escaping or unescaping a string, assume it refers to a file system path. This
+        simplifies the path (leading, trailing, and duplicate <literal>/</literal> characters are removed,
+        no-op path <literal>.</literal> components are removed, and for absolute paths, leading
+        <literal>..</literal> components are removed). After the simplification, the path must not contain
+        <literal>..</literal>.</para>
+
+        <para>This is particularly useful for generating strings suitable for unescaping with the
+        <literal>%f</literal> specifier in unit files, see
         <citerefentry><refentrytitle>systemd.unit</refentrytitle><manvolnum>5</manvolnum></citerefentry>.
         </para>
 

diff --git a/src/basic/unit-name.c b/src/basic/unit-name.c
index 06877227e424fb740e1f3191783f3e393102fbce..8bf28ba749c343385bbc7bd8979d87089c88f102 100644 (file)
--- a/src/basic/unit-name.c
+++ b/src/basic/unit-name.c
@@ -708,7 +708,7 @@ int unit_name_mangle_with_suffix(
                 char **ret) {
 
         _cleanup_free_ char *s = NULL;
-        bool mangled, suggest_escape = true;
+        bool mangled, suggest_escape = true, warn = flags & UNIT_NAME_MANGLE_WARN;
         int r;
 
         assert(name);
@@ -729,22 +729,28 @@ int unit_name_mangle_with_suffix(
         if (string_is_glob(name) && in_charset(name, VALID_CHARS_GLOB)) {
                 if (flags & UNIT_NAME_MANGLE_GLOB)
                         goto good;
-                log_full(flags & UNIT_NAME_MANGLE_WARN ? LOG_NOTICE : LOG_DEBUG,
+                log_full(warn ? LOG_NOTICE : LOG_DEBUG,
                          "Glob pattern passed%s%s, but globs are not supported for this.",
                          operation ? " " : "", strempty(operation));
                 suggest_escape = false;
         }
 
-        if (is_device_path(name)) {
-                r = unit_name_from_path(name, ".device", ret);
-                if (r >= 0)
-                        return 1;
-                if (r != -EINVAL)
+        if (path_is_absolute(name)) {
+                _cleanup_free_ char *n = NULL;
+
+                r = path_simplify_alloc(name, &n);
+                if (r < 0)
                         return r;
-        }
 
-        if (path_is_absolute(name)) {
-                r = unit_name_from_path(name, ".mount", ret);
+                if (is_device_path(n)) {
+                        r = unit_name_from_path(n, ".device", ret);
+                        if (r >= 0)
+                                return 1;
+                        if (r != -EINVAL)
+                                return r;
+                }
+
+                r = unit_name_from_path(n, ".mount", ret);
                 if (r >= 0)
                         return 1;
                 if (r != -EINVAL)
@@ -757,7 +763,7 @@ int unit_name_mangle_with_suffix(
 
         mangled = do_escape_mangle(name, flags & UNIT_NAME_MANGLE_GLOB, s);
         if (mangled)
-                log_full(flags & UNIT_NAME_MANGLE_WARN ? LOG_NOTICE : LOG_DEBUG,
+                log_full(warn ? LOG_NOTICE : LOG_DEBUG,
                          "Invalid unit name \"%s\" escaped as \"%s\"%s.",
                          name, s,
                          suggest_escape ? " (maybe you should use systemd-escape?)" : "");

diff --git a/src/test/test-unit-name.c b/src/test/test-unit-name.c
index ada8330408c47cbc9cfa04aa95c34bb066b4c0fd..8e9332c33e52460681d2021a19d6541edf7a197f 100644 (file)
--- a/src/test/test-unit-name.c
+++ b/src/test/test-unit-name.c
@@ -201,8 +201,12 @@ TEST(unit_name_to_path) {
 
 static void test_unit_name_mangle_one(bool allow_globs, const char *pattern, const char *expect, int ret) {
         _cleanup_free_ char *t = NULL;
+        int r;
+
+        r = unit_name_mangle(pattern, (allow_globs * UNIT_NAME_MANGLE_GLOB) | UNIT_NAME_MANGLE_WARN, &t);
+        log_debug("%s: %s -> %d, %s", __func__, pattern, r, strnull(t));
 
-        assert_se(unit_name_mangle(pattern, (allow_globs * UNIT_NAME_MANGLE_GLOB) | UNIT_NAME_MANGLE_WARN, &t) == ret);
+        assert_se(r == ret);
         puts(strna(t));
         assert_se(streq_ptr(t, expect));
 
@@ -234,6 +238,48 @@ TEST(unit_name_mangle) {
         test_unit_name_mangle_one(true, "ü*", "\\xc3\\xbc*", 1);
 }
 
+static void test_unit_name_mangle_with_suffix_one(const char *arg, int expected, const char *expected_name) {
+        _cleanup_free_ char *s = NULL;
+        int r;
+
+        r = unit_name_mangle_with_suffix(arg, NULL, 0, ".service", &s);
+        log_debug("%s: %s -> %d, %s", __func__, arg, r, strnull(s));
+
+        assert_se(r == expected);
+        assert_se(streq_ptr(s, expected_name));
+}
+
+TEST(unit_name_mangle_with_suffix) {
+        test_unit_name_mangle_with_suffix_one("", -EINVAL, NULL);
+
+        test_unit_name_mangle_with_suffix_one("/dev", 1, "dev.mount");
+        test_unit_name_mangle_with_suffix_one("/../dev", 1, "dev.mount");
+        test_unit_name_mangle_with_suffix_one("/../dev/.", 1, "dev.mount");
+        /* We don't skip the last '..', and it makes this an invalid device or mount name */
+        test_unit_name_mangle_with_suffix_one("/.././dev/..", 1, "-..-.-dev-...service");
+        test_unit_name_mangle_with_suffix_one("/.././dev", 1, "dev.mount");
+        test_unit_name_mangle_with_suffix_one("/./.././../dev/", 1, "dev.mount");
+
+        test_unit_name_mangle_with_suffix_one("/dev/sda", 1, "dev-sda.device");
+        test_unit_name_mangle_with_suffix_one("/dev/sda5", 1, "dev-sda5.device");
+
+        test_unit_name_mangle_with_suffix_one("/sys", 1, "sys.mount");
+        test_unit_name_mangle_with_suffix_one("/../sys", 1, "sys.mount");
+        test_unit_name_mangle_with_suffix_one("/../sys/.", 1, "sys.mount");
+        /* We don't skip the last '..', and it makes this an invalid device or mount name */
+        test_unit_name_mangle_with_suffix_one("/.././sys/..", 1, "-..-.-sys-...service");
+        test_unit_name_mangle_with_suffix_one("/.././sys", 1, "sys.mount");
+        test_unit_name_mangle_with_suffix_one("/./.././../sys/", 1, "sys.mount");
+
+        test_unit_name_mangle_with_suffix_one("/proc", 1, "proc.mount");
+        test_unit_name_mangle_with_suffix_one("/../proc", 1, "proc.mount");
+        test_unit_name_mangle_with_suffix_one("/../proc/.", 1, "proc.mount");
+        /* We don't skip the last '..', and it makes this an invalid device or mount name */
+        test_unit_name_mangle_with_suffix_one("/.././proc/..", 1, "-..-.-proc-...service");
+        test_unit_name_mangle_with_suffix_one("/.././proc", 1, "proc.mount");
+        test_unit_name_mangle_with_suffix_one("/./.././../proc/", 1, "proc.mount");
+}
+
 TEST_RET(unit_printf, .sd_booted = true) {
         _cleanup_free_ char
                 *architecture, *os_image_version, *boot_id = NULL, *os_build_id,

diff --git a/test/units/testsuite-74.escape.sh b/test/units/testsuite-74.escape.sh
index 584c7340b5f81109f3fc15068227ef747124f0e7..e398d4010b63f68a0388e7c967fe3ddfa3e4afe1 100755 (executable)
--- a/test/units/testsuite-74.escape.sh
+++ b/test/units/testsuite-74.escape.sh
@@ -65,13 +65,19 @@ assert_eq "$(systemd-escape --unescape --instance 'mount-my-stuff@-this-is-where
 assert_eq "$(systemd-escape --unescape --instance --path 'mount-my-stuff@this-is-where-my-stuff-is-\x20with\x20spaces\x20though\x20.service')" \
           '/this/is/where/my/stuff/is/ with spaces though '
 
-# --path
+# --path, reversible cases
 check_escape / '-' --path
 check_escape '/hello/world' 'hello-world' --path
 check_escape '/mnt/smb/おにぎり' \
              'mnt-smb-\xe3\x81\x8a\xe3\x81\xab\xe3\x81\x8e\xe3\x82\x8a' \
              --path
 
+# --path, non-reversible cases
+assert_eq "$(systemd-escape --path ///////////////)" '-'
+assert_eq "$(systemd-escape --path /..)" '-'
+assert_eq "$(systemd-escape --path /../.././../.././)" '-'
+assert_eq "$(systemd-escape --path /../.././../.././foo)" 'foo'
+
 # --mangle
 assert_eq "$(systemd-escape --mangle 'hello-world')" 'hello-world.service'
 assert_eq "$(systemd-escape --mangle '/mount/this')" 'mount-this.mount'
@@ -94,7 +100,7 @@ assert_eq "$(systemd-escape --mangle 'trailing-whitespace.mount ')" 'trailing-wh
 (! systemd-escape --instance 'hello@hello.service')
 (! systemd-escape --instance --template=hello@.service 'hello@hello.service')
 (! systemd-escape --unescape --instance --path 'mount-my-stuff@-this-is-where-my-stuff-is-\x20with\x20spaces\x20though\x20.service')
-(! systemd-escape --path '/../hello')
+(! systemd-escape --path '/../hello/..')
 (! systemd-escape --path '.')
 (! systemd-escape --path '..')
 (! systemd-escape --path "$(set +x; printf '%0.sa' {0..256})")