On 01/05/2018 08:19 AM, Kamil Dudka wrote:
I am only fixing the case where the destination file is created after the
lstat() call. In that case, the only result is ENOENT, which is harmless.
Ah, you're right. Sorry, I misread your patch. It should work.
On Friday, January 5, 2018 4:29:55 PM CET Pádraig Brady wrote:
Paul's also avoids a stat() in the common case
where the initial renameat2() succeeds.
At the cost of _not_ avoiding the renameat2() call in the most common case.
I expect that the most common case is 'mv A B' where B does not already
exist. This is the case that avoids the stat of B.
Come to think of it, we don't need to stat A either, when the initial
renameat2 succeeds. Attached is a revised proposed patchset to do that.
The first is the same as before; the second causes 'mv A B' to issue
just a renameat2 syscall (with no calls to stat) in the common case
where A exists and B does not.
I will go with my conservative (or even the
documentation-only) patch for RHEL-7, which was the trigger of this effort,
because Paul's patch comes with changes of behavior observable beyond the
Sounds good.
From 09d45e7c2e9b79a33c41271f9794d4133414f313 Mon Sep 17 00:00:00 2001
From: Paul Eggert <[email protected]>
Date: Thu, 4 Jan 2018 16:46:24 -0800
Subject: [PATCH 1/3] mv: improve -n atomicity
Problem reported by Kamil Dudka (Bug#29961).
* NEWS: Mention this.
* src/copy.c: Include renameat2.h.
(copy_internal): If mv, try renameat2 first thing, with
RENAME_NOREPLACE. If this works, skip most of the remaining code.
Also, fail quickly if it fails with EEXIST, and we are using -n.
---
NEWS | 6 +++++
src/copy.c | 88 +++++++++++++++++++++++++++++++++++++++-----------------------
2 files changed, 62 insertions(+), 32 deletions(-)
diff --git a/NEWS b/NEWS
index 4712f5a46..b7ec20085 100644
--- a/NEWS
+++ b/NEWS
@@ -4,6 +4,12 @@ GNU coreutils NEWS -*-
outline -*-
** Bug fixes
+ 'mv -n A B' no longer suffers from a race condition that can
+ overwrite a simultaneously-created B. This bug fix requires
+ platform support for the renameat2 or renameatx_np syscalls, found
+ in recent Linux and macOS kernels.
+ [bug introduced with coreutils-7.1]
+
'cp -n -u' and 'mv -n -u' now consistently ignore the -u option.
Previously, this option combination suffered from race conditions
that caused -u to sometimes override -n.
diff --git a/src/copy.c b/src/copy.c
index 2a804945e..a1dce8e87 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -53,6 +53,7 @@
#include "ignore-value.h"
#include "ioblksize.h"
#include "quote.h"
+#include "renameat2.h"
#include "root-uid.h"
#include "same.h"
#include "savedir.h"
@@ -1907,44 +1908,62 @@ copy_internal (char const *src_name, char const
*dst_name,
bool dereference = should_dereference (x, command_line_arg);
- if (!new_dst)
+ int rename_errno = -1;
+ if (x->move_mode)
{
- /* Regular files can be created by writing through symbolic
- links, but other files cannot. So use stat on the
- destination when copying a regular file, and lstat otherwise.
- However, if we intend to unlink or remove the destination
- first, use lstat, since a copy won't actually be made to the
- destination in that case. */
- bool use_stat =
- ((S_ISREG (src_mode)
- || (x->copy_as_regular
- && ! (S_ISDIR (src_mode) || S_ISLNK (src_mode))))
- && ! (x->move_mode || x->symbolic_link || x->hard_link
- || x->backup_type != no_backups
- || x->unlink_dest_before_opening));
- if ((use_stat
- ? stat (dst_name, &dst_sb)
- : lstat (dst_name, &dst_sb))
+ if (renameat2 (AT_FDCWD, src_name, AT_FDCWD, dst_name, RENAME_NOREPLACE)
!= 0)
+ rename_errno = errno;
+ else
+ {
+ rename_errno = 0;
+ new_dst = true;
+ if (rename_succeeded)
+ *rename_succeeded = true;
+ }
+ }
+
+ if (!new_dst)
+ {
+ if (! (rename_errno == EEXIST && x->interactive == I_ALWAYS_NO))
{
- if (errno != ENOENT)
+ /* Regular files can be created by writing through symbolic
+ links, but other files cannot. So use stat on the
+ destination when copying a regular file, and lstat otherwise.
+ However, if we intend to unlink or remove the destination
+ first, use lstat, since a copy won't actually be made to the
+ destination in that case. */
+ bool use_lstat
+ = ((! S_ISREG (src_mode)
+ && (! x->copy_as_regular
+ || S_ISDIR (src_mode) || S_ISLNK (src_mode)))
+ || x->move_mode || x->symbolic_link || x->hard_link
+ || x->backup_type != no_backups
+ || x->unlink_dest_before_opening);
+ int fstatat_flags = use_lstat ? AT_SYMLINK_NOFOLLOW : 0;
+ if (fstatat (AT_FDCWD, dst_name, &dst_sb, fstatat_flags) == 0)
{
- error (0, errno, _("cannot stat %s"), quoteaf (dst_name));
- return false;
+ have_dst_lstat = use_lstat;
+ rename_errno = EEXIST;
}
else
{
+ if (errno != ENOENT)
+ {
+ error (0, errno, _("cannot stat %s"), quoteaf (dst_name));
+ return false;
+ }
new_dst = true;
}
}
- else
- { /* Here, we know that dst_name exists, at least to the point
- that it is stat'able or lstat'able. */
- bool return_now;
- have_dst_lstat = !use_stat;
- if (! same_file_ok (src_name, &src_sb, dst_name, &dst_sb,
- x, &return_now))
+ if (rename_errno == EEXIST)
+ {
+ bool return_now = false;
+
+ if (x->interactive != I_ALWAYS_NO
+ && ! same_file_ok (src_name, &src_sb, dst_name, &dst_sb,
+ x, &return_now))
{
error (0, 0, _("%s and %s are the same file"),
quoteaf_n (0, src_name), quoteaf_n (1, dst_name));
@@ -2233,7 +2252,9 @@ copy_internal (char const *src_name, char const *dst_name,
Also, with --recursive, record dev/ino of each command-line directory.
We'll use that info to detect this problem: cp -R dir dir. */
- if (x->recursive && S_ISDIR (src_mode))
+ if (rename_errno == 0)
+ earlier_file = NULL;
+ else if (x->recursive && S_ISDIR (src_mode))
{
if (command_line_arg)
earlier_file = remember_copied (dst_name, src_sb.st_ino,
src_sb.st_dev);
@@ -2319,7 +2340,10 @@ copy_internal (char const *src_name, char const
*dst_name,
if (x->move_mode)
{
- if (rename (src_name, dst_name) == 0)
+ if (rename_errno == EEXIST)
+ rename_errno = rename (src_name, dst_name) == 0 ? 0 : errno;
+
+ if (rename_errno == 0)
{
if (x->verbose)
{
@@ -2356,7 +2380,7 @@ copy_internal (char const *src_name, char const *dst_name,
/* This happens when attempting to rename a directory to a
subdirectory of itself. */
- if (errno == EINVAL)
+ if (rename_errno == EINVAL)
{
/* FIXME: this is a little fragile in that it relies on rename(2)
failing with a specific errno value. Expect problems on
@@ -2391,7 +2415,7 @@ copy_internal (char const *src_name, char const *dst_name,
where you'd replace '18' with the integer in parentheses that
was output from the perl one-liner above.
If necessary, of course, change '/tmp' to some other directory. */
- if (errno != EXDEV)
+ if (rename_errno != EXDEV)
{
/* There are many ways this can happen due to a race condition.
When something happens between the initial XSTAT and the
@@ -2403,7 +2427,7 @@ copy_internal (char const *src_name, char const *dst_name,
If the permissions on the directory containing the source or
destination file are made too restrictive, the rename will
fail. Etc. */
- error (0, errno,
+ error (0, rename_errno,
_("cannot move %s to %s"),
quoteaf_n (0, src_name), quoteaf_n (1, dst_name));
forget_created (src_sb.st_ino, src_sb.st_dev);
--
2.14.3
From a412f8afc603378a789ce4e8d18bc6d9c4ca3d13 Mon Sep 17 00:00:00 2001
From: Paul Eggert <[email protected]>
Date: Fri, 5 Jan 2018 14:33:25 -0800
Subject: [PATCH 2/3] =?UTF-8?q?mv:=20fewer=20syscalls=20for=20=E2=80=98mv?=
=?UTF-8?q?=20a=20b=E2=80=99?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
This builds on a previous patch for mv atomicity (Bug#29961).
It merely improves performance; it does not fix bugs.
* src/copy.h (struct cp_options): New members last_file, rename_errno.
* src/copy.c (copy_internal): Support new rename_errno member
for the copy options. Avoid calling stat when new members
suggest it’s not needed.
(cp_options_default): Initialize new members.
* src/mv.c: Include renameat2.h.
(main): With two arguments, first call ‘renamat2 (AT_FDCWD, "a",
AT_FDCWD, "b", RENAME_NOREPLACE)’. Use its results to skip
remaining processing if possible; for example, if it succeeds
there is no need to stat either "a" or "b". Also, set
x.last_file when it is the last file to rename.
---
src/copy.c | 67 +++++++++++++++++++++++++++++++-------------------------------
src/copy.h | 9 +++++++++
src/mv.c | 22 +++++++++++++++++----
3 files changed, 61 insertions(+), 37 deletions(-)
diff --git a/src/copy.c b/src/copy.c
index a1dce8e87..e050d4199 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -1854,7 +1854,7 @@ copy_internal (char const *src_name, char const *dst_name,
{
struct stat src_sb;
struct stat dst_sb;
- mode_t src_mode;
+ mode_t src_mode IF_LINT ( = 0);
mode_t dst_mode IF_LINT ( = 0);
mode_t dst_mode_bits;
mode_t omitted_permissions;
@@ -1866,33 +1866,48 @@ copy_internal (char const *src_name, char const
*dst_name,
bool dest_is_symlink = false;
bool have_dst_lstat = false;
- if (x->move_mode && rename_succeeded)
- *rename_succeeded = false;
-
*copy_into_self = false;
- if (XSTAT (x, src_name, &src_sb) != 0)
+ int rename_errno = x->rename_errno;
+ if (x->move_mode)
{
- error (0, errno, _("cannot stat %s"), quoteaf (src_name));
- return false;
+ if (rename_errno < 0)
+ rename_errno = (renameat2 (AT_FDCWD, src_name, AT_FDCWD, dst_name,
+ RENAME_NOREPLACE)
+ ? errno : 0);
+ new_dst = rename_errno == 0;
+ if (rename_succeeded)
+ *rename_succeeded = new_dst;
}
- src_mode = src_sb.st_mode;
-
- if (S_ISDIR (src_mode) && !x->recursive)
+ if (rename_errno == 0
+ ? !x->last_file
+ : rename_errno != EEXIST || x->interactive != I_ALWAYS_NO)
{
- error (0, 0, ! x->install_mode /* cp */
- ? _("-r not specified; omitting directory %s")
- : _("omitting directory %s"),
- quoteaf (src_name));
- return false;
+ char const *name = rename_errno == 0 ? dst_name : src_name;
+ if (XSTAT (x, name, &src_sb) != 0)
+ {
+ error (0, errno, _("cannot stat %s"), quoteaf (name));
+ return false;
+ }
+
+ src_mode = src_sb.st_mode;
+
+ if (S_ISDIR (src_mode) && !x->recursive)
+ {
+ error (0, 0, ! x->install_mode /* cp */
+ ? _("-r not specified; omitting directory %s")
+ : _("omitting directory %s"),
+ quoteaf (src_name));
+ return false;
+ }
}
/* Detect the case in which the same source file appears more than
once on the command line and no backup option has been selected.
If so, simply warn and don't copy it the second time.
This check is enabled only if x->src_info is non-NULL. */
- if (command_line_arg)
+ if (command_line_arg && x->src_info)
{
if ( ! S_ISDIR (src_sb.st_mode)
&& x->backup_type == no_backups
@@ -1908,21 +1923,6 @@ copy_internal (char const *src_name, char const
*dst_name,
bool dereference = should_dereference (x, command_line_arg);
- int rename_errno = -1;
- if (x->move_mode)
- {
- if (renameat2 (AT_FDCWD, src_name, AT_FDCWD, dst_name, RENAME_NOREPLACE)
- != 0)
- rename_errno = errno;
- else
- {
- rename_errno = 0;
- new_dst = true;
- if (rename_succeeded)
- *rename_succeeded = true;
- }
- }
-
if (!new_dst)
{
if (! (rename_errno == EEXIST && x->interactive == I_ALWAYS_NO))
@@ -1970,7 +1970,7 @@ copy_internal (char const *src_name, char const *dst_name,
return false;
}
- if (!S_ISDIR (src_mode) && x->update)
+ if (x->update && !S_ISDIR (src_mode))
{
/* When preserving timestamps (but not moving within a file
system), don't worry if the destination timestamp is
@@ -2360,7 +2360,7 @@ copy_internal (char const *src_name, char const *dst_name,
if (rename_succeeded)
*rename_succeeded = true;
- if (command_line_arg)
+ if (command_line_arg && !x->last_file)
{
/* Record destination dev/ino/name, so that if we are asked
to overwrite that file again, we can detect it and fail. */
@@ -2998,6 +2998,7 @@ cp_options_default (struct cp_options *x)
#else
x->chown_privileges = x->owner_privileges = (geteuid () == ROOT_UID);
#endif
+ x->rename_errno = -1;
}
/* Return true if it's OK for chown to fail, where errno is
diff --git a/src/copy.h b/src/copy.h
index dd03385c3..c7fdcffbe 100644
--- a/src/copy.h
+++ b/src/copy.h
@@ -249,6 +249,15 @@ struct cp_options
such a symlink) and returns false. */
bool open_dangling_dest_symlink;
+ /* If true, this is the last filed to be copied. mv uses this to
+ avoid some unnecessary work. */
+ bool last_file;
+
+ /* Zero if the source has already been renamed to the destination; a
+ positive errno number if this failed with the given errno; -1 if
+ no attempt has been made to rename. Always -1, except for mv. */
+ int rename_errno;
+
/* Control creation of COW files. */
enum Reflink_type reflink_mode;
diff --git a/src/mv.c b/src/mv.c
index 818ca59b6..edc1e733f 100644
--- a/src/mv.c
+++ b/src/mv.c
@@ -31,6 +31,7 @@
#include "error.h"
#include "filenamecat.h"
#include "remove.h"
+#include "renameat2.h"
#include "root-dev-ino.h"
#include "priv-set.h"
@@ -452,8 +453,15 @@ main (int argc, char **argv)
else if (!target_directory)
{
assert (2 <= n_files);
- if (target_directory_operand (file[n_files - 1]))
- target_directory = file[--n_files];
+ if (n_files == 2)
+ x.rename_errno = (renameat2 (AT_FDCWD, file[0], AT_FDCWD, file[1],
+ RENAME_NOREPLACE)
+ ? errno : 0);
+ if (x.rename_errno != 0 && target_directory_operand (file[n_files - 1]))
+ {
+ x.rename_errno = -1;
+ target_directory = file[--n_files];
+ }
else if (2 < n_files)
die (EXIT_FAILURE, 0, _("target %s is not a directory"),
quoteaf (file[n_files - 1]));
@@ -487,10 +495,16 @@ main (int argc, char **argv)
ok = true;
for (int i = 0; i < n_files; ++i)
- ok &= movefile (file[i], target_directory, true, &x);
+ {
+ x.last_file = i + 1 == n_files;
+ ok &= movefile (file[i], target_directory, true, &x);
+ }
}
else
- ok = movefile (file[0], file[1], false, &x);
+ {
+ x.last_file = true;
+ ok = movefile (file[0], file[1], false, &x);
+ }
return ok ? EXIT_SUCCESS : EXIT_FAILURE;
}
--
2.14.3
From 5e9221924eb67a92134b0dcfb2629c82d77b943c Mon Sep 17 00:00:00 2001
From: Paul Eggert <[email protected]>
Date: Fri, 5 Jan 2018 14:36:02 -0800
Subject: [PATCH 3/3] =?UTF-8?q?mv:=20clarify=20=E2=80=98mv=20-n=20A=20A?=
=?UTF-8?q?=E2=80=99=20change?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
---
NEWS | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/NEWS b/NEWS
index b7ec20085..8a9e09eb4 100644
--- a/NEWS
+++ b/NEWS
@@ -7,7 +7,8 @@ GNU coreutils NEWS -*-
outline -*-
'mv -n A B' no longer suffers from a race condition that can
overwrite a simultaneously-created B. This bug fix requires
platform support for the renameat2 or renameatx_np syscalls, found
- in recent Linux and macOS kernels.
+ in recent Linux and macOS kernels. As a side effect, ‘mv -n A A’
+ now silently does nothing if A exists.
[bug introduced with coreutils-7.1]
'cp -n -u' and 'mv -n -u' now consistently ignore the -u option.
--
2.14.3
