From 673f48f6ca03131a32c8b5a8c2975f3995802423 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Mon, 11 Sep 2017 16:51:13 -0400 Subject: 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. --- glnx-fdio.c | 132 ++++++++++++++++++++-------------------------- tests/test-libglnx-fdio.c | 62 ++++++++++++++++++++++ 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); -- cgit v1.2.1