From 3f87ae2530d8bb022f54526c3a568165a34acf05 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 5 Jun 2018 16:52:22 +0200 Subject: [PATCH] copy: rework copy_file_atomic() to copy the specified file via O_TMPFILE if possible --- TODO | 7 +-- src/basic/copy.c | 103 +++++++++++++++++++------------------------ src/test/test-copy.c | 21 +++++++++ 3 files changed, 68 insertions(+), 63 deletions(-) diff --git a/TODO b/TODO index 2cf8a4cfc..7b940d5c4 100644 --- a/TODO +++ b/TODO @@ -14,6 +14,8 @@ Janitorial Clean-ups: * Rearrange tests so that the various test-xyz.c match a specific src/basic/xyz.c again +* copy.c: set the right chattrs before copying files and others after + * rework mount.c and swap.c to follow proper state enumeration/deserialization semantics, like we do for device.c now @@ -25,8 +27,6 @@ Features: * Add OnTimezoneChange= and OnTimeChange= stanzas to .timer units in order to schedule events based on time and timezone changes. -* add O_TMPFILE support to copy_file_atomic() - * nspawn: greater control over selinux label? * cgroups: figure out if we can somehow communicate in a cleaner way whether a @@ -36,9 +36,6 @@ Features: should be revisited to make clearer and also work if the payload elogind runs with full privs and without userns. -* portables: introduce a new unit file directory /etc/elogind/system.attached/ - or so, where we attach portable services to - * cgroups: use inotify to get notified when somebody else modifies cgroups owned by us, then log a friendly warning. diff --git a/src/basic/copy.c b/src/basic/copy.c index 964613f8c..5b53f2e0f 100644 --- a/src/basic/copy.c +++ b/src/basic/copy.c @@ -29,7 +29,6 @@ #include "io-util.h" //#include "macro.h" #include "missing.h" -//#include "mount-util.h" //#include "string-util.h" #include "strv.h" #include "time-util.h" @@ -37,12 +36,7 @@ #include "user-util.h" //#include "xattr-util.h" -#define COPY_BUFFER_SIZE (16U*1024U) - -/* A safety net for descending recursively into file system trees to copy. On Linux PATH_MAX is 4096, which means the - * deepest valid path one can build is around 2048, which we hence use as a safety net here, to not spin endlessly in - * case of bind mount cycles and suchlike. */ -#define COPY_DEPTH_MAX 2048U +#define COPY_BUFFER_SIZE (16*1024u) static ssize_t try_copy_file_range( int fd_in, loff_t *off_in, @@ -488,7 +482,6 @@ static int fd_copy_directory( int dt, const char *to, dev_t original_device, - unsigned depth_left, uid_t override_uid, gid_t override_gid, CopyFlags copy_flags) { @@ -502,9 +495,6 @@ static int fd_copy_directory( assert(st); assert(to); - if (depth_left == 0) - return -ENAMETOOLONG; - if (from) fdf = openat(df, from, O_RDONLY|O_DIRECTORY|O_CLOEXEC|O_NOCTTY|O_NOFOLLOW); else @@ -543,40 +533,13 @@ static int fd_copy_directory( continue; } - if (S_ISDIR(buf.st_mode)) { - /* - * Don't descend into directories on other file systems, if this is requested. We do a simple - * .st_dev check here, which basically comes for free. Note that we do this check only on - * directories, not other kind of file system objects, for two reason: - * - * • The kernel's overlayfs pseudo file system that overlays multiple real file systems - * propagates the .st_dev field of the file system a file originates from all the way up - * through the stack to stat(). It doesn't do that for directories however. This means that - * comparing .st_dev on non-directories suggests that they all are mount points. To avoid - * confusion we hence avoid relying on this check for regular files. - * - * • The main reason we do this check at all is to protect ourselves from bind mount cycles, - * where we really want to avoid descending down in all eternity. However the .st_dev check - * is usually not sufficient for this protection anyway, as bind mount cycles from the same - * file system onto itself can't be detected that way. (Note we also do a recursion depth - * check, which is probably the better protection in this regard, which is why - * COPY_SAME_MOUNT is optional). - */ - - if (FLAGS_SET(copy_flags, COPY_SAME_MOUNT)) { - if (buf.st_dev != original_device) - continue; - - r = fd_is_mount_point(dirfd(d), de->d_name, 0); - if (r < 0) - return r; - if (r > 0) - continue; - } + if (buf.st_dev != original_device) + continue; - q = fd_copy_directory(dirfd(d), de->d_name, &buf, fdt, de->d_name, original_device, depth_left-1, override_uid, override_gid, copy_flags); - } else if (S_ISREG(buf.st_mode)) + if (S_ISREG(buf.st_mode)) q = fd_copy_regular(dirfd(d), de->d_name, &buf, fdt, de->d_name, override_uid, override_gid, copy_flags); + else if (S_ISDIR(buf.st_mode)) + q = fd_copy_directory(dirfd(d), de->d_name, &buf, fdt, de->d_name, original_device, override_uid, override_gid, copy_flags); else if (S_ISLNK(buf.st_mode)) q = fd_copy_symlink(dirfd(d), de->d_name, &buf, fdt, de->d_name, override_uid, override_gid, copy_flags); else if (S_ISFIFO(buf.st_mode)) @@ -626,7 +589,7 @@ int copy_tree_at(int fdf, const char *from, int fdt, const char *to, uid_t overr if (S_ISREG(st.st_mode)) return fd_copy_regular(fdf, from, &st, fdt, to, override_uid, override_gid, copy_flags); else if (S_ISDIR(st.st_mode)) - return fd_copy_directory(fdf, from, &st, fdt, to, st.st_dev, COPY_DEPTH_MAX, override_uid, override_gid, copy_flags); + return fd_copy_directory(fdf, from, &st, fdt, to, st.st_dev, override_uid, override_gid, copy_flags); else if (S_ISLNK(st.st_mode)) return fd_copy_symlink(fdf, from, &st, fdt, to, override_uid, override_gid, copy_flags); else if (S_ISFIFO(st.st_mode)) @@ -653,7 +616,7 @@ int copy_directory_fd(int dirfd, const char *to, CopyFlags copy_flags) { if (!S_ISDIR(st.st_mode)) return -ENOTDIR; - return fd_copy_directory(dirfd, NULL, &st, AT_FDCWD, to, st.st_dev, COPY_DEPTH_MAX, UID_INVALID, GID_INVALID, copy_flags); + return fd_copy_directory(dirfd, NULL, &st, AT_FDCWD, to, st.st_dev, UID_INVALID, GID_INVALID, copy_flags); } int copy_directory(const char *from, const char *to, CopyFlags copy_flags) { @@ -668,7 +631,7 @@ int copy_directory(const char *from, const char *to, CopyFlags copy_flags) { if (!S_ISDIR(st.st_mode)) return -ENOTDIR; - return fd_copy_directory(AT_FDCWD, from, &st, AT_FDCWD, to, st.st_dev, COPY_DEPTH_MAX, UID_INVALID, GID_INVALID, copy_flags); + return fd_copy_directory(AT_FDCWD, from, &st, AT_FDCWD, to, st.st_dev, UID_INVALID, GID_INVALID, copy_flags); } int copy_file_fd(const char *from, int fdt, CopyFlags copy_flags) { @@ -721,31 +684,55 @@ int copy_file(const char *from, const char *to, int flags, mode_t mode, unsigned } int copy_file_atomic(const char *from, const char *to, mode_t mode, unsigned chattr_flags, CopyFlags copy_flags) { - _cleanup_free_ char *t = NULL; + _cleanup_(unlink_and_freep) char *t = NULL; + _cleanup_close_ int fdt = -1; int r; assert(from); assert(to); - r = tempfn_random(to, NULL, &t); - if (r < 0) - return r; + /* We try to use O_TMPFILE here to create the file if we can. Note that that only works if COPY_REPLACE is not + * set though as we need to use linkat() for linking the O_TMPFILE file into the file system but that system + * call can't replace existing files. Hence, if COPY_REPLACE is set we create a temporary name in the file + * system right-away and unconditionally which we then can renameat() to the right name after we completed + * writing it. */ + + if (copy_flags & COPY_REPLACE) { + r = tempfn_random(to, NULL, &t); + if (r < 0) + return r; - r = copy_file(from, t, O_NOFOLLOW|O_EXCL, mode, chattr_flags, copy_flags); + fdt = open(t, O_CREAT|O_EXCL|O_NOFOLLOW|O_NOCTTY|O_WRONLY|O_CLOEXEC, 0600); + if (fdt < 0) { + t = mfree(t); + return -errno; + } + } else { + fdt = open_tmpfile_linkable(to, O_WRONLY|O_CLOEXEC, &t); + if (fdt < 0) + return fdt; + } + + if (chattr_flags != 0) + (void) chattr_fd(fdt, chattr_flags, (unsigned) -1); + + r = copy_file_fd(from, fdt, copy_flags); if (r < 0) return r; + if (fchmod(fdt, mode) < 0) + return -errno; + if (copy_flags & COPY_REPLACE) { - r = renameat(AT_FDCWD, t, AT_FDCWD, to); + if (renameat(AT_FDCWD, t, AT_FDCWD, to) < 0) + return -errno; + } else { + r = link_tmpfile(fdt, t, to); if (r < 0) - r = -errno; - } else - r = rename_noreplace(AT_FDCWD, t, AT_FDCWD, to); - if (r < 0) { - (void) unlink(t); - return r; + return r; } + t = mfree(t); return 0; } diff --git a/src/test/test-copy.c b/src/test/test-copy.c index 0d39d41bb..9dc0ddf95 100644 --- a/src/test/test-copy.c +++ b/src/test/test-copy.c @@ -242,7 +242,27 @@ static void test_copy_bytes_regular_file(const char *src, bool try_reflink, uint unlink(fn3); } +static void test_copy_atomic(void) { + _cleanup_(rm_rf_physical_and_freep) char *p = NULL; + const char *q; + int r; + + assert_se(mkdtemp_malloc(NULL, &p) >= 0); + + q = strjoina(p, "/fstab"); + + r = copy_file_atomic("/etc/fstab", q, 0644, 0, COPY_REFLINK); + if (r == -ENOENT) + return; + + assert_se(copy_file_atomic("/etc/fstab", q, 0644, 0, COPY_REFLINK) == -EEXIST); + + assert_se(copy_file_atomic("/etc/fstab", q, 0644, 0, COPY_REPLACE) >= 0); +} + int main(int argc, char *argv[]) { + log_set_max_level(LOG_DEBUG); + #if 0 /// UNNEEDED by elogind test_copy_file(); test_copy_file_fd(); @@ -255,6 +275,7 @@ int main(int argc, char *argv[]) { test_copy_bytes_regular_file(argv[0], true, 1000); test_copy_bytes_regular_file(argv[0], false, 32000); /* larger than copy buffer size */ test_copy_bytes_regular_file(argv[0], true, 32000); + test_copy_atomic(); return 0; } -- 2.30.2