From 8dcb891c199aeb22fe0fe3e8d1f830c0de8a0c24 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 26 Jan 2021 12:28:23 +0100 Subject: [PATCH 1/4] path-util: add path_extract_directory(), to match path_extract_filename() These two together are a lot like dirname() + basename() but have the benefit that they return clear errors when one passes a special case path to them where the extraction doesn't make sense, i.e. "", "/", "foo", "foo/" and so on. Sooner or later we should probably port all our uses of dirname()/basename() over to this, to catch these special cases more safely. --- src/basic/path-util.c | 42 ++++++++++++++++++++++++++++ src/basic/path-util.h | 1 + src/test/test-path-util.c | 59 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 102 insertions(+) diff --git a/src/basic/path-util.c b/src/basic/path-util.c index f3398418c4..fe8321edce 100644 --- a/src/basic/path-util.c +++ b/src/basic/path-util.c @@ -846,6 +846,48 @@ int path_extract_filename(const char *p, char **ret) { return 0; } +int path_extract_directory(const char *p, char **ret) { + _cleanup_free_ char *a = NULL; + const char *c; + + /* The inverse of path_extract_filename(), i.e. returns the directory path prefix. Returns: + * + * -EINVAL → if the passed in path is not a valid path + * -EDESTADDRREQ → if no directory was specified in the passed in path, i.e. only a filename was passed + * -EADDRNOTAVAIL → if the passed in parameter had no filename but did have a directory, i.e. the root dir itself was specified + * -ENOMEM → no memory (surprise!) + * + * This function guarantees to return a fully valid path, i.e. one that passes path_is_valid(). + */ + + if (!path_is_valid(p)) + return -EINVAL; + + /* Special case the root dir, because otherwise for an input of "///" last_path_component() returns + * the pointer to the last slash only, which might be seen as a valid path below. */ + if (path_equal(p, "/")) + return -EADDRNOTAVAIL; + + c = last_path_component(p); + + /* Delete trailing slashes, but keep one */ + while (c > p+1 && c[-1] == '/') + c--; + + if (p == c) /* No path whatsoever? Then return a recognizable error */ + return -EDESTADDRREQ; + + a = strndup(p, c - p); + if (!a) + return -ENOMEM; + + if (!path_is_valid(a)) + return -EINVAL; + + *ret = TAKE_PTR(a); + return 0; +} + bool filename_is_valid(const char *p) { const char *e; diff --git a/src/basic/path-util.h b/src/basic/path-util.h index ba12b03dbe..74ee6362ea 100644 --- a/src/basic/path-util.h +++ b/src/basic/path-util.h @@ -147,6 +147,7 @@ int fsck_exists(const char *fstype); char* dirname_malloc(const char *path); const char *last_path_component(const char *path); int path_extract_filename(const char *p, char **ret); +int path_extract_directory(const char *p, char **ret); bool filename_is_valid(const char *p) _pure_; bool path_is_valid(const char *p) _pure_; diff --git a/src/test/test-path-util.c b/src/test/test-path-util.c index 59308473c1..db6c1a9efa 100644 --- a/src/test/test-path-util.c +++ b/src/test/test-path-util.c @@ -606,6 +606,64 @@ static void test_path_extract_filename(void) { test_path_extract_filename_one("./", NULL, -EINVAL); } +static void test_path_extract_directory_one(const char *input, const char *output, int ret) { + _cleanup_free_ char *k = NULL; + int r; + + r = path_extract_directory(input, &k); + log_info_errno(r, "%s → %s/%m [expected: %s/%s]", + strnull(input), + strnull(k), /* we output strerror_safe(r) via %m here, since otherwise the error buffer might be overwritten twice */ + strnull(output), strerror_safe(ret)); + assert_se(streq_ptr(k, output)); + assert_se(r == ret); + + /* Extra safety check: let's make sure that if we split out the filename too (and it works) the + * joined parts are identical to the original again */ + if (r >= 0) { + _cleanup_free_ char *f = NULL; + + r = path_extract_filename(input, &f); + if (r >= 0) { + _cleanup_free_ char *j = NULL; + + assert_se(j = path_join(k, f)); + assert_se(path_equal(input, j)); + } + } +} + +static void test_path_extract_directory(void) { + log_info("/* %s */", __func__); + + test_path_extract_directory_one(NULL, NULL, -EINVAL); + test_path_extract_directory_one("a/b/c", "a/b", 0); + test_path_extract_directory_one("a/b/c/", "a/b", 0); + test_path_extract_directory_one("/", NULL, -EADDRNOTAVAIL); + test_path_extract_directory_one("//", NULL, -EADDRNOTAVAIL); + test_path_extract_directory_one("///", NULL, -EADDRNOTAVAIL); + test_path_extract_directory_one(".", NULL, -EDESTADDRREQ); + test_path_extract_directory_one("./.", ".", 0); + test_path_extract_directory_one("././", ".", 0); + test_path_extract_directory_one("././/", ".", 0); + test_path_extract_directory_one("/foo/a", "/foo", 0); + test_path_extract_directory_one("/foo/a/", "/foo", 0); + test_path_extract_directory_one("", NULL, -EINVAL); + test_path_extract_directory_one("a", NULL, -EDESTADDRREQ); + test_path_extract_directory_one("a/", NULL, -EDESTADDRREQ); + test_path_extract_directory_one("/a", "/", 0); + test_path_extract_directory_one("/a/", "/", 0); + test_path_extract_directory_one("/////////////a/////////////", "/", 0); + test_path_extract_directory_one("xx/.", "xx", 0); + test_path_extract_directory_one("xx/..", "xx", 0); + test_path_extract_directory_one("..", NULL, -EDESTADDRREQ); + test_path_extract_directory_one("/..", "/", 0); + test_path_extract_directory_one("../", NULL, -EDESTADDRREQ); + test_path_extract_directory_one(".", NULL, -EDESTADDRREQ); + test_path_extract_directory_one("/.", "/", 0); + test_path_extract_directory_one("./", NULL, -EDESTADDRREQ); +} + static void test_filename_is_valid(void) { char foo[NAME_MAX+2]; @@ -793,6 +851,7 @@ int main(int argc, char **argv) { test_file_in_same_dir(); test_last_path_component(); test_path_extract_filename(); + test_path_extract_directory(); test_filename_is_valid(); test_path_is_valid(); test_hidden_or_backup_file(); From ee277c6bc750b6398546fd030208e720ee155012 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 23 Feb 2021 16:49:29 +0100 Subject: [PATCH 2/4] path-util: return O_DIRECTORY from path_extract_filename() when path ends in slash Let's fine-tune the path_extract_filename() interface: on succes return O_DIRECTORY as indicator that the input path was slash-suffixed, and regular 0 otherwise. This is useful since in many cases it is useful to filter out paths that must refer to dirs early on. I opted for O_DIRECTORY instead of the following other ideas: 1. return -EISDIR: I think the function should return an extracted filename even when referring to an obvious dir, so this is not an option. 2. S_ISDIR, this was a strong contender, but I think O_DIRECTORY is a tiny bit nicer since quite likely we will go on and open the thing, maybe with openat(), and hence it's quite nice to be able to OR in the return value into the flags argument of openat(). 3. A new enum defined with two values "dont-know" and "definitely-directory". But I figured this was unnecessary, given we have other options too, that reuse existing definitions for very similar purposes. --- src/basic/path-util.c | 21 ++++++++++++++++----- src/test/test-path-util.c | 15 +++++++++------ 2 files changed, 25 insertions(+), 11 deletions(-) diff --git a/src/basic/path-util.c b/src/basic/path-util.c index fe8321edce..50ba44492e 100644 --- a/src/basic/path-util.c +++ b/src/basic/path-util.c @@ -819,11 +819,21 @@ const char *last_path_component(const char *path) { int path_extract_filename(const char *p, char **ret) { _cleanup_free_ char *a = NULL; const char *c; + size_t n; /* Extracts the filename part (i.e. right-most component) from a path, i.e. string that passes - * filename_is_valid(). A wrapper around last_path_component(), but eats up trailing slashes. Returns - * -EADDRNOTAVAIL if specified parameter includes no filename (i.e. is "/" or so). Returns -EINVAL if - * not a valid path in the first place. */ + * filename_is_valid(). A wrapper around last_path_component(), but eats up trailing + * slashes. Returns: + * + * -EINVAL → if the passed in path is not a valid path + * -EADDRNOTAVAIL → if only a directory was specified, but no filename, i.e. the root dir itself is specified + * -ENOMEM → no memory + * + * Returns >= 0 on success. If the input path has a trailing slash, returns O_DIRECTORY, to indicate + * the referenced file must be a directory. + * + * This function guarantees to return a fully valid filename, i.e. one that passes + * filename_is_valid() – this means "." and ".." are not accepted. */ if (!path_is_valid(p)) return -EINVAL; @@ -834,8 +844,9 @@ int path_extract_filename(const char *p, char **ret) { return -EADDRNOTAVAIL; c = last_path_component(p); + n = strcspn(c, "/"); - a = strndup(c, strcspn(c, "/")); + a = strndup(c, n); if (!a) return -ENOMEM; @@ -843,7 +854,7 @@ int path_extract_filename(const char *p, char **ret) { return -EINVAL; *ret = TAKE_PTR(a); - return 0; + return c[n] == '/' ? O_DIRECTORY : 0; } int path_extract_directory(const char *p, char **ret) { diff --git a/src/test/test-path-util.c b/src/test/test-path-util.c index db6c1a9efa..b92a77b9f4 100644 --- a/src/test/test-path-util.c +++ b/src/test/test-path-util.c @@ -570,7 +570,10 @@ static void test_path_extract_filename_one(const char *input, const char *output int r; r = path_extract_filename(input, &k); - log_info("%s → %s/%s [expected: %s/%s]", strnull(input), strnull(k), strerror_safe(r), strnull(output), strerror_safe(ret)); + log_info_errno(r, "%s → %s/%m [expected: %s/%s]", + strnull(input), + strnull(k), /* strerror(r) is printed via %m, to avoid that the two strerror()'s overwrite each other's buffers */ + strnull(output), ret < 0 ? strerror_safe(ret) : "-"); assert_se(streq_ptr(k, output)); assert_se(r == ret); } @@ -580,7 +583,7 @@ static void test_path_extract_filename(void) { test_path_extract_filename_one(NULL, NULL, -EINVAL); test_path_extract_filename_one("a/b/c", "c", 0); - test_path_extract_filename_one("a/b/c/", "c", 0); + test_path_extract_filename_one("a/b/c/", "c", O_DIRECTORY); test_path_extract_filename_one("/", NULL, -EADDRNOTAVAIL); test_path_extract_filename_one("//", NULL, -EADDRNOTAVAIL); test_path_extract_filename_one("///", NULL, -EADDRNOTAVAIL); @@ -589,13 +592,13 @@ static void test_path_extract_filename(void) { test_path_extract_filename_one("././", NULL, -EINVAL); test_path_extract_filename_one("././/", NULL, -EINVAL); test_path_extract_filename_one("/foo/a", "a", 0); - test_path_extract_filename_one("/foo/a/", "a", 0); + test_path_extract_filename_one("/foo/a/", "a", O_DIRECTORY); test_path_extract_filename_one("", NULL, -EINVAL); test_path_extract_filename_one("a", "a", 0); - test_path_extract_filename_one("a/", "a", 0); + test_path_extract_filename_one("a/", "a", O_DIRECTORY); test_path_extract_filename_one("/a", "a", 0); - test_path_extract_filename_one("/a/", "a", 0); - test_path_extract_filename_one("/////////////a/////////////", "a", 0); + test_path_extract_filename_one("/a/", "a", O_DIRECTORY); + test_path_extract_filename_one("/////////////a/////////////", "a", O_DIRECTORY); test_path_extract_filename_one("xx/.", NULL, -EINVAL); test_path_extract_filename_one("xx/..", NULL, -EINVAL); test_path_extract_filename_one("..", NULL, -EINVAL); From 7fc607637f2e27a47a44d20a81658b1b9fb52a95 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 23 Feb 2021 17:22:31 +0100 Subject: [PATCH 3/4] machinectl: make sure of path_extract_filename() returning O_DIRECTORY --- src/machine/machinectl.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/machine/machinectl.c b/src/machine/machinectl.c index 1ae10e5880..e5befe50e1 100644 --- a/src/machine/machinectl.c +++ b/src/machine/machinectl.c @@ -1872,6 +1872,9 @@ static int import_tar(int argc, char *argv[], void *userdata) { r = path_extract_filename(path, &fn); if (r < 0) return log_error_errno(r, "Cannot extract container name from filename: %m"); + if (r == O_DIRECTORY) + return log_error_errno(SYNTHETIC_ERRNO(EISDIR), + "Path '%s' refers to directory, but we need a regular file: %m", path); local = fn; } @@ -1932,6 +1935,9 @@ static int import_raw(int argc, char *argv[], void *userdata) { r = path_extract_filename(path, &fn); if (r < 0) return log_error_errno(r, "Cannot extract container name from filename: %m"); + if (r == O_DIRECTORY) + return log_error_errno(SYNTHETIC_ERRNO(EISDIR), + "Path '%s' refers to directory, but we need a regular file: %m", path); local = fn; } From 62a88d7a3ef43fead338aca135467786d60ac9b7 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 26 Feb 2021 18:24:58 +0100 Subject: [PATCH 4/4] tmpfile: port tempfn_*() to path_extract_*() --- src/basic/tmpfile-util.c | 83 +++++++++++++++------------- src/test/meson.build | 2 + src/test/test-tmpfile-util.c | 102 +++++++++++++++++++++++++++++++++++ 3 files changed, 150 insertions(+), 37 deletions(-) create mode 100644 src/test/test-tmpfile-util.c diff --git a/src/basic/tmpfile-util.c b/src/basic/tmpfile-util.c index bac5eb6b26..5ee71d0158 100644 --- a/src/basic/tmpfile-util.c +++ b/src/basic/tmpfile-util.c @@ -94,16 +94,11 @@ int fmkostemp_safe(char *pattern, const char *mode, FILE **ret_f) { } int tempfn_xxxxxx(const char *p, const char *extra, char **ret) { - const char *fn; - char *t; + _cleanup_free_ char *d = NULL, *fn = NULL, *nf = NULL; + int r; assert(ret); - if (isempty(p)) - return -EINVAL; - if (path_equal(p, "/")) - return -EINVAL; - /* * Turns this: * /foo/bar/waldo @@ -112,34 +107,41 @@ int tempfn_xxxxxx(const char *p, const char *extra, char **ret) { * /foo/bar/.#waldoXXXXXX */ - fn = basename(p); - if (!filename_is_valid(fn)) - return -EINVAL; + r = path_extract_directory(p, &d); + if (r < 0 && r != -EDESTADDRREQ) /* EDESTADDRREQ → No directory specified, just a filename */ + return r; - extra = strempty(extra); + r = path_extract_filename(p, &fn); + if (r < 0) + return r; - t = new(char, strlen(p) + 2 + strlen(extra) + 6 + 1); - if (!t) + nf = strjoin(".#", strempty(extra), fn, "XXXXXX"); + if (!nf) return -ENOMEM; - strcpy(stpcpy(stpcpy(stpcpy(mempcpy(t, p, fn - p), ".#"), extra), fn), "XXXXXX"); + if (!filename_is_valid(nf)) /* New name is not valid? (Maybe because too long?) Refuse. */ + return -EINVAL; + + if (d) { + char *j; + + j = path_join(d, nf); + if (!j) + return -ENOMEM; + + *ret = path_simplify(j, false); + } else + *ret = TAKE_PTR(nf); - *ret = path_simplify(t, false); return 0; } int tempfn_random(const char *p, const char *extra, char **ret) { - const char *fn; - char *t, *x; - uint64_t u; + _cleanup_free_ char *d = NULL, *fn = NULL, *nf = NULL; + int r; assert(ret); - if (isempty(p)) - return -EINVAL; - if (path_equal(p, "/")) - return -EINVAL; - /* * Turns this: * /foo/bar/waldo @@ -148,27 +150,34 @@ int tempfn_random(const char *p, const char *extra, char **ret) { * /foo/bar/.#waldobaa2a261115984a9 */ - fn = basename(p); - if (!filename_is_valid(fn)) - return -EINVAL; + r = path_extract_directory(p, &d); + if (r < 0 && r != -EDESTADDRREQ) /* EDESTADDRREQ → No directory specified, just a filename */ + return r; - extra = strempty(extra); + r = path_extract_filename(p, &fn); + if (r < 0) + return r; - t = new(char, strlen(p) + 2 + strlen(extra) + 16 + 1); - if (!t) + if (asprintf(&nf, ".#%s%s%016" PRIx64, + strempty(extra), + fn, + random_u64()) < 0) return -ENOMEM; - x = stpcpy(stpcpy(stpcpy(mempcpy(t, p, fn - p), ".#"), extra), fn); + if (!filename_is_valid(nf)) /* Not valid? (maybe because too long now?) — refuse early */ + return -EINVAL; - u = random_u64(); - for (unsigned i = 0; i < 16; i++) { - *(x++) = hexchar(u & 0xF); - u >>= 4; - } + if (d) { + char *j; - *x = 0; + j = path_join(d, nf); + if (!j) + return -ENOMEM; + + *ret = path_simplify(j, false); + } else + *ret = TAKE_PTR(nf); - *ret = path_simplify(t, false); return 0; } diff --git a/src/test/meson.build b/src/test/meson.build index c567a4cfcb..62e31fd830 100644 --- a/src/test/meson.build +++ b/src/test/meson.build @@ -379,6 +379,8 @@ tests += [ [['src/test/test-clock.c']], + [['src/test/test-tmpfile-util.c']], + [['src/test/test-architecture.c']], [['src/test/test-log.c']], diff --git a/src/test/test-tmpfile-util.c b/src/test/test-tmpfile-util.c new file mode 100644 index 0000000000..83bac15d00 --- /dev/null +++ b/src/test/test-tmpfile-util.c @@ -0,0 +1,102 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ + +#include "alloc-util.h" +#include "errno-util.h" +#include "log.h" +#include "string-util.h" +#include "tests.h" +#include "tmpfile-util.h" + +static void test_tempfn_random_one(const char *p, const char *extra, const char *expect, int ret) { + _cleanup_free_ char *s = NULL; + int r; + + r = tempfn_random(p, extra, &s); + log_info_errno(r, "%s+%s → %s vs. %s (%i/%m vs. %i/%s)", p, strna(extra), strna(s), strna(expect), r, ret, strerror_safe(ret)); + + assert(!s == !expect); + if (s) { + const char *suffix; + + assert_se(suffix = startswith(s, expect)); + assert_se(in_charset(suffix, HEXDIGITS)); + assert_se(strlen(suffix) == 16); + } + assert(ret == r); +} + +static void test_tempfn_random(void) { + test_tempfn_random_one("", NULL, NULL, -EINVAL); + test_tempfn_random_one(".", NULL, NULL, -EINVAL); + test_tempfn_random_one("..", NULL, NULL, -EINVAL); + test_tempfn_random_one("/", NULL, NULL, -EADDRNOTAVAIL); + + test_tempfn_random_one("foo", NULL, ".#foo", 0); + test_tempfn_random_one("foo", "bar", ".#barfoo", 0); + test_tempfn_random_one("/tmp/foo", NULL, "/tmp/.#foo", 0); + test_tempfn_random_one("/tmp/foo", "bar", "/tmp/.#barfoo", 0); + test_tempfn_random_one("./foo", NULL, "./.#foo", 0); + test_tempfn_random_one("./foo", "bar", "./.#barfoo", 0); + test_tempfn_random_one("../foo", NULL, "../.#foo", 0); + test_tempfn_random_one("../foo", "bar", "../.#barfoo", 0); + + test_tempfn_random_one("foo/", NULL, ".#foo", 0); + test_tempfn_random_one("foo/", "bar", ".#barfoo", 0); + test_tempfn_random_one("/tmp/foo/", NULL, "/tmp/.#foo", 0); + test_tempfn_random_one("/tmp/foo/", "bar", "/tmp/.#barfoo", 0); + test_tempfn_random_one("./foo/", NULL, "./.#foo", 0); + test_tempfn_random_one("./foo/", "bar", "./.#barfoo", 0); + test_tempfn_random_one("../foo/", NULL, "../.#foo", 0); + test_tempfn_random_one("../foo/", "bar", "../.#barfoo", 0); +} + +static void test_tempfn_xxxxxx_one(const char *p, const char *extra, const char *expect, int ret) { + _cleanup_free_ char *s = NULL; + int r; + + r = tempfn_xxxxxx(p, extra, &s); + log_info_errno(r, "%s+%s → %s vs. %s (%i/%m vs. %i/%s)", p, strna(extra), strna(s), strna(expect), r, ret, strerror_safe(ret)); + + assert(!s == !expect); + if (s) { + const char *suffix; + + assert_se(suffix = startswith(s, expect)); + assert_se(streq(suffix, "XXXXXX")); + } + assert(ret == r); +} + +static void test_tempfn_xxxxxx(void) { + test_tempfn_xxxxxx_one("", NULL, NULL, -EINVAL); + test_tempfn_xxxxxx_one(".", NULL, NULL, -EINVAL); + test_tempfn_xxxxxx_one("..", NULL, NULL, -EINVAL); + test_tempfn_xxxxxx_one("/", NULL, NULL, -EADDRNOTAVAIL); + + test_tempfn_xxxxxx_one("foo", NULL, ".#foo", 0); + test_tempfn_xxxxxx_one("foo", "bar", ".#barfoo", 0); + test_tempfn_xxxxxx_one("/tmp/foo", NULL, "/tmp/.#foo", 0); + test_tempfn_xxxxxx_one("/tmp/foo", "bar", "/tmp/.#barfoo", 0); + test_tempfn_xxxxxx_one("./foo", NULL, "./.#foo", 0); + test_tempfn_xxxxxx_one("./foo", "bar", "./.#barfoo", 0); + test_tempfn_xxxxxx_one("../foo", NULL, "../.#foo", 0); + test_tempfn_xxxxxx_one("../foo", "bar", "../.#barfoo", 0); + + test_tempfn_xxxxxx_one("foo/", NULL, ".#foo", 0); + test_tempfn_xxxxxx_one("foo/", "bar", ".#barfoo", 0); + test_tempfn_xxxxxx_one("/tmp/foo/", NULL, "/tmp/.#foo", 0); + test_tempfn_xxxxxx_one("/tmp/foo/", "bar", "/tmp/.#barfoo", 0); + test_tempfn_xxxxxx_one("./foo/", NULL, "./.#foo", 0); + test_tempfn_xxxxxx_one("./foo/", "bar", "./.#barfoo", 0); + test_tempfn_xxxxxx_one("../foo/", NULL, "../.#foo", 0); + test_tempfn_xxxxxx_one("../foo/", "bar", "../.#barfoo", 0); +} + +int main(int argc, char **argv) { + test_setup_logging(LOG_DEBUG); + + test_tempfn_random(); + test_tempfn_xxxxxx(); + + return 0; +}