Skip to content
Snippets Groups Projects
Commit 673f48f6 authored by Colin Walters's avatar Colin Walters
Browse files

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.
parent 9d995a36
No related branches found
No related tags found
No related merge requests found
...@@ -872,11 +872,15 @@ glnx_regfile_copy_bytes (int fdf, int fdt, off_t max_bytes) ...@@ -872,11 +872,15 @@ glnx_regfile_copy_bytes (int fdf, int fdt, off_t max_bytes)
* @cancellable: cancellable * @cancellable: cancellable
* @error: Error * @error: Error
* *
* Perform a full copy of the regular file or * Perform a full copy of the regular file or symbolic link from @src_subpath to
* symbolic link from @src_subpath to @dest_subpath. * @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 * If the source is a regular file and the destination exists as a symbolic
* file or symbolic link, an error will be returned. * 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 gboolean
glnx_file_copy_at (int src_dfd, glnx_file_copy_at (int src_dfd,
...@@ -888,31 +892,23 @@ glnx_file_copy_at (int src_dfd, ...@@ -888,31 +892,23 @@ glnx_file_copy_at (int src_dfd,
GCancellable *cancellable, GCancellable *cancellable,
GError **error) GError **error)
{ {
gboolean ret = FALSE; /* Canonicalize dfds */
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;
src_dfd = glnx_dirfd_canonicalize (src_dfd); src_dfd = glnx_dirfd_canonicalize (src_dfd);
dest_dfd = glnx_dirfd_canonicalize (dest_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 */ /* Automatically do stat() if no stat buffer was supplied */
struct stat local_stbuf;
if (!src_stbuf) if (!src_stbuf)
{ {
if (fstatat (src_dfd, src_subpath, &local_stbuf, AT_SYMLINK_NOFOLLOW) != 0) if (!glnx_fstatat (src_dfd, src_subpath, &local_stbuf, AT_SYMLINK_NOFOLLOW, error))
{ return FALSE;
glnx_set_error_from_errno (error);
goto out;
}
src_stbuf = &local_stbuf; src_stbuf = &local_stbuf;
} }
/* For symlinks, defer entirely to copy_symlink_at() */
if (S_ISLNK (src_stbuf->st_mode)) if (S_ISLNK (src_stbuf->st_mode))
{ {
return copy_symlink_at (src_dfd, src_subpath, src_stbuf, return copy_symlink_at (src_dfd, src_subpath, src_stbuf,
...@@ -924,47 +920,26 @@ glnx_file_copy_at (int src_dfd, ...@@ -924,47 +920,26 @@ glnx_file_copy_at (int src_dfd,
{ {
g_set_error (error, G_IO_ERROR, G_IO_ERROR_NOT_SUPPORTED, g_set_error (error, G_IO_ERROR, G_IO_ERROR_NOT_SUPPORTED,
"Cannot copy non-regular/non-symlink file: %s", src_subpath); "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)) /* Regular file path below here */
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;
}
r = glnx_regfile_copy_bytes (src_fd, dest_fd, (off_t) -1); glnx_fd_close int src_fd = -1;
if (r < 0) if (!glnx_openat_rdonly (src_dfd, src_subpath, FALSE, &src_fd, error))
{ return FALSE;
glnx_set_error_from_errno (error);
goto out;
}
if (fchown (dest_fd, src_stbuf->st_uid, src_stbuf->st_gid) != 0) /* Open a tmpfile for dest */
{ g_auto(GLnxTmpfile) tmp_dest = { 0, };
glnx_set_error_from_errno (error); if (!glnx_open_tmpfile_linkable_at (dest_dfd, ".", O_WRONLY | O_CLOEXEC,
goto out; &tmp_dest, error))
} return FALSE;
if (fchmod (dest_fd, src_stbuf->st_mode & 07777) != 0) if (glnx_regfile_copy_bytes (src_fd, tmp_dest.fd, (off_t) -1) < 0)
{ return glnx_throw_errno_prefix (error, "regfile copy");
glnx_set_error_from_errno (error);
goto out;
}
ts[0] = src_stbuf->st_atim; if (fchown (tmp_dest.fd, src_stbuf->st_uid, src_stbuf->st_gid) != 0)
ts[1] = src_stbuf->st_mtim; return glnx_throw_errno_prefix (error, "fchown");
(void) futimens (dest_fd, ts);
if (!(copyflags & GLNX_FILE_COPY_NOXATTRS)) if (!(copyflags & GLNX_FILE_COPY_NOXATTRS))
{ {
...@@ -972,35 +947,40 @@ glnx_file_copy_at (int src_dfd, ...@@ -972,35 +947,40 @@ glnx_file_copy_at (int src_dfd,
if (!glnx_fd_get_all_xattrs (src_fd, &xattrs, if (!glnx_fd_get_all_xattrs (src_fd, &xattrs,
cancellable, error)) 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)) 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 (copyflags & GLNX_FILE_COPY_DATASYNC)
{ {
if (fdatasync (dest_fd) < 0) if (fdatasync (tmp_dest.fd) < 0)
{ return glnx_throw_errno_prefix (error, "fdatasync");
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;
} }
ret = TRUE; const GLnxLinkTmpfileReplaceMode replacemode =
out: (copyflags & GLNX_FILE_COPY_OVERWRITE) ?
if (!ret) GLNX_LINK_TMPFILE_REPLACE :
(void) unlinkat (dest_dfd, dest_subpath, 0); GLNX_LINK_TMPFILE_NOREPLACE;
return ret;
if (!glnx_link_tmpfile_at (&tmp_dest, replacemode, dest_dfd, dest_subpath, error))
return FALSE;
return TRUE;
} }
/** /**
......
...@@ -211,6 +211,67 @@ test_fstatat (void) ...@@ -211,6 +211,67 @@ test_fstatat (void)
g_assert_no_error (local_error); 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 main (int argc, char **argv)
{ {
int ret; int ret;
...@@ -219,6 +280,7 @@ int main (int argc, char **argv) ...@@ -219,6 +280,7 @@ int main (int argc, char **argv)
g_test_add_func ("/tmpfile", test_tmpfile); g_test_add_func ("/tmpfile", test_tmpfile);
g_test_add_func ("/stdio-file", test_stdio_file); 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-noreplace", test_renameat2_noreplace);
g_test_add_func ("/renameat2-exchange", test_renameat2_exchange); g_test_add_func ("/renameat2-exchange", test_renameat2_exchange);
g_test_add_func ("/fstat", test_fstatat); g_test_add_func ("/fstat", test_fstatat);
......
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment