From 7486f9c3421a5cd011c72a8c8d9c8b1cf62410ca Mon Sep 17 00:00:00 2001 From: Daan De Meyer Date: Tue, 14 Mar 2023 09:55:04 +0100 Subject: [PATCH 1/5] fs-util: Add xopenat() xopenat() will create directories if O_DIRECTORY and O_CREAT are specified. Note that this is not an atomic operation. --- src/basic/fs-util.c | 73 ++++++++++++++++++++++++++--------------- src/basic/fs-util.h | 2 ++ src/test/test-fs-util.c | 25 ++++++++++++++ 3 files changed, 73 insertions(+), 27 deletions(-) diff --git a/src/basic/fs-util.c b/src/basic/fs-util.c index a895f4f2df..5007a829ea 100644 --- a/src/basic/fs-util.c +++ b/src/basic/fs-util.c @@ -993,7 +993,6 @@ int parse_cifs_service( int open_mkdir_at(int dirfd, const char *path, int flags, mode_t mode) { _cleanup_close_ int fd = -EBADF, parent_fd = -EBADF; _cleanup_free_ char *fname = NULL; - bool made; int r; /* Creates a directory with mkdirat() and then opens it, in the "most atomic" fashion we can @@ -1033,33 +1032,11 @@ int open_mkdir_at(int dirfd, const char *path, int flags, mode_t mode) { path = fname; } - r = RET_NERRNO(mkdirat(dirfd, path, mode)); - if (r == -EEXIST) { - if (FLAGS_SET(flags, O_EXCL)) - return -EEXIST; - - made = false; - } else if (r < 0) - return r; - else - made = true; - - fd = RET_NERRNO(openat(dirfd, path, (flags & ~O_EXCL)|O_DIRECTORY|O_NOFOLLOW)); - if (fd < 0) { - if (fd == -ENOENT) /* We got ENOENT? then someone else immediately removed it after we - * created it. In that case let's return immediately without unlinking - * anything, because there simply isn't anything to unlink anymore. */ - return -ENOENT; - if (fd == -ELOOP) /* is a symlink? exists already → created by someone else, don't unlink */ - return -EEXIST; - if (fd == -ENOTDIR) /* not a directory? exists already → created by someone else, don't unlink */ - return -EEXIST; - - if (made) - (void) unlinkat(dirfd, path, AT_REMOVEDIR); - + fd = xopenat(dirfd, path, flags|O_CREAT|O_DIRECTORY|O_NOFOLLOW, mode); + if (IN_SET(fd, -ELOOP, -ENOTDIR)) + return -EEXIST; + if (fd < 0) return fd; - } return TAKE_FD(fd); } @@ -1110,3 +1087,45 @@ int openat_report_new(int dirfd, const char *pathname, int flags, mode_t mode, b return -EEXIST; } } + +int xopenat(int dir_fd, const char *path, int flags, mode_t mode) { + _cleanup_close_ int fd = -EBADF; + bool made = false; + int r; + + if (FLAGS_SET(flags, O_DIRECTORY|O_CREAT)) { + r = RET_NERRNO(mkdirat(dir_fd, path, mode)); + if (r == -EEXIST) { + if (FLAGS_SET(flags, O_EXCL)) + return -EEXIST; + + made = false; + } else if (r < 0) + return r; + else + made = true; + + flags &= ~(O_EXCL|O_CREAT); + } + + fd = RET_NERRNO(openat(dir_fd, path, flags, mode)); + if (fd < 0) { + if (IN_SET(fd, + /* We got ENOENT? then someone else immediately removed it after we + * created it. In that case let's return immediately without unlinking + * anything, because there simply isn't anything to unlink anymore. */ + -ENOENT, + /* is a symlink? exists already → created by someone else, don't unlink */ + -ELOOP, + /* not a directory? exists already → created by someone else, don't unlink */ + -ENOTDIR)) + return fd; + + if (made) + (void) unlinkat(dir_fd, path, AT_REMOVEDIR); + + return fd; + } + + return TAKE_FD(fd); +} diff --git a/src/basic/fs-util.h b/src/basic/fs-util.h index 932d003f19..3db9df2224 100644 --- a/src/basic/fs-util.h +++ b/src/basic/fs-util.h @@ -129,3 +129,5 @@ int parse_cifs_service(const char *s, char **ret_host, char **ret_service, char int open_mkdir_at(int dirfd, const char *path, int flags, mode_t mode); int openat_report_new(int dirfd, const char *pathname, int flags, mode_t mode, bool *ret_newly_created); + +int xopenat(int dir_fd, const char *path, int flags, mode_t mode); diff --git a/src/test/test-fs-util.c b/src/test/test-fs-util.c index c009424a2b..065ad3c6e7 100644 --- a/src/test/test-fs-util.c +++ b/src/test/test-fs-util.c @@ -1155,6 +1155,31 @@ TEST(openat_report_new) { assert_se(b); } +TEST(xopenat) { + _cleanup_close_ int tfd = -EBADF, fd = -EBADF; + _cleanup_(rm_rf_physical_and_freep) char *t = NULL; + + assert_se((tfd = mkdtemp_open(NULL, 0, &t)) >= 0); + + /* Test that xopenat() creates directories if O_DIRECTORY is specified. */ + + assert_se((fd = xopenat(tfd, "abc", O_DIRECTORY|O_CREAT|O_EXCL|O_CLOEXEC, 0755)) >= 0); + assert_se((fd_verify_directory(fd) >= 0)); + fd = safe_close(fd); + + assert_se(xopenat(tfd, "abc", O_DIRECTORY|O_CREAT|O_EXCL|O_CLOEXEC, 0755) == -EEXIST); + + assert_se((fd = xopenat(tfd, "abc", O_DIRECTORY|O_CREAT|O_CLOEXEC, 0755)) >= 0); + assert_se((fd_verify_directory(fd) >= 0)); + fd = safe_close(fd); + + /* Test that xopenat() creates regular files if O_DIRECTORY is not specified. */ + + assert_se((fd = xopenat(tfd, "def", O_CREAT|O_EXCL|O_CLOEXEC, 0644)) >= 0); + assert_se(fd_verify_regular(fd) >= 0); + fd = safe_close(fd); +} + static int intro(void) { arg_test_dir = saved_argv[1]; return EXIT_SUCCESS; From 0a9b88787893a95397d0fdfa0ebaf099ab21944e Mon Sep 17 00:00:00 2001 From: Daan De Meyer Date: Mon, 13 Mar 2023 17:07:50 +0100 Subject: [PATCH 2/5] chase-symlinks: Fix memory leak --- src/basic/chase-symlinks.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/basic/chase-symlinks.c b/src/basic/chase-symlinks.c index f134d36925..bf90ba23cc 100644 --- a/src/basic/chase-symlinks.c +++ b/src/basic/chase-symlinks.c @@ -522,7 +522,9 @@ int chase_symlinks( return r; if (ret_path) { - char *q = path_join(empty_to_root(root), p); + _cleanup_free_ char *q = NULL; + + q = path_join(empty_to_root(root), p); if (!q) return -ENOMEM; From 06344330a16b2bc6daeeef55bf77eea937446755 Mon Sep 17 00:00:00 2001 From: Daan De Meyer Date: Mon, 13 Mar 2023 21:55:04 +0100 Subject: [PATCH 3/5] chase-symlinks: Remove unused ret_fd arguments --- src/basic/chase-symlinks.c | 15 ++++----------- src/basic/chase-symlinks.h | 4 ++-- src/basic/stat-util.c | 2 +- src/boot/bootctl-install.c | 2 +- src/boot/bootctl-status.c | 2 +- src/core/dbus-manager.c | 2 +- src/coredump/coredumpctl.c | 2 +- src/nspawn/nspawn-mount.c | 2 +- src/partition/repart.c | 2 +- src/shared/bootspec.c | 2 +- src/systemctl/systemctl-switch-root.c | 4 ++-- 11 files changed, 16 insertions(+), 23 deletions(-) diff --git a/src/basic/chase-symlinks.c b/src/basic/chase-symlinks.c index bf90ba23cc..1b434950e6 100644 --- a/src/basic/chase-symlinks.c +++ b/src/basic/chase-symlinks.c @@ -631,21 +631,19 @@ int chase_symlinks_and_stat( const char *root, ChaseSymlinksFlags chase_flags, char **ret_path, - struct stat *ret_stat, - int *ret_fd) { + struct stat *ret_stat) { _cleanup_close_ int path_fd = -EBADF; _cleanup_free_ char *p = NULL; int r; assert(path); - assert(ret_stat); if (chase_flags & (CHASE_NONEXISTENT|CHASE_STEP)) return -EINVAL; if (empty_or_root(root) && !ret_path && - (chase_flags & (CHASE_NO_AUTOFS|CHASE_SAFE|CHASE_PROHIBIT_SYMLINKS|CHASE_PARENT|CHASE_MKDIR_0755)) == 0 && !ret_fd) { + (chase_flags & (CHASE_NO_AUTOFS|CHASE_SAFE|CHASE_PROHIBIT_SYMLINKS|CHASE_PARENT|CHASE_MKDIR_0755)) == 0) { /* Shortcut this call if none of the special features of this call are requested */ if (fstatat(AT_FDCWD, path, ret_stat, FLAGS_SET(chase_flags, CHASE_NOFOLLOW) ? AT_SYMLINK_NOFOLLOW : 0) < 0) @@ -664,8 +662,6 @@ int chase_symlinks_and_stat( if (ret_path) *ret_path = TAKE_PTR(p); - if (ret_fd) - *ret_fd = TAKE_FD(path_fd); return 1; } @@ -675,8 +671,7 @@ int chase_symlinks_and_access( const char *root, ChaseSymlinksFlags chase_flags, int access_mode, - char **ret_path, - int *ret_fd) { + char **ret_path) { _cleanup_close_ int path_fd = -EBADF; _cleanup_free_ char *p = NULL; @@ -688,7 +683,7 @@ int chase_symlinks_and_access( return -EINVAL; if (empty_or_root(root) && !ret_path && - (chase_flags & (CHASE_NO_AUTOFS|CHASE_SAFE|CHASE_PROHIBIT_SYMLINKS|CHASE_PARENT|CHASE_MKDIR_0755)) == 0 && !ret_fd) { + (chase_flags & (CHASE_NO_AUTOFS|CHASE_SAFE|CHASE_PROHIBIT_SYMLINKS|CHASE_PARENT|CHASE_MKDIR_0755)) == 0) { /* Shortcut this call if none of the special features of this call are requested */ if (faccessat(AT_FDCWD, path, access_mode, FLAGS_SET(chase_flags, CHASE_NOFOLLOW) ? AT_SYMLINK_NOFOLLOW : 0) < 0) @@ -708,8 +703,6 @@ int chase_symlinks_and_access( if (ret_path) *ret_path = TAKE_PTR(p); - if (ret_fd) - *ret_fd = TAKE_FD(path_fd); return 1; } diff --git a/src/basic/chase-symlinks.h b/src/basic/chase-symlinks.h index 19acf9e4b5..26e8cfceca 100644 --- a/src/basic/chase-symlinks.h +++ b/src/basic/chase-symlinks.h @@ -33,8 +33,8 @@ int chase_symlinks(const char *path_with_prefix, const char *root, ChaseSymlinks int chase_symlinks_and_open(const char *path, const char *root, ChaseSymlinksFlags chase_flags, int open_flags, char **ret_path); int chase_symlinks_and_opendir(const char *path, const char *root, ChaseSymlinksFlags chase_flags, char **ret_path, DIR **ret_dir); -int chase_symlinks_and_stat(const char *path, const char *root, ChaseSymlinksFlags chase_flags, char **ret_path, struct stat *ret_stat, int *ret_fd); -int chase_symlinks_and_access(const char *path, const char *root, ChaseSymlinksFlags chase_flags, int access_mode, char **ret_path, int *ret_fd); +int chase_symlinks_and_stat(const char *path, const char *root, ChaseSymlinksFlags chase_flags, char **ret_path, struct stat *ret_stat); +int chase_symlinks_and_access(const char *path, const char *root, ChaseSymlinksFlags chase_flags, int access_mode, char **ret_path); int chase_symlinks_and_fopen_unlocked(const char *path, const char *root, ChaseSymlinksFlags chase_flags, const char *open_flags, char **ret_path, FILE **ret_file); int chase_symlinks_and_unlink(const char *path, const char *root, ChaseSymlinksFlags chase_flags, int unlink_flags, char **ret_path); diff --git a/src/basic/stat-util.c b/src/basic/stat-util.c index 700e28f2c2..88b8346a0f 100644 --- a/src/basic/stat-util.c +++ b/src/basic/stat-util.c @@ -145,7 +145,7 @@ int null_or_empty_path_with_root(const char *fn, const char *root) { if (path_equal_ptr(path_startswith(fn, root ?: "/"), "dev/null")) return true; - r = chase_symlinks_and_stat(fn, root, CHASE_PREFIX_ROOT, NULL, &st, NULL); + r = chase_symlinks_and_stat(fn, root, CHASE_PREFIX_ROOT, NULL, &st); if (r < 0) return r; diff --git a/src/boot/bootctl-install.c b/src/boot/bootctl-install.c index 624b88abe3..1b86c7773e 100644 --- a/src/boot/bootctl-install.c +++ b/src/boot/bootctl-install.c @@ -663,7 +663,7 @@ static int install_variables( return 0; } - r = chase_symlinks_and_access(path, esp_path, CHASE_PREFIX_ROOT|CHASE_PROHIBIT_SYMLINKS, F_OK, NULL, NULL); + r = chase_symlinks_and_access(path, esp_path, CHASE_PREFIX_ROOT|CHASE_PROHIBIT_SYMLINKS, F_OK, NULL); if (r == -ENOENT) return 0; if (r < 0) diff --git a/src/boot/bootctl-status.c b/src/boot/bootctl-status.c index e25cd963ae..ce97b013cf 100644 --- a/src/boot/bootctl-status.c +++ b/src/boot/bootctl-status.c @@ -560,7 +560,7 @@ static void deref_unlink_file(Hashmap *known_files, const char *fn, const char * return; if (arg_dry_run) { - r = chase_symlinks_and_access(fn, root, CHASE_PREFIX_ROOT|CHASE_PROHIBIT_SYMLINKS, F_OK, &path, NULL); + r = chase_symlinks_and_access(fn, root, CHASE_PREFIX_ROOT|CHASE_PROHIBIT_SYMLINKS, F_OK, &path); if (r < 0) log_info_errno(r, "Unable to determine whether \"%s\" exists, ignoring: %m", fn); else diff --git a/src/core/dbus-manager.c b/src/core/dbus-manager.c index 242a662bca..2198e73f24 100644 --- a/src/core/dbus-manager.c +++ b/src/core/dbus-manager.c @@ -1768,7 +1768,7 @@ static int method_switch_root(sd_bus_message *message, void *userdata, sd_bus_er return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "Path to init binary '%s' not absolute.", init); - r = chase_symlinks_and_access(init, root, CHASE_PREFIX_ROOT, X_OK, NULL, NULL); + r = chase_symlinks_and_access(init, root, CHASE_PREFIX_ROOT, X_OK, NULL); if (r == -EACCES) return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "Init binary %s is not executable.", init); diff --git a/src/coredump/coredumpctl.c b/src/coredump/coredumpctl.c index 4c312d0fbb..909ffc8d8b 100644 --- a/src/coredump/coredumpctl.c +++ b/src/coredump/coredumpctl.c @@ -981,7 +981,7 @@ static int save_core(sd_journal *j, FILE *file, char **path, bool *unlink_temp) return r; assert(r > 0); - r = chase_symlinks_and_access(filename, arg_root, CHASE_PREFIX_ROOT, F_OK, &resolved, NULL); + r = chase_symlinks_and_access(filename, arg_root, CHASE_PREFIX_ROOT, F_OK, &resolved); if (r < 0) return log_error_errno(r, "Cannot access \"%s%s\": %m", strempty(arg_root), filename); diff --git a/src/nspawn/nspawn-mount.c b/src/nspawn/nspawn-mount.c index ea54b23e08..f190239cad 100644 --- a/src/nspawn/nspawn-mount.c +++ b/src/nspawn/nspawn-mount.c @@ -926,7 +926,7 @@ static int mount_inaccessible(const char *dest, CustomMount *m) { assert(dest); assert(m); - r = chase_symlinks_and_stat(m->destination, dest, CHASE_PREFIX_ROOT, &where, &st, NULL); + r = chase_symlinks_and_stat(m->destination, dest, CHASE_PREFIX_ROOT, &where, &st); if (r < 0) { log_full_errno(m->graceful ? LOG_DEBUG : LOG_ERR, r, "Failed to resolve %s/%s: %m", dest, m->destination); return m->graceful ? 0 : r; diff --git a/src/partition/repart.c b/src/partition/repart.c index e8f3cb7fe7..2b1fbbdae2 100644 --- a/src/partition/repart.c +++ b/src/partition/repart.c @@ -4021,7 +4021,7 @@ static int add_exclude_path(const char *path, Hashmap **denylist, DenyType type) if (!st) return log_oom(); - r = chase_symlinks_and_stat(path, arg_root, CHASE_PREFIX_ROOT, NULL, st, NULL); + r = chase_symlinks_and_stat(path, arg_root, CHASE_PREFIX_ROOT, NULL, st); if (r == -ENOENT) return 0; if (r < 0) diff --git a/src/shared/bootspec.c b/src/shared/bootspec.c index 49fb34b4d6..9dddb0e289 100644 --- a/src/shared/bootspec.c +++ b/src/shared/bootspec.c @@ -1269,7 +1269,7 @@ static void boot_entry_file_list( assert(p); assert(ret_status); - int status = chase_symlinks_and_access(p, root, CHASE_PREFIX_ROOT|CHASE_PROHIBIT_SYMLINKS, F_OK, NULL, NULL); + int status = chase_symlinks_and_access(p, root, CHASE_PREFIX_ROOT|CHASE_PROHIBIT_SYMLINKS, F_OK, NULL); /* Note that this shows two '/' between the root and the file. This is intentional to highlight (in * the absence of color support) to the user that the boot loader is only interested in the second diff --git a/src/systemctl/systemctl-switch-root.c b/src/systemctl/systemctl-switch-root.c index 7fa8b26f59..5c71d3d97f 100644 --- a/src/systemctl/systemctl-switch-root.c +++ b/src/systemctl/systemctl-switch-root.c @@ -21,11 +21,11 @@ static int same_file_in_root( struct stat sta, stb; int r; - r = chase_symlinks_and_stat(a, root, CHASE_PREFIX_ROOT, NULL, &sta, NULL); + r = chase_symlinks_and_stat(a, root, CHASE_PREFIX_ROOT, NULL, &sta); if (r < 0) return r; - r = chase_symlinks_and_stat(b, root, CHASE_PREFIX_ROOT, NULL, &stb, NULL); + r = chase_symlinks_and_stat(b, root, CHASE_PREFIX_ROOT, NULL, &stb); if (r < 0) return r; From 47f0e1b5e04c27572b540ae4a86e522d268ffd3c Mon Sep 17 00:00:00 2001 From: Daan De Meyer Date: Mon, 13 Mar 2023 16:17:21 +0100 Subject: [PATCH 4/5] chase-symlinks: Rework open() functions and some chase flags Currently, when CHASE_PARENT is specified, we chase the parent directory of the symlink itself. Let's change this and chase the parent directory of the symlink target so that trying to open the actual file later with O_NOFOLLOW doesn't fail with ELOOP. To get the current behavior, callers can add CHASE_NOFOLLOW to chase the parent directory of the symlink itself. Currently, when CHASE_MKDIR_0755 is specified, we create all components of the path as directories. Instead, let's change the flag to only create parent directories and leave the final component of the PATH untouched. Also, allow CHASE_NONEXISTENT with CHASE_MKDIR_0755 now that it doesn't create all components anymore. Finally, rework chase_symlinks_and_open() and chase_symlinkat_at_and_open() to always chase the parent directory and use xopenat() to open the final component of the path. This allows us to pass O_CREAT to create the file or directory (O_DIRECTORY) if it is missing. If CHASE_PARENT is configured, we just reopen the parent directory that we chased. --- src/basic/chase-symlinks.c | 105 ++++++++++++++++++++----------------- src/basic/chase-symlinks.h | 6 ++- src/test/test-fs-util.c | 66 +++++++++++++++-------- 3 files changed, 106 insertions(+), 71 deletions(-) diff --git a/src/basic/chase-symlinks.c b/src/basic/chase-symlinks.c index 1b434950e6..b3bd635e86 100644 --- a/src/basic/chase-symlinks.c +++ b/src/basic/chase-symlinks.c @@ -98,20 +98,9 @@ int chase_symlinks_at( if ((flags & CHASE_STEP) && ret_fd) return -EINVAL; - if (FLAGS_SET(flags, CHASE_MKDIR_0755|CHASE_NONEXISTENT)) - return -EINVAL; - if (isempty(path)) path = "."; - if (flags & CHASE_PARENT) { - r = path_extract_directory(path, &buffer); - if (r == -EDESTADDRREQ) - path = "."; /* If we don't have a parent directory, fall back to the dir_fd directory. */ - else if (r < 0) - return r; - } - /* This function resolves symlinks of the path relative to the given directory file descriptor. If * CHASE_SYMLINKS_RESOLVE_IN_ROOT is specified and a directory file descriptor is provided, symlinks * are resolved relative to the given directory file descriptor. Otherwise, they are resolved @@ -292,6 +281,9 @@ int chase_symlinks_at( previous_stat = st; } + if (FLAGS_SET(flags, CHASE_PARENT) && isempty(todo)) + break; + close_and_replace(fd, fd_parent); continue; @@ -306,20 +298,23 @@ int chase_symlinks_at( if (!isempty(todo) && !path_is_safe(todo)) return r; - if (flags & CHASE_NONEXISTENT) { + if (FLAGS_SET(flags, CHASE_MKDIR_0755) && !isempty(todo)) { + child = open_mkdir_at(fd, first, O_CLOEXEC|O_PATH|O_EXCL, 0755); + if (child < 0) + return child; + } else if (FLAGS_SET(flags, CHASE_PARENT) && isempty(todo)) { + if (!path_extend(&done, first)) + return -ENOMEM; + + break; + } else if (flags & CHASE_NONEXISTENT) { if (!path_extend(&done, first, todo)) return -ENOMEM; exists = false; break; - } - - if (!(flags & CHASE_MKDIR_0755)) + } else return r; - - child = open_mkdir_at(fd, first, O_CLOEXEC|O_PATH|O_EXCL, 0755); - if (child < 0) - return child; } if (fstat(child, &st) < 0) @@ -394,11 +389,14 @@ int chase_symlinks_at( if (!path_extend(&done, first)) return -ENOMEM; + if (FLAGS_SET(flags, CHASE_PARENT) && isempty(todo)) + break; + /* And iterate again, but go one directory further down. */ close_and_replace(fd, child); } - if (flags & (CHASE_PARENT|CHASE_MKDIR_0755)) { + if (flags & CHASE_PARENT) { r = fd_verify_directory(fd); if (r < 0) return r; @@ -551,28 +549,38 @@ int chase_symlinks_and_open( char **ret_path) { _cleanup_close_ int path_fd = -EBADF; - _cleanup_free_ char *p = NULL; + _cleanup_free_ char *p = NULL, *fname = NULL; + mode_t mode = open_flags & O_DIRECTORY ? 0755 : 0644; + const char *q; int r; if (chase_flags & (CHASE_NONEXISTENT|CHASE_STEP)) return -EINVAL; if (empty_or_root(root) && !ret_path && - (chase_flags & (CHASE_NO_AUTOFS|CHASE_SAFE|CHASE_PROHIBIT_SYMLINKS|CHASE_PARENT|CHASE_MKDIR_0755)) == 0) { + (chase_flags & (CHASE_NO_AUTOFS|CHASE_SAFE|CHASE_PROHIBIT_SYMLINKS|CHASE_PARENT|CHASE_MKDIR_0755)) == 0) /* Shortcut this call if none of the special features of this call are requested */ - r = open(path, open_flags | (FLAGS_SET(chase_flags, CHASE_NOFOLLOW) ? O_NOFOLLOW : 0)); - if (r < 0) - return -errno; + return RET_NERRNO(xopenat(AT_FDCWD, path, + open_flags | (FLAGS_SET(chase_flags, CHASE_NOFOLLOW) ? O_NOFOLLOW : 0), + mode)); - return r; - } - - r = chase_symlinks(path, root, chase_flags, ret_path ? &p : NULL, &path_fd); + r = chase_symlinks(path, root, CHASE_PARENT|chase_flags, &p, &path_fd); if (r < 0) return r; assert(path_fd >= 0); - r = fd_reopen(path_fd, open_flags); + assert_se(q = path_startswith(p, empty_to_root(root))); + if (isempty(q)) + q = "."; + + r = path_extract_filename(q, &fname); + if (r < 0 && r != -EADDRNOTAVAIL) + return r; + + if (FLAGS_SET(chase_flags, CHASE_PARENT) || r == -EADDRNOTAVAIL) + r = fd_reopen(path_fd, open_flags); + else + r = xopenat(path_fd, fname, open_flags|O_NOFOLLOW, mode); if (r < 0) return r; @@ -748,31 +756,25 @@ int chase_symlinks_and_unlink( int unlink_flags, char **ret_path) { - _cleanup_free_ char *p = NULL, *rp = NULL, *fname = NULL; + _cleanup_free_ char *p = NULL, *fname = NULL; _cleanup_close_ int fd = -EBADF; int r; assert(path); - r = path_extract_filename(path, &fname); - if (r < 0) - return r; - - fd = chase_symlinks_and_open(path, root, chase_flags|CHASE_PARENT, O_PATH|O_DIRECTORY|O_CLOEXEC, ret_path ? &p : NULL); + fd = chase_symlinks_and_open(path, root, chase_flags|CHASE_PARENT|CHASE_NOFOLLOW, O_PATH|O_DIRECTORY|O_CLOEXEC, &p); if (fd < 0) return fd; - if (p) { - rp = path_join(p, fname); - if (!rp) - return -ENOMEM; - } + r = path_extract_filename(p, &fname); + if (r < 0) + return r; if (unlinkat(fd, fname, unlink_flags) < 0) return -errno; if (ret_path) - *ret_path = TAKE_PTR(rp); + *ret_path = TAKE_PTR(p); return 0; } @@ -785,7 +787,8 @@ int chase_symlinks_at_and_open( char **ret_path) { _cleanup_close_ int path_fd = -EBADF; - _cleanup_free_ char *p = NULL; + _cleanup_free_ char *p = NULL, *fname = NULL; + mode_t mode = open_flags & O_DIRECTORY ? 0755 : 0644; int r; if (chase_flags & (CHASE_NONEXISTENT|CHASE_STEP)) @@ -794,14 +797,22 @@ int chase_symlinks_at_and_open( if (dir_fd == AT_FDCWD && !ret_path && (chase_flags & (CHASE_NO_AUTOFS|CHASE_SAFE|CHASE_PROHIBIT_SYMLINKS|CHASE_PARENT|CHASE_MKDIR_0755)) == 0) /* Shortcut this call if none of the special features of this call are requested */ - return RET_NERRNO(openat(dir_fd, path, open_flags | (FLAGS_SET(chase_flags, CHASE_NOFOLLOW) ? O_NOFOLLOW : 0))); + return RET_NERRNO(xopenat(dir_fd, path, + open_flags | (FLAGS_SET(chase_flags, CHASE_NOFOLLOW) ? O_NOFOLLOW : 0), + mode)); - r = chase_symlinks_at(dir_fd, path, chase_flags, ret_path ? &p : NULL, &path_fd); + r = chase_symlinks_at(dir_fd, path, chase_flags|CHASE_PARENT, &p, &path_fd); if (r < 0) return r; - assert(path_fd >= 0); - r = fd_reopen(path_fd, open_flags); + r = path_extract_filename(p, &fname); + if (r < 0 && r != -EDESTADDRREQ) + return r; + + if (FLAGS_SET(chase_flags, CHASE_PARENT) || r == -EDESTADDRREQ) + r = fd_reopen(path_fd, open_flags); + else + r = xopenat(path_fd, fname, open_flags|O_NOFOLLOW, mode); if (r < 0) return r; diff --git a/src/basic/chase-symlinks.h b/src/basic/chase-symlinks.h index 26e8cfceca..448471fa6d 100644 --- a/src/basic/chase-symlinks.h +++ b/src/basic/chase-symlinks.h @@ -20,8 +20,10 @@ typedef enum ChaseSymlinksFlags { CHASE_AT_RESOLVE_IN_ROOT = 1 << 8, /* Same as openat2()'s RESOLVE_IN_ROOT flag, symlinks are resolved * relative to the given directory fd instead of root. */ CHASE_PROHIBIT_SYMLINKS = 1 << 9, /* Refuse all symlinks */ - CHASE_PARENT = 1 << 10, /* Chase the parent directory of the given path. */ - CHASE_MKDIR_0755 = 1 << 11, /* Create any missing directories in the given path. */ + 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. */ + CHASE_MKDIR_0755 = 1 << 11, /* Create any missing parent directories in the given path. */ } ChaseSymlinksFlags; bool unsafe_transition(const struct stat *a, const struct stat *b); diff --git a/src/test/test-fs-util.c b/src/test/test-fs-util.c index 065ad3c6e7..a18dc00fc5 100644 --- a/src/test/test-fs-util.c +++ b/src/test/test-fs-util.c @@ -331,10 +331,10 @@ TEST(chase_symlinks) { } assert_se(lstat(p, &st) >= 0); - r = chase_symlinks_and_unlink(p, NULL, 0, 0, &result); + r = chase_symlinks_and_unlink(p, NULL, 0, 0, &result); + assert_se(r == 0); assert_se(path_equal(result, p)); result = mfree(result); - assert_se(r == 0); assert_se(lstat(p, &st) == -1 && errno == ENOENT); /* Test CHASE_NOFOLLOW */ @@ -426,18 +426,6 @@ TEST(chase_symlinks) { assert_se(chase_symlinks("top/dot/dot", temp, CHASE_PREFIX_ROOT|CHASE_PROHIBIT_SYMLINKS, NULL, NULL) == -EREMCHG); assert_se(chase_symlinks("top/dot/dot", temp, CHASE_PREFIX_ROOT|CHASE_PROHIBIT_SYMLINKS|CHASE_WARN, NULL, NULL) == -EREMCHG); - /* Test CHASE_PARENT */ - - assert_se(chase_symlinks("/chase/parent", temp, CHASE_PREFIX_ROOT|CHASE_PARENT|CHASE_NONEXISTENT, &result, NULL) >= 0); - p = strjoina(temp, "/chase"); - assert_se(streq(p, result)); - result = mfree(result); - assert_se(chase_symlinks("/chase", temp, CHASE_PREFIX_ROOT|CHASE_PARENT|CHASE_NONEXISTENT, &result, NULL) >= 0); - assert_se(streq(temp, result)); - result = mfree(result); - assert_se(chase_symlinks("/", temp, CHASE_PREFIX_ROOT|CHASE_PARENT|CHASE_NONEXISTENT, NULL, NULL) == -EADDRNOTAVAIL); - assert_se(chase_symlinks(".", temp, CHASE_PREFIX_ROOT|CHASE_PARENT|CHASE_NONEXISTENT, NULL, NULL) == -EADDRNOTAVAIL); - cleanup: assert_se(rm_rf(temp, REMOVE_ROOT|REMOVE_PHYSICAL) >= 0); } @@ -479,23 +467,51 @@ TEST(chase_symlinks_at) { /* Test CHASE_PARENT */ - assert_se(chase_symlinks_at(tfd, "chase/parent", CHASE_NONEXISTENT|CHASE_PARENT, &result, NULL) >= 0); - assert_se(streq(result, "chase")); + assert_se((fd = open_mkdir_at(tfd, "chase", O_CLOEXEC, 0755)) >= 0); + assert_se(symlinkat("/def", fd, "parent") >= 0); + fd = safe_close(fd); + + /* Make sure that when we chase a symlink parent directory, that we chase the parent directory of the + * symlink target and not the symlink itself. But if we add CHASE_NOFOLLOW, we get the parent + * directory of the symlink itself. */ + + assert_se(chase_symlinks_at(tfd, "chase/parent", CHASE_PARENT|CHASE_AT_RESOLVE_IN_ROOT, &result, &fd) >= 0); + assert_se(faccessat(fd, "def", F_OK, 0) >= 0); + assert_se(streq(result, "def")); + fd = safe_close(fd); result = mfree(result); - assert_se(chase_symlinks_at(tfd, "chase", CHASE_NONEXISTENT|CHASE_PARENT, &result, NULL) >= 0); + assert_se(chase_symlinks_at(tfd, "chase/parent", CHASE_AT_RESOLVE_IN_ROOT|CHASE_PARENT|CHASE_NOFOLLOW, &result, &fd) >= 0); + assert_se(faccessat(fd, "parent", F_OK, AT_SYMLINK_NOFOLLOW) >= 0); + assert_se(streq(result, "chase/parent")); + fd = safe_close(fd); + result = mfree(result); + + assert_se(chase_symlinks_at(tfd, "chase", CHASE_PARENT|CHASE_AT_RESOLVE_IN_ROOT, &result, &fd) >= 0); + assert_se(faccessat(fd, "chase", F_OK, 0) >= 0); + assert_se(streq(result, "chase")); + fd = safe_close(fd); + result = mfree(result); + + assert_se(chase_symlinks_at(tfd, "/", CHASE_PARENT|CHASE_AT_RESOLVE_IN_ROOT, &result, NULL) >= 0); + assert_se(streq(result, ".")); + result = mfree(result); + + assert_se(chase_symlinks_at(tfd, ".", CHASE_PARENT|CHASE_AT_RESOLVE_IN_ROOT, &result, NULL) >= 0); assert_se(streq(result, ".")); result = mfree(result); /* Test CHASE_MKDIR_0755 */ - assert_se(chase_symlinks_at(tfd, "m/k/d/i/r", CHASE_MKDIR_0755, &result, NULL) >= 0); - assert_se(faccessat(tfd, "m/k/d/i/r", F_OK, 0) >= 0); + assert_se(chase_symlinks_at(tfd, "m/k/d/i/r", CHASE_MKDIR_0755|CHASE_NONEXISTENT, &result, NULL) >= 0); + assert_se(faccessat(tfd, "m/k/d/i", F_OK, 0) >= 0); + assert_se(RET_NERRNO(faccessat(tfd, "m/k/d/i/r", F_OK, 0)) == -ENOENT); assert_se(streq(result, "m/k/d/i/r")); result = mfree(result); - assert_se(chase_symlinks_at(tfd, "m/../q", CHASE_MKDIR_0755, &result, NULL) >= 0); - assert_se(faccessat(tfd, "q", F_OK, 0) >= 0); + assert_se(chase_symlinks_at(tfd, "m/../q", CHASE_MKDIR_0755|CHASE_NONEXISTENT, &result, NULL) >= 0); + assert_se(faccessat(tfd, "m", F_OK, 0) >= 0); + assert_se(RET_NERRNO(faccessat(tfd, "q", F_OK, 0)) == -ENOENT); assert_se(streq(result, "q")); result = mfree(result); @@ -503,8 +519,14 @@ TEST(chase_symlinks_at) { /* Test chase_symlinks_at_and_open() */ - fd = chase_symlinks_at_and_open(tfd, "o/p/e/n", CHASE_MKDIR_0755, O_CLOEXEC, NULL); + fd = chase_symlinks_at_and_open(tfd, "o/p/e/n/f/i/l/e", CHASE_MKDIR_0755, O_CREAT|O_EXCL|O_CLOEXEC, NULL); assert_se(fd >= 0); + assert_se(fd_verify_regular(fd) >= 0); + fd = safe_close(fd); + + fd = chase_symlinks_at_and_open(tfd, "o/p/e/n/d/i/r", CHASE_MKDIR_0755, O_DIRECTORY|O_CREAT|O_EXCL|O_CLOEXEC, NULL); + assert_se(fd >= 0); + assert_se(fd_verify_directory(fd) >= 0); fd = safe_close(fd); } From ea8282b6fc90a80f48ed66fead1e662ddb9050b9 Mon Sep 17 00:00:00 2001 From: Daan De Meyer Date: Tue, 14 Mar 2023 13:52:50 +0100 Subject: [PATCH 5/5] chase-symlinks: Add more assertions Let's turn some runtime errors into assertions and add a few new assertions. --- src/basic/chase-symlinks.c | 31 +++++++++++++------------------ 1 file changed, 13 insertions(+), 18 deletions(-) diff --git a/src/basic/chase-symlinks.c b/src/basic/chase-symlinks.c index b3bd635e86..5c2b56ea20 100644 --- a/src/basic/chase-symlinks.c +++ b/src/basic/chase-symlinks.c @@ -92,11 +92,11 @@ int chase_symlinks_at( assert(dir_fd >= 0 || dir_fd == AT_FDCWD); /* Either the file may be missing, or we return an fd to the final object, but both make no sense */ - if ((flags & CHASE_NONEXISTENT) && ret_fd) - return -EINVAL; + if ((flags & CHASE_NONEXISTENT)) + assert(!ret_fd); - if ((flags & CHASE_STEP) && ret_fd) - return -EINVAL; + if ((flags & CHASE_STEP)) + assert(!ret_fd); if (isempty(path)) path = "."; @@ -554,8 +554,7 @@ int chase_symlinks_and_open( const char *q; int r; - if (chase_flags & (CHASE_NONEXISTENT|CHASE_STEP)) - return -EINVAL; + assert(!(chase_flags & (CHASE_NONEXISTENT|CHASE_STEP))); if (empty_or_root(root) && !ret_path && (chase_flags & (CHASE_NO_AUTOFS|CHASE_SAFE|CHASE_PROHIBIT_SYMLINKS|CHASE_PARENT|CHASE_MKDIR_0755)) == 0) @@ -602,10 +601,8 @@ int chase_symlinks_and_opendir( DIR *d; int r; - if (!ret_dir) - return -EINVAL; - if (chase_flags & (CHASE_NONEXISTENT|CHASE_STEP)) - return -EINVAL; + assert(!(chase_flags & (CHASE_NONEXISTENT|CHASE_STEP))); + assert(ret_dir); if (empty_or_root(root) && !ret_path && (chase_flags & (CHASE_NO_AUTOFS|CHASE_SAFE|CHASE_PROHIBIT_SYMLINKS|CHASE_PARENT|CHASE_MKDIR_0755)) == 0) { @@ -646,9 +643,8 @@ int chase_symlinks_and_stat( int r; assert(path); - - if (chase_flags & (CHASE_NONEXISTENT|CHASE_STEP)) - return -EINVAL; + assert(!(chase_flags & (CHASE_NONEXISTENT|CHASE_STEP))); + assert(ret_stat); if (empty_or_root(root) && !ret_path && (chase_flags & (CHASE_NO_AUTOFS|CHASE_SAFE|CHASE_PROHIBIT_SYMLINKS|CHASE_PARENT|CHASE_MKDIR_0755)) == 0) { @@ -686,9 +682,7 @@ int chase_symlinks_and_access( int r; assert(path); - - if (chase_flags & (CHASE_NONEXISTENT|CHASE_STEP)) - return -EINVAL; + assert(!(chase_flags & (CHASE_NONEXISTENT|CHASE_STEP))); if (empty_or_root(root) && !ret_path && (chase_flags & (CHASE_NO_AUTOFS|CHASE_SAFE|CHASE_PROHIBIT_SYMLINKS|CHASE_PARENT|CHASE_MKDIR_0755)) == 0) { @@ -728,6 +722,7 @@ int chase_symlinks_and_fopen_unlocked( int mode_flags, r; assert(path); + assert(!(chase_flags & (CHASE_NONEXISTENT|CHASE_STEP|CHASE_PARENT))); assert(open_flags); assert(ret_file); @@ -761,6 +756,7 @@ int chase_symlinks_and_unlink( int r; assert(path); + assert(!(chase_flags & (CHASE_NONEXISTENT|CHASE_STEP|CHASE_PARENT))); fd = chase_symlinks_and_open(path, root, chase_flags|CHASE_PARENT|CHASE_NOFOLLOW, O_PATH|O_DIRECTORY|O_CLOEXEC, &p); if (fd < 0) @@ -791,8 +787,7 @@ int chase_symlinks_at_and_open( mode_t mode = open_flags & O_DIRECTORY ? 0755 : 0644; int r; - if (chase_flags & (CHASE_NONEXISTENT|CHASE_STEP)) - return -EINVAL; + assert(!(chase_flags & (CHASE_NONEXISTENT|CHASE_STEP))); if (dir_fd == AT_FDCWD && !ret_path && (chase_flags & (CHASE_NO_AUTOFS|CHASE_SAFE|CHASE_PROHIBIT_SYMLINKS|CHASE_PARENT|CHASE_MKDIR_0755)) == 0)