From db0096f2addd3e2fd74dc8c6bfa719775a13ea74 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Mon, 10 Apr 2023 08:17:07 +0900 Subject: [PATCH 1/9] chase: drop unnecessary conditions for buffer --- src/basic/chase.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/basic/chase.c b/src/basic/chase.c index 318454ca88..64f8cd366f 100644 --- a/src/basic/chase.c +++ b/src/basic/chase.c @@ -176,7 +176,7 @@ int chaseat(int dir_fd, const char *path, ChaseFlags flags, char **ret_path, int /* Shortcut the ret_fd case if the caller isn't interested in the actual path and has no root * set and doesn't care about any of the other special features we provide either. */ - r = openat(dir_fd, buffer ?: path, O_PATH|O_CLOEXEC|((flags & CHASE_NOFOLLOW) ? O_NOFOLLOW : 0)); + r = openat(dir_fd, path, O_PATH|O_CLOEXEC|((flags & CHASE_NOFOLLOW) ? O_NOFOLLOW : 0)); if (r < 0) return -errno; @@ -184,11 +184,9 @@ int chaseat(int dir_fd, const char *path, ChaseFlags flags, char **ret_path, int return 0; } - if (!buffer) { - buffer = strdup(path); - if (!buffer) - return -ENOMEM; - } + buffer = strdup(path); + if (!buffer) + return -ENOMEM; /* 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. */ From ad66c7f1d3c46ffa91bb47604191b24730855740 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Mon, 10 Apr 2023 08:40:08 +0900 Subject: [PATCH 2/9] chase: make struct stat always sync with the opened fd Previously, struct stat may not be correctly synced with the currently opened fd, e.g. when a path contains symlink which points to an absolute path. This also rename variables for struct stat, to make them consistent with the corresponding fd. --- src/basic/chase.c | 48 ++++++++++++++++++++++------------------------- 1 file changed, 22 insertions(+), 26 deletions(-) diff --git a/src/basic/chase.c b/src/basic/chase.c index 64f8cd366f..699ca6985f 100644 --- a/src/basic/chase.c +++ b/src/basic/chase.c @@ -77,7 +77,7 @@ int chaseat(int dir_fd, const char *path, ChaseFlags flags, char **ret_path, int _cleanup_close_ int fd = -EBADF, root_fd = -EBADF; unsigned max_follow = CHASE_MAX; /* how many symlinks to follow before giving up and returning ELOOP */ bool exists = true, append_trail_slash = false; - struct stat previous_stat; + struct stat st; /* stat obtained from fd */ const char *todo; int r; @@ -218,7 +218,7 @@ int chaseat(int dir_fd, const char *path, ChaseFlags flags, char **ret_path, int if (fd < 0) return -errno; - if (fstat(fd, &previous_stat) < 0) + if (fstat(fd, &st) < 0) return -errno; if (flags & CHASE_TRAIL_SLASH) @@ -227,7 +227,7 @@ int chaseat(int dir_fd, const char *path, ChaseFlags flags, char **ret_path, int for (todo = buffer;;) { _cleanup_free_ char *first = NULL; _cleanup_close_ int child = -EBADF; - struct stat st; + struct stat st_child; const char *e; r = path_find_first_component(&todo, /* accept_dot_dot= */ true, &e); @@ -248,6 +248,7 @@ int chaseat(int dir_fd, const char *path, ChaseFlags flags, char **ret_path, int if (path_equal(first, "..")) { _cleanup_free_ char *parent = NULL; _cleanup_close_ int fd_parent = -EBADF; + struct stat st_parent; /* If we already are at the top, then going up will not change anything. This is * in-line with how the kernel handles this. */ @@ -258,12 +259,12 @@ int chaseat(int dir_fd, const char *path, ChaseFlags flags, char **ret_path, int if (fd_parent < 0) return -errno; - if (fstat(fd_parent, &st) < 0) + if (fstat(fd_parent, &st_parent) < 0) return -errno; /* If we opened the same directory, that means we're at the host root directory, so * going up won't change anything. */ - if (st.st_dev == previous_stat.st_dev && st.st_ino == previous_stat.st_ino) + if (st_parent.st_dev == st.st_dev && st_parent.st_ino == st.st_ino) continue; r = path_extract_directory(done, &parent); @@ -279,18 +280,16 @@ int chaseat(int dir_fd, const char *path, ChaseFlags flags, char **ret_path, int if (flags & CHASE_STEP) goto chased_one; - if (flags & CHASE_SAFE) { - if (unsafe_transition(&previous_stat, &st)) - return log_unsafe_transition(fd, fd_parent, path, flags); - - previous_stat = st; - } + if (flags & CHASE_SAFE && + unsafe_transition(&st, &st_parent)) + return log_unsafe_transition(fd, fd_parent, path, flags); if (FLAGS_SET(flags, CHASE_PARENT) && isempty(todo)) break; + /* update fd and stat */ + st = st_parent; close_and_replace(fd, fd_parent); - continue; } @@ -322,19 +321,18 @@ int chaseat(int dir_fd, const char *path, ChaseFlags flags, char **ret_path, int return r; } - if (fstat(child, &st) < 0) + if (fstat(child, &st_child) < 0) return -errno; - if ((flags & CHASE_SAFE) && - unsafe_transition(&previous_stat, &st)) - return log_unsafe_transition(fd, child, path, flags); - previous_stat = st; + if ((flags & CHASE_SAFE) && + unsafe_transition(&st, &st_child)) + return log_unsafe_transition(fd, child, path, flags); if ((flags & CHASE_NO_AUTOFS) && fd_is_fs_type(child, AUTOFS_SUPER_MAGIC) > 0) return log_autofs_mount_point(child, path, flags); - if (S_ISLNK(st.st_mode) && !((flags & CHASE_NOFOLLOW) && isempty(todo))) { + if (S_ISLNK(st_child.st_mode) && !((flags & CHASE_NOFOLLOW) && isempty(todo))) { _cleanup_free_ char *destination = NULL; if (flags & CHASE_PROHIBIT_SYMLINKS) @@ -361,15 +359,12 @@ int chaseat(int dir_fd, const char *path, ChaseFlags flags, char **ret_path, int if (fd < 0) return fd; - if (flags & CHASE_SAFE) { - if (fstat(fd, &st) < 0) - return -errno; + if (fstat(fd, &st) < 0) + return -errno; - if (unsafe_transition(&previous_stat, &st)) - return log_unsafe_transition(child, fd, path, flags); - - previous_stat = st; - } + if (flags & CHASE_SAFE && + unsafe_transition(&st_child, &st)) + return log_unsafe_transition(child, fd, path, flags); r = free_and_strdup(&done, need_absolute ? "/" : NULL); if (r < 0) @@ -398,6 +393,7 @@ int chaseat(int dir_fd, const char *path, ChaseFlags flags, char **ret_path, int break; /* And iterate again, but go one directory further down. */ + st = st_child; close_and_replace(fd, child); } From 75adfc3b17176339d98de1132ab1fba6e203389e Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Mon, 10 Apr 2023 08:43:28 +0900 Subject: [PATCH 3/9] chase: drop one redundant call of fstat() Now, 'st' is always synced with 'fd'. Hence, we can use stat_verify_directory(). --- src/basic/chase.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/basic/chase.c b/src/basic/chase.c index 699ca6985f..dcc2339944 100644 --- a/src/basic/chase.c +++ b/src/basic/chase.c @@ -398,7 +398,7 @@ int chaseat(int dir_fd, const char *path, ChaseFlags flags, char **ret_path, int } if (flags & CHASE_PARENT) { - r = fd_verify_directory(fd); + r = stat_verify_directory(&st); if (r < 0) return r; } From a0d3e0ec797974e7aaab6e56a89de44fc382d52d Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Mon, 10 Apr 2023 08:57:01 +0900 Subject: [PATCH 4/9] chase: extend comment about CHASE_PARENT --- src/basic/chase.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/basic/chase.h b/src/basic/chase.h index 1314777cb4..7e9ebe0685 100644 --- a/src/basic/chase.h +++ b/src/basic/chase.h @@ -22,7 +22,9 @@ typedef enum ChaseFlags { CHASE_PROHIBIT_SYMLINKS = 1 << 9, /* Refuse all symlinks */ CHASE_PARENT = 1 << 10, /* Chase the parent directory of the given path. Note that the * full path is still stored in ret_path and only the returned - * file descriptor will point to the parent directory. */ + * file descriptor will point to the parent directory. Note that + * the result path is the root or '.', then the file descriptor + * also points to the result path even if this flag is set. */ CHASE_MKDIR_0755 = 1 << 11, /* Create any missing parent directories in the given path. */ CHASE_EXTRACT_FILENAME = 1 << 12, /* Only return the last component of the resolved path */ } ChaseFlags; @@ -51,4 +53,3 @@ int chase_and_accessat(int dir_fd, const char *path, ChaseFlags chase_flags, int int chase_and_fopenat_unlocked(int dir_fd, const char *path, ChaseFlags chase_flags, const char *open_flags, char **ret_path, FILE **ret_file); int chase_and_unlinkat(int dir_fd, const char *path, ChaseFlags chase_flags, int unlink_flags, char **ret_path); int chase_and_open_parent_at(int dir_fd, const char *path, ChaseFlags chase_flags, char **ret_filename); - From 3a7ba9f6b9d9a80c7f909bfbf24b5fc8c99a3176 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Mon, 10 Apr 2023 09:30:14 +0900 Subject: [PATCH 5/9] path-util: make iterator for path_find_last_component() always finish with the beginning of the buffer --- src/basic/path-util.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/basic/path-util.c b/src/basic/path-util.c index a5ab6045cd..0b0f0da760 100644 --- a/src/basic/path-util.c +++ b/src/basic/path-util.c @@ -901,6 +901,8 @@ static const char *skip_slash_or_dot_backward(const char *path, const char *q) { continue; if (q > path && strneq(q - 1, "/.", 2)) continue; + if (q == path && *q == '.') + continue; break; } return q; @@ -925,6 +927,12 @@ int path_find_last_component(const char *path, bool accept_dot_dot, const char * * ret: "bbbbb/cc//././" * return value: 5 (== strlen("bbbbb")) * + * Input: path: "//.//aaa///bbbbb/cc//././" + * next: "///bbbbb/cc//././" + * Output: next: "//.//aaa///bbbbb/cc//././" (next == path) + * ret: "aaa///bbbbb/cc//././" + * return value: 3 (== strlen("aaa")) + * * Input: path: "/", ".", "", or NULL * Output: next: equivalent to path * ret: NULL From de68bf789607e83d0a3c31b34b5e7a9efe319131 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Mon, 10 Apr 2023 09:31:28 +0900 Subject: [PATCH 6/9] test: add tests about iterator position for path_find_{first,last}_component() When path_find_first_component() returns the last component, the iterator must be an empty string. The fact is heavily used in chaseat(). Let's explicitly test it. --- src/test/test-path-util.c | 27 +++++++++++++++++++++++---- 1 file changed, 23 insertions(+), 4 deletions(-) diff --git a/src/test/test-path-util.c b/src/test/test-path-util.c index 87e33919e0..e40ffea4d5 100644 --- a/src/test/test-path-util.c +++ b/src/test/test-path-util.c @@ -676,9 +676,10 @@ static void test_path_find_first_component_one( r = path_find_first_component(&p, accept_dot_dot, &e); if (r <= 0) { if (r == 0) { - if (path) + if (path) { assert_se(p == path + strlen_ptr(path)); - else + assert_se(isempty(p)); + } else assert_se(!p); assert_se(!e); } @@ -691,6 +692,15 @@ static void test_path_find_first_component_one( assert_se(strcspn(e, "/") == (size_t) r); assert_se(strlen_ptr(*expected) == (size_t) r); assert_se(strneq(e, *expected++, r)); + + assert_se(p); + log_debug("p=%s", p); + if (!isempty(*expected)) + assert_se(startswith(p, *expected)); + else if (ret >= 0) { + assert_se(p == path + strlen_ptr(path)); + assert_se(isempty(p)); + } } } @@ -712,7 +722,7 @@ TEST(path_find_first_component) { test_path_find_first_component_one("././//.///aa/bbb//./ccc", false, STRV_MAKE("aa", "bbb", "ccc"), 0); test_path_find_first_component_one("././//.///aa/.../../bbb//./ccc/.", false, STRV_MAKE("aa", "..."), -EINVAL); test_path_find_first_component_one("//./aaa///.//./.bbb/..///c.//d.dd///..eeee/.", false, STRV_MAKE("aaa", ".bbb"), -EINVAL); - test_path_find_first_component_one("a/foo./b", false, STRV_MAKE("a", "foo.", "b"), 0); + test_path_find_first_component_one("a/foo./b//././/", false, STRV_MAKE("a", "foo.", "b"), 0); test_path_find_first_component_one(NULL, true, NULL, 0); test_path_find_first_component_one("", true, NULL, 0); @@ -728,7 +738,7 @@ TEST(path_find_first_component) { test_path_find_first_component_one("././//.///aa/bbb//./ccc", true, STRV_MAKE("aa", "bbb", "ccc"), 0); test_path_find_first_component_one("././//.///aa/.../../bbb//./ccc/.", true, STRV_MAKE("aa", "...", "..", "bbb", "ccc"), 0); test_path_find_first_component_one("//./aaa///.//./.bbb/..///c.//d.dd///..eeee/.", true, STRV_MAKE("aaa", ".bbb", "..", "c.", "d.dd", "..eeee"), 0); - test_path_find_first_component_one("a/foo./b", true, STRV_MAKE("a", "foo.", "b"), 0); + test_path_find_first_component_one("a/foo./b//././/", true, STRV_MAKE("a", "foo.", "b"), 0); memset(foo, 'a', sizeof(foo) -1); char_array_0(foo); @@ -770,6 +780,15 @@ static void test_path_find_last_component_one( assert_se(strcspn(e, "/") == (size_t) r); assert_se(strlen_ptr(*expected) == (size_t) r); assert_se(strneq(e, *expected++, r)); + + assert_se(next); + log_debug("path=%s\nnext=%s", path, next); + if (!isempty(*expected)) { + assert_se(next < path + strlen(path)); + assert_se(next >= path + strlen(*expected)); + assert_se(startswith(next - strlen(*expected), *expected)); + } else if (ret >= 0) + assert_se(next == path); } } From 4443e3003a8b01a87c773ff3e1638057344fc46f Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Mon, 10 Apr 2023 09:45:21 +0900 Subject: [PATCH 7/9] chase: use stat_inode_same() at one more place --- src/basic/chase.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/basic/chase.c b/src/basic/chase.c index dcc2339944..b8441aa28b 100644 --- a/src/basic/chase.c +++ b/src/basic/chase.c @@ -264,7 +264,7 @@ int chaseat(int dir_fd, const char *path, ChaseFlags flags, char **ret_path, int /* If we opened the same directory, that means we're at the host root directory, so * going up won't change anything. */ - if (st_parent.st_dev == st.st_dev && st_parent.st_ino == st.st_ino) + if (stat_inode_same(&st_parent, &st)) continue; r = path_extract_directory(done, &parent); From a6ef5ef70726c77e621341b4de12413e1864e934 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Mon, 10 Apr 2023 09:55:22 +0900 Subject: [PATCH 8/9] chase: use dir_fd_is_root() to check if fd points to the root directory As commented in dir_fd_is_root(), comparing inode is not enough to determine if we are at the root directory. --- src/basic/chase.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/basic/chase.c b/src/basic/chase.c index b8441aa28b..f62c317026 100644 --- a/src/basic/chase.c +++ b/src/basic/chase.c @@ -262,10 +262,16 @@ int chaseat(int dir_fd, const char *path, ChaseFlags flags, char **ret_path, int if (fstat(fd_parent, &st_parent) < 0) return -errno; - /* If we opened the same directory, that means we're at the host root directory, so + /* If we opened the same directory, that _may_ indicate that we're at the host root + * directory. Let's confirm that in more detail with dir_fd_is_root(). And if so, * going up won't change anything. */ - if (stat_inode_same(&st_parent, &st)) - continue; + if (stat_inode_same(&st_parent, &st)) { + r = dir_fd_is_root(fd); + if (r < 0) + return r; + if (r > 0) + continue; + } r = path_extract_directory(done, &parent); if (r >= 0 || r == -EDESTADDRREQ) From b4cb4c5cc6b074f79e317c96930109e6efb8de25 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Mon, 10 Apr 2023 10:04:25 +0900 Subject: [PATCH 9/9] fd-util: slightly optimize dir_fd_is_root() When STATX_MNT_ID is not supported, we need to manually obtain mount id. Let's compare inodes earlier. --- src/basic/fd-util.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/src/basic/fd-util.c b/src/basic/fd-util.c index 3cc4f44bcd..7125e28e1b 100644 --- a/src/basic/fd-util.c +++ b/src/basic/fd-util.c @@ -903,6 +903,14 @@ int dir_fd_is_root(int dir_fd) { if (r < 0) return r; + r = statx_fallback(dir_fd, "..", 0, STATX_TYPE|STATX_INO|STATX_MNT_ID, &pst.sx); + if (r < 0) + return r; + + /* First, compare inode. If these are different, the fd does not point to the root directory "/". */ + if (!statx_inode_same(&st.sx, &pst.sx)) + return false; + if (!FLAGS_SET(st.nsx.stx_mask, STATX_MNT_ID)) { int mntid; @@ -915,10 +923,6 @@ int dir_fd_is_root(int dir_fd) { st.nsx.stx_mask |= STATX_MNT_ID; } - r = statx_fallback(dir_fd, "..", 0, STATX_TYPE|STATX_INO|STATX_MNT_ID, &pst.sx); - if (r < 0) - return r; - if (!FLAGS_SET(pst.nsx.stx_mask, STATX_MNT_ID)) { int mntid; @@ -931,14 +935,14 @@ int dir_fd_is_root(int dir_fd) { pst.nsx.stx_mask |= STATX_MNT_ID; } - /* If the parent directory is the same inode, the fd points to the root directory "/". We also check - * that the mount ids are the same. Otherwise, a construct like the following could be used to trick - * us: + /* Even if the parent directory has the same inode, the fd may not point to the root directory "/", + * and we also need to check that the mount ids are the same. Otherwise, a construct like the + * following could be used to trick us: * * $ mkdir /tmp/x /tmp/x/y * $ mount --bind /tmp/x /tmp/x/y */ - return statx_inode_same(&st.sx, &pst.sx) && statx_mount_same(&st.nsx, &pst.nsx); + return statx_mount_same(&st.nsx, &pst.nsx); } const char *accmode_to_string(int flags) {