copy: rework copy_file_atomic() to copy the specified file via O_TMPFILE if possible

diff --git a/TODO b/TODO
index 2cf8a4cfc1e3fe3eda3af8c1495535721cf8be4f..7b940d5c4221560e11d473546b46ab9c2bd3fcc4 100644 (file)
--- 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
 
 
 * 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
 
 * 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 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
 * 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.
 
   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.
 
 * 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 964613f8c74867ea0293bed989b29b60695be107..5b53f2e0fc3fefe57025cbeb80c29550b3f1402d 100644 (file)
--- 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 "io-util.h"
 //#include "macro.h"
 #include "missing.h"

-//#include "mount-util.h"

 //#include "string-util.h"
 #include "strv.h"
 #include "time-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"
 
 #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,
 
 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,
                 int dt,
                 const char *to,
                 dev_t original_device,

-                unsigned depth_left,

                 uid_t override_uid,
                 gid_t override_gid,
                 CopyFlags copy_flags) {
                 uid_t override_uid,
                 gid_t override_gid,
                 CopyFlags copy_flags) {


@@ -502,9 +495,6 @@ static int fd_copy_directory(
         assert(st);
         assert(to);
 
         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
         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;
                 }
 
                         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);
                         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))
                 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))
         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))
         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;
 
         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) {
 }
 
 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;
 
         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) {
 }
 
 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) {
 }
 
 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);
 
         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 (r < 0)
                 return r;
 

+        if (fchmod(fdt, mode) < 0)
+                return -errno;
+

         if (copy_flags & COPY_REPLACE) {
         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)
                 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;
 }
 
         return 0;
 }
 

diff --git a/src/test/test-copy.c b/src/test/test-copy.c
index 0d39d41bbacc0b0b4b676c4261c0b65225a5d357..9dc0ddf9580b1e896874a14c4be998f6c300cc08 100644 (file)
--- 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);
 }
 
         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[]) {
 int main(int argc, char *argv[]) {

+        log_set_max_level(LOG_DEBUG);
+

 #if 0 /// UNNEEDED by elogind
         test_copy_file();
         test_copy_file_fd();
 #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_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;
 }
 
         return 0;
 }