fdio: Use O_TMPFILE + rename-overwrite for regfile copies

I was working on rpm-ostree unified core, and hit the fact that
`glnx_file_copy_at()` had the same bug with `fsetxattr()` and files whose mode
is <= `0400` (e.g. `000` in the case of `/etc/shadow`) that libostree did a
while ago.  Basically, Linux currently allows `write()` on non-writable open files
but not `fsetxattr()`.  This situation is masked for privileged (i.e.
`CAP_DAC_OVERRIDE`) code.

Looking at this, I think it's cleaner to convert to `O_TMPFILE` here,
since that code already handles setting the tmpfile to mode `0600`.  Now,
this *is* a behavior change in the corner case of existing files which
are symbolic links.  Previously we'd do an `open(O_TRUNC)` which would follow
the link.

But in the big picture, I think the use cases for `open(O_TRUNC)` are really
rare - I audited all callers of this in ostree/rpm-ostree/flatpak, and all of
them will be fine with this behavior change. For example, the ostree `/etc`
merge code already explicitly unlinks the target beforehand. Other cases like
supporting `repo/pubring.gpg` in an ostree repo being a symlink...eh, just no.

Making this change allows us to convert to new style, and brings all of the
general benefits of using `O_TMPFILE` too.

2 files changed, 118 insertions, 76 deletions

diff --git a/glnx-fdio.c b/glnx-fdio.c
index 0046807..8093836 100644
--- a/glnx-fdio.c
+++ b/glnx-fdio.c
@@ -872,11 +872,15 @@ glnx_regfile_copy_bytes (int fdf, int fdt, off_t max_bytes)
  * @cancellable: cancellable
  * @error: Error
  *
- * Perform a full copy of the regular file or
- * symbolic link from @src_subpath to @dest_subpath.
+ * Perform a full copy of the regular file or symbolic link from @src_subpath to
+ * @dest_subpath; if @src_subpath is anything other than a regular file or
+ * symbolic link, an error will be returned.
  *
- * If @src_subpath is anything other than a regular
- * file or symbolic link, an error will be returned.
+ * If the source is a regular file and the destination exists as a symbolic
+ * link, the symbolic link will not be followed; rather the link itself will be
+ * replaced. Related to this: for regular files, when `GLNX_FILE_COPY_OVERWRITE`
+ * is specified, this function always uses `O_TMPFILE` (if available) and does a
+ * rename-into-place rather than `open(O_TRUNC)`.
  */
 gboolean
 glnx_file_copy_at (int                   src_dfd,
@@ -888,31 +892,23 @@ glnx_file_copy_at (int                   src_dfd,
                    GCancellable         *cancellable,
                    GError              **error)
 {
-  gboolean ret = FALSE;
-  int r;
-  int dest_open_flags;
-  struct timespec ts[2];
-  glnx_fd_close int src_fd = -1;
-  glnx_fd_close int dest_fd = -1;
-  struct stat local_stbuf;
-
-  if (g_cancellable_set_error_if_cancelled (cancellable, error))
-    goto out;
-
+  /* Canonicalize dfds */
   src_dfd = glnx_dirfd_canonicalize (src_dfd);
   dest_dfd = glnx_dirfd_canonicalize (dest_dfd);
 
+  if (g_cancellable_set_error_if_cancelled (cancellable, error))
+    return FALSE;
+
   /* Automatically do stat() if no stat buffer was supplied */
+  struct stat local_stbuf;
   if (!src_stbuf)
     {
-      if (fstatat (src_dfd, src_subpath, &local_stbuf, AT_SYMLINK_NOFOLLOW) != 0)
-        {
-          glnx_set_error_from_errno (error);
-          goto out;
-        }
+      if (!glnx_fstatat (src_dfd, src_subpath, &local_stbuf, AT_SYMLINK_NOFOLLOW, error))
+        return FALSE;
       src_stbuf = &local_stbuf;
     }
 
+  /* For symlinks, defer entirely to copy_symlink_at() */
   if (S_ISLNK (src_stbuf->st_mode))
     {
       return copy_symlink_at (src_dfd, src_subpath, src_stbuf,
@@ -924,47 +920,26 @@ glnx_file_copy_at (int                   src_dfd,
     {
       g_set_error (error, G_IO_ERROR, G_IO_ERROR_NOT_SUPPORTED,
                    "Cannot copy non-regular/non-symlink file: %s", src_subpath);
-      goto out;
+      return FALSE;
     }
 
-  if (!glnx_openat_rdonly (src_dfd, src_subpath, FALSE, &src_fd, error))
-    goto out;
-
-  dest_open_flags = O_WRONLY | O_CREAT | O_CLOEXEC | O_NOCTTY;
-  if (!(copyflags & GLNX_FILE_COPY_OVERWRITE))
-    dest_open_flags |= O_EXCL;
-  else
-    dest_open_flags |= O_TRUNC;
-
-  dest_fd = TEMP_FAILURE_RETRY (openat (dest_dfd, dest_subpath, dest_open_flags, src_stbuf->st_mode));
-  if (dest_fd == -1)
-    {
-      glnx_set_error_from_errno (error);
-      goto out;
-    }
+  /* Regular file path below here */
 
-  r = glnx_regfile_copy_bytes (src_fd, dest_fd, (off_t) -1);
-  if (r < 0)
-    {
-      glnx_set_error_from_errno (error);
-      goto out;
-    }
+  glnx_fd_close int src_fd = -1;
+  if (!glnx_openat_rdonly (src_dfd, src_subpath, FALSE, &src_fd, error))
+    return FALSE;
 
-  if (fchown (dest_fd, src_stbuf->st_uid, src_stbuf->st_gid) != 0)
-    {
-      glnx_set_error_from_errno (error);
-      goto out;
-    }
+  /* Open a tmpfile for dest */
+  g_auto(GLnxTmpfile) tmp_dest = { 0, };
+  if (!glnx_open_tmpfile_linkable_at (dest_dfd, ".", O_WRONLY | O_CLOEXEC,
+                                      &tmp_dest, error))
+    return FALSE;
 
-  if (fchmod (dest_fd, src_stbuf->st_mode & 07777) != 0)
-    {
-      glnx_set_error_from_errno (error);
-      goto out;
-    }
+  if (glnx_regfile_copy_bytes (src_fd, tmp_dest.fd, (off_t) -1) < 0)
+    return glnx_throw_errno_prefix (error, "regfile copy");
 
-  ts[0] = src_stbuf->st_atim;
-  ts[1] = src_stbuf->st_mtim;
-  (void) futimens (dest_fd, ts);
+  if (fchown (tmp_dest.fd, src_stbuf->st_uid, src_stbuf->st_gid) != 0)
+    return glnx_throw_errno_prefix (error, "fchown");
 
   if (!(copyflags & GLNX_FILE_COPY_NOXATTRS))
     {
@@ -972,35 +947,40 @@ glnx_file_copy_at (int                   src_dfd,
 
       if (!glnx_fd_get_all_xattrs (src_fd, &xattrs,
                                    cancellable, error))
-        goto out;
+        return FALSE;
 
-      if (!glnx_fd_set_all_xattrs (dest_fd, xattrs,
+      if (!glnx_fd_set_all_xattrs (tmp_dest.fd, xattrs,
                                    cancellable, error))
-        goto out;
+        return FALSE;
     }
 
+  /* Always chmod after setting xattrs, in case the file has mode 0400 or less,
+   * like /etc/shadow.  Linux currently allows write() on non-writable open files
+   * but not fsetxattr().
+   */
+  if (fchmod (tmp_dest.fd, src_stbuf->st_mode & 07777) != 0)
+    return glnx_throw_errno_prefix (error, "fchmod");
+
+  struct timespec ts[2];
+  ts[0] = src_stbuf->st_atim;
+  ts[1] = src_stbuf->st_mtim;
+  (void) futimens (tmp_dest.fd, ts);
+
   if (copyflags & GLNX_FILE_COPY_DATASYNC)
     {
-      if (fdatasync (dest_fd) < 0)
-        {
-          glnx_set_error_from_errno (error);
-          goto out;
-        }
-    }
-  
-  r = close (dest_fd);
-  dest_fd = -1;
-  if (r < 0)
-    {
-      glnx_set_error_from_errno (error);
-      goto out;
+      if (fdatasync (tmp_dest.fd) < 0)
+        return glnx_throw_errno_prefix (error, "fdatasync");
     }
 
-  ret = TRUE;
- out:
-  if (!ret)
-    (void) unlinkat (dest_dfd, dest_subpath, 0);
-  return ret;
+  const GLnxLinkTmpfileReplaceMode replacemode =
+    (copyflags & GLNX_FILE_COPY_OVERWRITE) ?
+    GLNX_LINK_TMPFILE_REPLACE :
+    GLNX_LINK_TMPFILE_NOREPLACE;
+
+  if (!glnx_link_tmpfile_at (&tmp_dest, replacemode, dest_dfd, dest_subpath, error))
+    return FALSE;
+
+  return TRUE;
 }
 
 /**
diff --git a/tests/test-libglnx-fdio.c b/tests/test-libglnx-fdio.c
index 36ded80..4047df6 100644
--- a/tests/test-libglnx-fdio.c
+++ b/tests/test-libglnx-fdio.c
@@ -211,6 +211,67 @@ test_fstatat (void)
   g_assert_no_error (local_error);
 }
 
+static void
+test_filecopy (void)
+{
+  g_autoptr(GError) local_error = NULL;
+  GError **error = &local_error;
+  g_auto(GLnxTmpfile) tmpf = { 0, };
+  const char foo[] = "foo";
+
+  if (!glnx_file_replace_contents_at (AT_FDCWD, foo, (guint8*)foo, sizeof (foo),
+                                      GLNX_FILE_REPLACE_NODATASYNC, NULL, error))
+    goto out;
+
+  if (!glnx_file_copy_at (AT_FDCWD, foo, NULL, AT_FDCWD, "bar",
+                          GLNX_FILE_COPY_NOXATTRS, NULL, error))
+    goto out;
+
+  if (glnx_file_copy_at (AT_FDCWD, foo, NULL, AT_FDCWD, "bar",
+                         GLNX_FILE_COPY_NOXATTRS, NULL, error))
+    g_assert_not_reached ();
+  g_assert_error (local_error, G_IO_ERROR, G_IO_ERROR_EXISTS);
+  g_clear_error (&local_error);
+
+  if (!glnx_file_copy_at (AT_FDCWD, foo, NULL, AT_FDCWD, "bar",
+                          GLNX_FILE_COPY_NOXATTRS | GLNX_FILE_COPY_OVERWRITE,
+                          NULL, error))
+    goto out;
+
+  if (symlinkat ("nosuchtarget", AT_FDCWD, "link") < 0)
+    {
+      glnx_throw_errno_prefix (error, "symlinkat");
+      goto out;
+    }
+
+  /* Shouldn't be able to overwrite a symlink without GLNX_FILE_COPY_OVERWRITE */
+  if (glnx_file_copy_at (AT_FDCWD, foo, NULL, AT_FDCWD, "link",
+                         GLNX_FILE_COPY_NOXATTRS,
+                         NULL, error))
+    g_assert_not_reached ();
+  g_assert_error (local_error, G_IO_ERROR, G_IO_ERROR_EXISTS);
+  g_clear_error (&local_error);
+
+  /* Test overwriting symlink */
+  if (!glnx_file_copy_at (AT_FDCWD, foo, NULL, AT_FDCWD, "link",
+                          GLNX_FILE_COPY_NOXATTRS | GLNX_FILE_COPY_OVERWRITE,
+                          NULL, error))
+    goto out;
+
+  struct stat stbuf;
+  if (!glnx_fstatat_allow_noent (AT_FDCWD, "nosuchtarget", &stbuf, AT_SYMLINK_NOFOLLOW, error))
+    goto out;
+  g_assert_cmpint (errno, ==, ENOENT);
+  g_assert_no_error (local_error);
+
+  if (!glnx_fstatat (AT_FDCWD, "link", &stbuf, AT_SYMLINK_NOFOLLOW, error))
+    goto out;
+  g_assert (S_ISREG (stbuf.st_mode));
+
+ out:
+  g_assert_no_error (local_error);
+}
+
 int main (int argc, char **argv)
 {
   int ret;
@@ -219,6 +280,7 @@ int main (int argc, char **argv)
 
   g_test_add_func ("/tmpfile", test_tmpfile);
   g_test_add_func ("/stdio-file", test_stdio_file);
+  g_test_add_func ("/filecopy", test_filecopy);
   g_test_add_func ("/renameat2-noreplace", test_renameat2_noreplace);
   g_test_add_func ("/renameat2-exchange", test_renameat2_exchange);
   g_test_add_func ("/fstat", test_fstatat);