From 5342eb4633e4a913e0049946ea31cc665894cc10 Mon Sep 17 00:00:00 2001 From: =?utf8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sat, 16 Sep 2023 20:22:29 +0200 Subject: [PATCH] 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. --- man/systemd-escape.xml | 12 +++++--- src/basic/unit-name.c | 28 +++++++++++------- src/test/test-unit-name.c | 48 ++++++++++++++++++++++++++++++- test/units/testsuite-74.escape.sh | 10 +++++-- 4 files changed, 80 insertions(+), 18 deletions(-) diff --git a/man/systemd-escape.xml b/man/systemd-escape.xml index 6a5f31a2e3a..1d351c7f9bd 100644 --- a/man/systemd-escape.xml +++ b/man/systemd-escape.xml @@ -90,10 +90,14 @@ - When escaping or unescaping a string, assume it refers to a file system path. This eliminates - leading, trailing or duplicate / characters and rejects . and - .. path components. This is particularly useful for generating strings suitable for - unescaping with the %f specifier in unit files, see + When escaping or unescaping a string, assume it refers to a file system path. This + simplifies the path (leading, trailing, and duplicate / characters are removed, + no-op path . components are removed, and for absolute paths, leading + .. components are removed). After the simplification, the path must not contain + ... + + This is particularly useful for generating strings suitable for unescaping with the + %f specifier in unit files, see systemd.unit5. diff --git a/src/basic/unit-name.c b/src/basic/unit-name.c index 06877227e42..8bf28ba749c 100644 --- 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 ada8330408c..8e9332c33e5 100644 --- 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 584c7340b5f..e398d4010b6 100755 --- 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})") -- 2.39.5