From 00a050b395b6c38f6dea86fd660741bba00fadf0 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Thu, 20 Jul 2023 14:56:43 +0900 Subject: [PATCH 1/7] chase: reuse "done" to open fd of starting point For readability that 'done' and 'fd' are always consistent with each other. - dir_fd == AT_FDCWD: - path is absolute: - previous: fd = open("/") - current: fd = openat(AT_FDCWD, "/") - path is relative: - previous: fd = openat(AT_FDCWD, ".") - current: fd = openat(AT_FDCWD, ".") - dir_fd >= 0: - dir_fd points to "/": - previous: fd = openat(dir_fd, ".") - current: fd = openat(dir_fd, "/") - dir_fd does not point to "/": - previous: fd = openat(dir_fd, ".") - current: fd = openat(dir_fd, ".") Hence, this should not change any behavior. Just refactoring. --- src/basic/chase.c | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/src/basic/chase.c b/src/basic/chase.c index a4d9edb4c9..2db0b41261 100644 --- a/src/basic/chase.c +++ b/src/basic/chase.c @@ -212,6 +212,17 @@ int chaseat(int dir_fd, const char *path, ChaseFlags flags, char **ret_path, int return -ENOMEM; } + /* If a positive directory file descriptor is provided, always resolve the given path relative to it, + * regardless of whether it is absolute or not. If we get AT_FDCWD, follow regular openat() + * semantics, if the path is relative, resolve against the current working directory. Otherwise, + * resolve against root. */ + fd = openat(dir_fd, done ?: ".", O_CLOEXEC|O_DIRECTORY|O_PATH); + if (fd < 0) + return -errno; + + if (fstat(fd, &st) < 0) + return -errno; + /* If we get AT_FDCWD, we always resolve symlinks relative to the host's root. Only if a positive * directory file descriptor is provided we will look at CHASE_AT_RESOLVE_IN_ROOT to determine * whether to resolve symlinks in it or not. */ @@ -222,20 +233,6 @@ int chaseat(int dir_fd, const char *path, ChaseFlags flags, char **ret_path, int if (root_fd < 0) return -errno; - /* If a positive directory file descriptor is provided, always resolve the given path relative to it, - * regardless of whether it is absolute or not. If we get AT_FDCWD, follow regular openat() - * semantics, if the path is relative, resolve against the current working directory. Otherwise, - * resolve against root. */ - if (dir_fd >= 0 || !path_is_absolute(path)) - fd = openat(dir_fd, ".", O_CLOEXEC|O_DIRECTORY|O_PATH); - else - fd = open("/", O_CLOEXEC|O_DIRECTORY|O_PATH); - if (fd < 0) - return -errno; - - if (fstat(fd, &st) < 0) - return -errno; - if (FLAGS_SET(flags, CHASE_TRAIL_SLASH)) append_trail_slash = ENDSWITH_SET(buffer, "/", "/."); From 5f0bae7bffa8fe334dfef957d8eb48fc643e47ae Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Thu, 20 Jul 2023 15:04:01 +0900 Subject: [PATCH 2/7] chase: fix CHASE_STEP with ".." --- src/basic/chase.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/basic/chase.c b/src/basic/chase.c index 2db0b41261..8f9c361a09 100644 --- a/src/basic/chase.c +++ b/src/basic/chase.c @@ -264,8 +264,11 @@ int chaseat(int dir_fd, const char *path, ChaseFlags flags, char **ret_path, int /* If we already are at the top, then going up will not change anything. This is * in-line with how the kernel handles this. */ - if (empty_or_root(done) && FLAGS_SET(flags, CHASE_AT_RESOLVE_IN_ROOT)) + if (empty_or_root(done) && FLAGS_SET(flags, CHASE_AT_RESOLVE_IN_ROOT)) { + if (FLAGS_SET(flags, CHASE_STEP)) + goto chased_one; continue; + } fd_parent = openat(fd, "..", O_CLOEXEC|O_NOFOLLOW|O_PATH|O_DIRECTORY); if (fd_parent < 0) @@ -281,8 +284,11 @@ int chaseat(int dir_fd, const char *path, ChaseFlags flags, char **ret_path, int r = dir_fd_is_root(fd); if (r < 0) return r; - if (r > 0) + if (r > 0) { + if (FLAGS_SET(flags, CHASE_STEP)) + goto chased_one; continue; + } } r = path_extract_directory(done, &parent); From b7e957d30f7ae5318bf6c294ef796bc75ca3fe20 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Thu, 20 Jul 2023 21:17:32 +0900 Subject: [PATCH 3/7] chase: drop CHASE_AT_RESOLVE_IN_ROOT earlier The flag will be anyway dropped in chaseat(), but let's shortcut. --- src/basic/chase.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/basic/chase.c b/src/basic/chase.c index 8f9c361a09..e66070b5d1 100644 --- a/src/basic/chase.c +++ b/src/basic/chase.c @@ -511,9 +511,14 @@ int chase(const char *path, const char *root, ChaseFlags flags, char **ret_path, return -EINVAL; /* A root directory of "/" or "" is identical to "/". */ - if (empty_or_root(root)) + if (empty_or_root(root)) { root = "/"; - else { + + /* When the root directory is "/", we will drop CHASE_AT_RESOLVE_IN_ROOT in chaseat(), + * hence below is not necessary, but let's shortcut. */ + flags &= ~CHASE_AT_RESOLVE_IN_ROOT; + + } else { r = path_make_absolute_cwd(root, &root_abs); if (r < 0) return r; From 1c13bdf34291dd2444439a4e30ad35a29d22a5d8 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Fri, 21 Jul 2023 11:07:01 +0900 Subject: [PATCH 4/7] chase: add two more assertions When 'need_absolute' is true, 'done' should always contain "/" at the beginning, and thus should not be NULL. --- src/basic/chase.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/basic/chase.c b/src/basic/chase.c index e66070b5d1..4d883e18d8 100644 --- a/src/basic/chase.c +++ b/src/basic/chase.c @@ -449,6 +449,7 @@ int chaseat(int dir_fd, const char *path, ChaseFlags flags, char **ret_path, int } if (!done) { + assert(!need_absolute || FLAGS_SET(flags, CHASE_EXTRACT_FILENAME)); done = strdup(append_trail_slash ? "./" : "."); if (!done) return -ENOMEM; @@ -475,6 +476,7 @@ chased_one: const char *e; if (!done) { + assert(!need_absolute); done = strdup(append_trail_slash ? "./" : "."); if (!done) return -ENOMEM; From 4de5b4e3751609e6a0fe5e3f5dfef22ccae56fad Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Thu, 20 Jul 2023 15:17:27 +0900 Subject: [PATCH 5/7] chase: carefully handle result of extracting parent directory Should not change any behavior. --- src/basic/chase.c | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/src/basic/chase.c b/src/basic/chase.c index 4d883e18d8..b612a2fef6 100644 --- a/src/basic/chase.c +++ b/src/basic/chase.c @@ -292,9 +292,25 @@ int chaseat(int dir_fd, const char *path, ChaseFlags flags, char **ret_path, int } r = path_extract_directory(done, &parent); - if (r >= 0 || r == -EDESTADDRREQ) + if (r >= 0) { + assert(!need_absolute || path_is_absolute(parent)); free_and_replace(done, parent); - else if (IN_SET(r, -EINVAL, -EADDRNOTAVAIL)) { + } else if (r == -EDESTADDRREQ) { + /* 'done' contains filename only (i.e. no slash). */ + assert(!need_absolute); + done = mfree(done); + } else if (r == -EADDRNOTAVAIL) { + /* 'done' is "/". This branch should be already handled in the above. */ + assert(!FLAGS_SET(flags, CHASE_AT_RESOLVE_IN_ROOT)); + assert_not_reached(); + } else if (r == -EINVAL) { + /* 'done' is an empty string, ends with '..', or an invalid path. */ + assert(!need_absolute); + assert(!FLAGS_SET(flags, CHASE_AT_RESOLVE_IN_ROOT)); + + if (!path_is_valid(done)) + return -EINVAL; + /* If we're at the top of "dir_fd", start appending ".." to "done". */ if (!path_extend(&done, "..")) return -ENOMEM; From 83c57d8cde7ac43e5d00e2ed247c831f4e241ba8 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Fri, 21 Jul 2023 13:28:39 +0900 Subject: [PATCH 6/7] chase: check root path in more detail In chaseat() we call dir_fd_is_root() several places, and the final result depends on it. If the root path specified to `chase()` is not normalized but points to "/", e.g. "/../", assertions in `chaseat()` or `chase()` may be triggered. --- src/basic/chase.c | 36 +++++++++++++++++++++++++++++++++++- src/basic/fd-util.h | 3 +++ src/test/test-chase.c | 5 +++++ 3 files changed, 43 insertions(+), 1 deletion(-) diff --git a/src/basic/chase.c b/src/basic/chase.c index b612a2fef6..abb9b73384 100644 --- a/src/basic/chase.c +++ b/src/basic/chase.c @@ -518,6 +518,27 @@ chased_one: return 0; } +static int empty_or_root_to_null(const char **path) { + int r; + + assert(path); + + /* This nullifies the input path when the path is empty or points to "/". */ + + if (empty_or_root(*path)) { + *path = NULL; + return 0; + } + + r = path_is_root(*path); + if (r < 0) + return r; + if (r > 0) + *path = NULL; + + return 0; +} + int chase(const char *path, const char *root, ChaseFlags flags, char **ret_path, int *ret_fd) { _cleanup_free_ char *root_abs = NULL, *absolute = NULL, *p = NULL; _cleanup_close_ int fd = -EBADF, pfd = -EBADF; @@ -528,6 +549,10 @@ int chase(const char *path, const char *root, ChaseFlags flags, char **ret_path, if (isempty(path)) return -EINVAL; + r = empty_or_root_to_null(&root); + if (r < 0) + return r; + /* A root directory of "/" or "" is identical to "/". */ if (empty_or_root(root)) { root = "/"; @@ -624,8 +649,13 @@ int chaseat_prefix_root(const char *path, const char *root, char **ret) { if (!path_is_absolute(path)) { _cleanup_free_ char *root_abs = NULL; + r = empty_or_root_to_null(&root); + if (r < 0 && r != -ENOENT) + return r; + /* If the dir_fd points to the root directory, chaseat() always returns an absolute path. */ - assert(!empty_or_root(root)); + if (empty_or_root(root)) + return -EINVAL; r = path_make_absolute_cwd(root, &root_abs); if (r < 0) @@ -658,6 +688,10 @@ int chase_extract_filename(const char *path, const char *root, char **ret) { if (!path_is_absolute(path)) return -EINVAL; + r = empty_or_root_to_null(&root); + if (r < 0 && r != -ENOENT) + return r; + if (!empty_or_root(root)) { _cleanup_free_ char *root_abs = NULL; diff --git a/src/basic/fd-util.h b/src/basic/fd-util.h index c870a1b899..6aecb91fab 100644 --- a/src/basic/fd-util.h +++ b/src/basic/fd-util.h @@ -102,6 +102,9 @@ int read_nr_open(void); int fd_get_diskseq(int fd, uint64_t *ret); int path_is_root_at(int dir_fd, const char *path); +static inline int path_is_root(const char *path) { + return path_is_root_at(AT_FDCWD, path); +} static inline int dir_fd_is_root(int dir_fd) { return path_is_root_at(dir_fd, NULL); } diff --git a/src/test/test-chase.c b/src/test/test-chase.c index 75c508970e..d3399c11c6 100644 --- a/src/test/test-chase.c +++ b/src/test/test-chase.c @@ -73,6 +73,11 @@ TEST(chase) { assert_se(path_equal(result, "/usr")); result = mfree(result); + r = chase(p, "/.//../../../", 0, &result, NULL); + assert_se(r > 0); + assert_se(path_equal(result, "/usr")); + result = mfree(result); + pslash = strjoina(p, "/"); r = chase(pslash, NULL, 0, &result, NULL); assert_se(r > 0); From 4445242a1ea9e4988768c8ef36fa8ffe298ca925 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Fri, 28 Jul 2023 10:07:05 +0900 Subject: [PATCH 7/7] chase: propagate error in dir_fd_is_root() Otherwise, if it fails, chaseat() may return unexpected result and triggers an assertion in chase(). --- src/basic/chase.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/basic/chase.c b/src/basic/chase.c index abb9b73384..c3ecbbb3fe 100644 --- a/src/basic/chase.c +++ b/src/basic/chase.c @@ -72,6 +72,13 @@ static int log_prohibited_symlink(int fd, ChaseFlags flags) { strna(n1)); } +static int chaseat_needs_absolute(int dir_fd, const char *path) { + if (dir_fd < 0) + return path_is_absolute(path); + + return dir_fd_is_root(dir_fd); +} + int chaseat(int dir_fd, const char *path, ChaseFlags flags, char **ret_path, int *ret_fd) { _cleanup_free_ char *buffer = NULL, *done = NULL; _cleanup_close_ int fd = -EBADF, root_fd = -EBADF; @@ -205,7 +212,11 @@ int chaseat(int dir_fd, const char *path, ChaseFlags flags, char **ret_path, int /* If we receive an absolute path together with AT_FDCWD, we need to return an absolute path, because * a relative path would be interpreted relative to the current working directory. Also, let's make * the result absolute when the file descriptor of the root directory is specified. */ - bool need_absolute = (dir_fd == AT_FDCWD && path_is_absolute(path)) || (dir_fd >= 0 && dir_fd_is_root(dir_fd) > 0); + r = chaseat_needs_absolute(dir_fd, path); + if (r < 0) + return r; + + bool need_absolute = r; if (need_absolute) { done = strdup("/"); if (!done)