From 9d874aec451b591401d9b14cf8743b9d179159b2 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 6 Aug 2018 15:40:16 +0200 Subject: [PATCH 1/9] tmpfiles: use fd_get_path() less excessively fd_get_path() is an ugly API, as it creates ambiguities related to the " (deleted)" suffix /proc/$PID/fd/$FD shows. Let's use it a bit less excessively, and whenever we have a good valid path already, let's simply pass that along, instead of forgetting it in one stackframe and reacquiring it in the next. --- src/tmpfiles/tmpfiles.c | 100 +++++++++++++++++++++++----------------- 1 file changed, 58 insertions(+), 42 deletions(-) diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c index cfd9044c5b..614356113f 100644 --- a/src/tmpfiles/tmpfiles.c +++ b/src/tmpfiles/tmpfiles.c @@ -770,17 +770,21 @@ static bool hardlink_vulnerable(const struct stat *st) { return !S_ISDIR(st->st_mode) && st->st_nlink > 1 && dangerous_hardlinks(); } -static int fd_set_perms(Item *i, int fd, const struct stat *st) { - _cleanup_free_ char *path = NULL; +static int fd_set_perms(Item *i, int fd, const char *path, const struct stat *st) { + _cleanup_free_ char *buffer = NULL; struct stat stbuf; int r; assert(i); assert(fd); - r = fd_get_path(fd, &path); - if (r < 0) - return r; + if (!path) { + r = fd_get_path(fd, &buffer); + if (r < 0) + return log_error_errno(r, "Failed to get path: %m"); + + path = buffer; + } if (!i->mode_set && !i->uid_set && !i->gid_set) goto shortcut; @@ -897,7 +901,7 @@ static int path_set_perms(Item *i, const char *path) { if (fd < 0) return fd; - return fd_set_perms(i, fd, NULL); + return fd_set_perms(i, fd, path, NULL); } static int parse_xattrs_from_arg(Item *i) { @@ -938,18 +942,22 @@ static int parse_xattrs_from_arg(Item *i) { return 0; } -static int fd_set_xattrs(Item *i, int fd, const struct stat *st) { +static int fd_set_xattrs(Item *i, int fd, const char *path, const struct stat *st) { char procfs_path[STRLEN("/proc/self/fd/") + DECIMAL_STR_MAX(int)]; - _cleanup_free_ char *path = NULL; + _cleanup_free_ char *buffer = NULL; char **name, **value; int r; assert(i); assert(fd); - r = fd_get_path(fd, &path); - if (r < 0) - return r; + if (!path) { + r = fd_get_path(fd, &buffer); + if (r < 0) + return log_error_errno(r, "Failed to get path: %m"); + + path = buffer; + } xsprintf(procfs_path, "/proc/self/fd/%i", fd); @@ -972,7 +980,7 @@ static int path_set_xattrs(Item *i, const char *path) { if (fd < 0) return fd; - return fd_set_xattrs(i, fd, NULL); + return fd_set_xattrs(i, fd, path, NULL); } static int parse_acls_from_arg(Item *item) { @@ -1040,19 +1048,23 @@ static int path_set_acl(const char *path, const char *pretty, acl_type_t type, a } #endif -static int fd_set_acls(Item *item, int fd, const struct stat *st) { +static int fd_set_acls(Item *item, int fd, const char *path, const struct stat *st) { int r = 0; #if HAVE_ACL char procfs_path[STRLEN("/proc/self/fd/") + DECIMAL_STR_MAX(int)]; - _cleanup_free_ char *path = NULL; + _cleanup_free_ char *buffer = NULL; struct stat stbuf; assert(item); assert(fd); - r = fd_get_path(fd, &path); - if (r < 0) - return r; + if (!path) { + r = fd_get_path(fd, &buffer); + if (r < 0) + return log_error_errno(r, "Failed to get path: %m"); + + path = buffer; + } if (!st) { if (fstat(fd, &stbuf) < 0) @@ -1103,7 +1115,7 @@ static int path_set_acls(Item *item, const char *path) { if (fd < 0) return fd; - r = fd_set_acls(item, fd, NULL); + r = fd_set_acls(item, fd, path, NULL); #endif return r; } @@ -1207,9 +1219,9 @@ static int parse_attribute_from_arg(Item *item) { return 0; } -static int fd_set_attribute(Item *item, int fd, const struct stat *st) { +static int fd_set_attribute(Item *item, int fd, const char *path, const struct stat *st) { _cleanup_close_ int procfs_fd = -1; - _cleanup_free_ char *path = NULL; + _cleanup_free_ char *buffer = NULL; struct stat stbuf; unsigned f; int r; @@ -1217,9 +1229,13 @@ static int fd_set_attribute(Item *item, int fd, const struct stat *st) { if (!item->attribute_set || item->attribute_mask == 0) return 0; - r = fd_get_path(fd, &path); - if (r < 0) - return r; + if (!path) { + r = fd_get_path(fd, &buffer); + if (r < 0) + return log_error_errno(r, "Failed to get path: %m"); + + path = buffer; + } if (!st) { if (fstat(fd, &stbuf) < 0) @@ -1265,7 +1281,7 @@ static int path_set_attribute(Item *item, const char *path) { if (fd < 0) return fd; - return fd_set_attribute(item, fd, NULL); + return fd_set_attribute(item, fd, path, NULL); } static int write_one_file(Item *i, const char *path) { @@ -1303,7 +1319,7 @@ static int write_one_file(Item *i, const char *path) { if (r < 0) return log_error_errno(r, "Failed to write file \"%s\": %m", path); - return fd_set_perms(i, fd, NULL); + return fd_set_perms(i, fd, path, NULL); } static int create_file(Item *i, const char *path) { @@ -1370,7 +1386,7 @@ static int create_file(Item *i, const char *path) { } } - return fd_set_perms(i, fd, st); + return fd_set_perms(i, fd, path, st); } static int truncate_file(Item *i, const char *path) { @@ -1454,7 +1470,7 @@ static int truncate_file(Item *i, const char *path) { } } - return fd_set_perms(i, fd, st); + return fd_set_perms(i, fd, path, st); } static int copy_files(Item *i) { @@ -1506,7 +1522,7 @@ static int copy_files(Item *i) { if (fd < 0) return log_error_errno(errno, "Failed to openat(%s): %m", i->path); - return fd_set_perms(i, fd, NULL); + return fd_set_perms(i, fd, i->path, NULL); } typedef enum { @@ -1601,7 +1617,7 @@ static int create_directory(Item *i, const char *path) { if (fd < 0) return fd; - return fd_set_perms(i, fd, NULL); + return fd_set_perms(i, fd, path, NULL); } static int create_subvolume(Item *i, const char *path) { @@ -1633,8 +1649,8 @@ static int create_subvolume(Item *i, const char *path) { log_debug("Quota for subvolume \"%s\" already in place, no change made.", i->path); } - r = fd_set_perms(i, fd, NULL); - if (q < 0) + r = fd_set_perms(i, fd, path, NULL); + if (q < 0) /* prefer the quota change error from above */ return q; return r; @@ -1734,7 +1750,7 @@ static int create_device(Item *i, mode_t file_type) { if (fd < 0) return log_error_errno(errno, "Failed to openat(%s): %m", i->path); - return fd_set_perms(i, fd, NULL); + return fd_set_perms(i, fd, i->path, NULL); } static int create_fifo(Item *i, const char *path) { @@ -1790,13 +1806,13 @@ static int create_fifo(Item *i, const char *path) { if (fd < 0) return log_error_errno(fd, "Failed to openat(%s): %m", path); - return fd_set_perms(i, fd, NULL); + return fd_set_perms(i, fd, i->path, NULL); } -typedef int (*action_t)(Item *, const char *); -typedef int (*fdaction_t)(Item *, int fd, const struct stat *st); +typedef int (*action_t)(Item *i, const char *path); +typedef int (*fdaction_t)(Item *i, int fd, const char *path, const struct stat *st); -static int item_do(Item *i, int fd, fdaction_t action) { +static int item_do(Item *i, int fd, const char *path, fdaction_t action) { struct stat st; int r = 0, q; @@ -1810,7 +1826,7 @@ static int item_do(Item *i, int fd, fdaction_t action) { /* This returns the first error we run into, but nevertheless * tries to go on */ - r = action(i, fd, &st); + r = action(i, fd, path, &st); if (S_ISDIR(st.st_mode)) { char procfs_path[STRLEN("/proc/self/fd/") + DECIMAL_STR_MAX(int)]; @@ -1834,11 +1850,11 @@ static int item_do(Item *i, int fd, fdaction_t action) { continue; de_fd = openat(fd, de->d_name, O_NOFOLLOW|O_CLOEXEC|O_PATH); - if (de_fd >= 0) - /* pass ownership of dirent fd over */ - q = item_do(i, de_fd, action); + if (de_fd < 0) + q = log_error_errno(errno, "Failed to open() file '%s': %m", de->d_name); else - q = -errno; + /* Pass ownership of dirent fd over */ + q = item_do(i, de_fd, NULL, action); if (q < 0 && r == 0) r = q; @@ -1894,7 +1910,7 @@ static int glob_item_recursively(Item *i, fdaction_t action) { continue; } - k = item_do(i, fd, action); + k = item_do(i, fd, *fn, action); if (k < 0 && r == 0) r = k; From 09f12798c94ab59690c2103ca5f883b50107ca4b Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 6 Aug 2018 15:44:24 +0200 Subject: [PATCH 2/9] tmpfiles: add log message where we previously failed silently --- src/tmpfiles/tmpfiles.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c index 614356113f..c29524f443 100644 --- a/src/tmpfiles/tmpfiles.c +++ b/src/tmpfiles/tmpfiles.c @@ -1259,7 +1259,7 @@ static int fd_set_attribute(Item *item, int fd, const char *path, const struct s procfs_fd = fd_reopen(fd, O_RDONLY|O_CLOEXEC|O_NOATIME); if (procfs_fd < 0) - return -errno; + return log_error_errno(errno, "Failed to re-open '%s': %m", path); r = chattr_fd(procfs_fd, f, item->attribute_mask); if (r < 0) @@ -1601,7 +1601,8 @@ static int create_directory_or_subvolume(const char *path, mode_t mode, bool sub r = openat(pfd, basename(path), O_NOCTTY|O_CLOEXEC|O_DIRECTORY); if (r < 0) - return -errno; + return log_error_errno(errno, "Failed to open directory '%s': %m", basename(path)); + return r; } @@ -1820,7 +1821,7 @@ static int item_do(Item *i, int fd, const char *path, fdaction_t action) { assert(fd >= 0); if (fstat(fd, &st) < 0) { - r = -errno; + r = log_error_errno(errno, "fstat() on file failed: %m"); goto finish; } @@ -1839,7 +1840,9 @@ static int item_do(Item *i, int fd, const char *path, fdaction_t action) { d = opendir(procfs_path); if (!d) { - r = r ?: -errno; + log_error_errno(errno, "Failed to opendir() '%s': %m", procfs_path); + if (r == 0) + r = -errno; goto finish; } @@ -1906,7 +1909,9 @@ static int glob_item_recursively(Item *i, fdaction_t action) { fd = open(*fn, O_CLOEXEC|O_NOFOLLOW|O_PATH); if (fd < 0) { - r = r ?: -errno; + log_error_errno(errno, "Opening '%s' failed: %m", *fn); + if (r == 0) + r = -errno; continue; } From 15b45fac8728c6d0b538f047c6966f570c325bbb Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 6 Aug 2018 15:44:47 +0200 Subject: [PATCH 3/9] tmpfiles: clarify that we ignore file attribute setting errors --- src/tmpfiles/tmpfiles.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c index c29524f443..ed7022fbb4 100644 --- a/src/tmpfiles/tmpfiles.c +++ b/src/tmpfiles/tmpfiles.c @@ -1265,7 +1265,7 @@ static int fd_set_attribute(Item *item, int fd, const char *path, const struct s if (r < 0) log_full_errno(IN_SET(r, -ENOTTY, -EOPNOTSUPP) ? LOG_DEBUG : LOG_WARNING, r, - "Cannot set file attribute for '%s', value=0x%08x, mask=0x%08x: %m", + "Cannot set file attribute for '%s', value=0x%08x, mask=0x%08x, ignoring: %m", path, item->attribute_value, item->attribute_mask); return 0; From a5df05ff202c08728bca18d7de7d5ef013d5a26a Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 6 Aug 2018 15:46:01 +0200 Subject: [PATCH 4/9] tmpfiles: use correct error variable --- src/tmpfiles/tmpfiles.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c index ed7022fbb4..7af1359c92 100644 --- a/src/tmpfiles/tmpfiles.c +++ b/src/tmpfiles/tmpfiles.c @@ -1805,7 +1805,7 @@ static int create_fifo(Item *i, const char *path) { fd = openat(pfd, bn, O_NOFOLLOW|O_CLOEXEC|O_PATH); if (fd < 0) - return log_error_errno(fd, "Failed to openat(%s): %m", path); + return log_error_errno(errno, "Failed to openat(%s): %m", path); return fd_set_perms(i, fd, i->path, NULL); } From 713998cfc8c2f08d096edf047bc0707c3e53286d Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 6 Aug 2018 15:46:32 +0200 Subject: [PATCH 5/9] tmpfiles: reindent one comment less weirdly --- src/tmpfiles/tmpfiles.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c index 7af1359c92..7b98e66cfb 100644 --- a/src/tmpfiles/tmpfiles.c +++ b/src/tmpfiles/tmpfiles.c @@ -2063,10 +2063,8 @@ static int create_item(Item *i) { case CREATE_BLOCK_DEVICE: case CREATE_CHAR_DEVICE: if (have_effective_cap(CAP_MKNOD) == 0) { - /* In a container we lack CAP_MKNOD. We - shouldn't attempt to create the device node in - that case to avoid noise, and we don't support - virtualized devices in containers anyway. */ + /* In a container we lack CAP_MKNOD. We shouldn't attempt to create the device node in that + * case to avoid noise, and we don't support virtualized devices in containers anyway. */ log_debug("We lack CAP_MKNOD, skipping creation of device node %s.", i->path); return 0; From 2e6e61688748473c4230ca49b402aea2bec9b8ab Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 6 Aug 2018 19:32:00 +0200 Subject: [PATCH 6/9] btrfs-util: unfuck tmpfiles' subvol creation tmpfiles now passes an O_PATH fd to btrfs_subvol_make_fd() under the assumption it will accept it like mkdirat() does. So far this assumption was wrong, let's correct that. Without that tmpfiles' on btrfs file systems failed systematically... --- src/basic/btrfs-util.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/basic/btrfs-util.c b/src/basic/btrfs-util.c index abd34824e7..376844b7b1 100644 --- a/src/basic/btrfs-util.c +++ b/src/basic/btrfs-util.c @@ -118,6 +118,7 @@ int btrfs_is_subvol(const char *path) { int btrfs_subvol_make_fd(int fd, const char *subvolume) { struct btrfs_ioctl_vol_args args = {}; + _cleanup_close_ int real_fd = -1; int r; assert(subvolume); @@ -126,6 +127,20 @@ int btrfs_subvol_make_fd(int fd, const char *subvolume) { if (r < 0) return r; + r = fcntl(fd, F_GETFL); + if (r < 0) + return -errno; + if (FLAGS_SET(r, O_PATH)) { + /* An O_PATH fd was specified, let's convert here to a proper one, as btrfs ioctl's can't deal with + * O_PATH. */ + + real_fd = fd_reopen(fd, O_RDONLY|O_CLOEXEC|O_DIRECTORY); + if (real_fd < 0) + return real_fd; + + fd = real_fd; + } + strncpy(args.name, subvolume, sizeof(args.name)-1); if (ioctl(fd, BTRFS_IOC_SUBVOL_CREATE, &args) < 0) From c3e03778d64de6294d5030c822774c67ce416fe8 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 6 Aug 2018 20:19:52 +0200 Subject: [PATCH 7/9] tmpfiles: return correct error variable after fd_reopen() --- src/tmpfiles/tmpfiles.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c index 7b98e66cfb..730f5bbc6d 100644 --- a/src/tmpfiles/tmpfiles.c +++ b/src/tmpfiles/tmpfiles.c @@ -1259,7 +1259,7 @@ static int fd_set_attribute(Item *item, int fd, const char *path, const struct s procfs_fd = fd_reopen(fd, O_RDONLY|O_CLOEXEC|O_NOATIME); if (procfs_fd < 0) - return log_error_errno(errno, "Failed to re-open '%s': %m", path); + return log_error_errno(procfs_fd, "Failed to re-open '%s': %m", path); r = chattr_fd(procfs_fd, f, item->attribute_mask); if (r < 0) From 721474fe79b4c4a7e8ab283c1749440abe09f78c Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 6 Aug 2018 20:56:45 +0200 Subject: [PATCH 8/9] test: don't use "nobody:nogroup" for tests This user/group doesn't apply to Fedora. Let's use daemon:daemon instead like the other tests, as it actually tends to exist everywhere. --- test/TEST-22-TMPFILES/test-03.sh | 52 ++++++++++++++++---------------- test/TEST-22-TMPFILES/test-04.sh | 8 ++--- 2 files changed, 30 insertions(+), 30 deletions(-) diff --git a/test/TEST-22-TMPFILES/test-03.sh b/test/TEST-22-TMPFILES/test-03.sh index 68f34cb392..39a4badc38 100755 --- a/test/TEST-22-TMPFILES/test-03.sh +++ b/test/TEST-22-TMPFILES/test-03.sh @@ -27,12 +27,12 @@ test "$(< /tmp/f/2)" = "This string should be written" ### The perms are supposed to be updated even if the file already exists. systemd-tmpfiles --create - </tmp/F/truncated-with-content systemd-tmpfiles --create - < Date: Mon, 6 Aug 2018 20:57:26 +0200 Subject: [PATCH 9/9] test: make TEST-22 easier to debug, by outputting to /dev/console --- test/TEST-22-TMPFILES/testsuite.service | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/TEST-22-TMPFILES/testsuite.service b/test/TEST-22-TMPFILES/testsuite.service index 3a44b41989..123b12790f 100644 --- a/test/TEST-22-TMPFILES/testsuite.service +++ b/test/TEST-22-TMPFILES/testsuite.service @@ -6,3 +6,5 @@ After=multi-user.target WorkingDirectory=/testsuite ExecStart=/testsuite/run-tmpfiles-tests.sh Type=oneshot +StandardOutput=tty +StandardError=tty