On 01/04/2018 03:01 AM, Kamil Dudka wrote:
On Thursday, January 4, 2018 10:48:56 AM CET Paul Eggert wrote:
Kamil Dudka wrote:
- if (rename (src_name, dst_name) == 0)
+ int flags = 0;
+ if (x->interactive == I_ALWAYS_NO)
+ /* do not replace DST_NAME if it was created since our last check
*/ + flags = RENAME_NOREPLACE;
By then it's too late, as multiple decisions have been made on the basis of
stat/lstat calls that are still subject to races.
Do you mean in the case of mv -n? Which decisions exactly?
Mostly mv -n, but I suspect problems also for mv without -n. It's all
the decisions that depend on the result of lstat of dst_name, before
abandon_move decides whether to skip the rename. With the patch you
proposed, mv -n could call lstat and get a failure (with errno ==
ENOENT), then (after another process creates the file) call renameat2
with RENAME_NOREPLACE and after this fails (with errno == EEXIST) report
an error. mv -n should silently succeed in that case.
Sounds like a corner case. Please consider writing a separate patch
for that.
OK, that's pretty straightforward so I installed it. Please see the
first attached patch.
I had difficulties trying to evaluate the patch. It does not compile
That's what I get for sending an untested patch. Sorry. I fixed the bugs
you mentioned and tested the result. Please see the second attached
patch, which I have not installed.
There is an interesting behavior change with this second patch.
Currently, 'mv -n a a' fails with a diagnostic "mv: 'a' and 'a' are the
same file". With the patch, 'mv -n a a' silently succeeds. The coreutils
documentation allows both behaviors. I doubt whether anyone cares, and
doing it the new way avoids some syscalls so I left it that way.
>From 7e244891b0c41bbf9f5b5917d1a71c183a8367ac Mon Sep 17 00:00:00 2001
From: Paul Eggert <[email protected]>
Date: Thu, 4 Jan 2018 13:57:40 -0800
Subject: [PATCH 1/2] mv: -n overrides -u
* NEWS: Mention these fixes.
* doc/coreutils.texi (cp invocation, mv invocation):
Mention that -n is silent, and that it overrides -u.
* src/cp.c, src/mv.c (main): -n overrides -u.
---
NEWS | 7 +++++++
doc/coreutils.texi | 13 +++++++++----
src/cp.c | 3 +++
src/mv.c | 3 +++
4 files changed, 22 insertions(+), 4 deletions(-)
diff --git a/NEWS b/NEWS
index b89254f7e..4712f5a46 100644
--- a/NEWS
+++ b/NEWS
@@ -2,6 +2,13 @@ GNU coreutils NEWS -*- outline -*-
* Noteworthy changes in release ?.? (????-??-??) [?]
+** Bug fixes
+
+ '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.
+ [bug introduced with coreutils-7.1]
+
* Noteworthy changes in release 8.29 (2017-12-27) [stable]
diff --git a/doc/coreutils.texi b/doc/coreutils.texi
index 1c0e8a36c..6bb9f0906 100644
--- a/doc/coreutils.texi
+++ b/doc/coreutils.texi
@@ -8569,7 +8569,8 @@ a regular file in the destination tree.
@itemx --no-clobber
@opindex -n
@opindex --no-clobber
-Do not overwrite an existing file. The @option{-n} option overrides a previous
+Do not overwrite an existing file; silently do nothing instead.
+This option overrides a previous
@option{-i} option. This option is mutually exclusive with @option{-b} or
@option{--backup} option.
@@ -8809,8 +8810,10 @@ the comparison is to the source timestamp truncated to the
resolutions of the destination file system and of the system calls
used to update timestamps; this avoids duplicate work if several
@samp{cp -pu} commands are executed with the same source and destination.
-If @option{--preserve=links} is also specified (like with @samp{cp -au}
-for example), that will take precedence. Consequently, depending on the
+This option is ignored if the @option{-n} or @option{--no-clobber}
+option is also specified.
+Also, if @option{--preserve=links} is also specified (like with @samp{cp -au}
+for example), that will take precedence; consequently, depending on the
order that files are processed from the source, newer files in the destination
may be replaced, to mirror hard links in the source.
@@ -9657,7 +9660,7 @@ If the response is not affirmative, the file is skipped.
@opindex -n
@opindex --no-clobber
@cindex prompts, omitting
-Do not overwrite an existing file.
+Do not overwrite an existing file; silently do nothing instead.
@mvOptsIfn
This option is mutually exclusive with @option{-b} or @option{--backup} option.
@@ -9673,6 +9676,8 @@ source timestamp truncated to the resolutions of the destination file
system and of the system calls used to update timestamps; this avoids
duplicate work if several @samp{mv -u} commands are executed with the
same source and destination.
+This option is ignored if the @option{-n} or @option{--no-clobber}
+option is also specified.
@item -v
@itemx --verbose
diff --git a/src/cp.c b/src/cp.c
index 1b5bf7285..d81d41859 100644
--- a/src/cp.c
+++ b/src/cp.c
@@ -1145,6 +1145,9 @@ main (int argc, char **argv)
usage (EXIT_FAILURE);
}
+ if (x.interactive == I_ALWAYS_NO)
+ x.update = false;
+
if (make_backups && x.interactive == I_ALWAYS_NO)
{
error (0, 0,
diff --git a/src/mv.c b/src/mv.c
index a8df730a7..818ca59b6 100644
--- a/src/mv.c
+++ b/src/mv.c
@@ -459,6 +459,9 @@ main (int argc, char **argv)
quoteaf (file[n_files - 1]));
}
+ if (x.interactive == I_ALWAYS_NO)
+ x.update = false;
+
if (make_backups && x.interactive == I_ALWAYS_NO)
{
error (0, 0,
--
2.14.3
>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 2/2] 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
