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. It's better to try the renameat2 with RENAME_NOREPLACE first thing, and fall back on the existing code only if renameat2 fails with errno != EEXIST.
Plus, there are some other problems when combining -u and -n. How about the attached patch instead?
From f80cb02081cdb1d5a31abac651c99bf0280fa6a6 Mon Sep 17 00:00:00 2001 From: Paul Eggert <[email protected]> Date: Thu, 4 Jan 2018 01:46:01 -0800 Subject: [PATCH] mv: improve -n atomicity Problem reported by Kamil Dudka (Bug#29961). * NEWS: Mention these fixes. * doc/coreutils.texi (cp invocation, mv invocation): Mention that -n is silent, and that it overrides -u. * 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. * src/cp.c, src/mv.c (main): -n overrides -u. --- NEWS | 13 ++++++++ doc/coreutils.texi | 13 +++++--- src/copy.c | 87 ++++++++++++++++++++++++++++++++++++------------------ src/cp.c | 3 ++ src/mv.c | 3 ++ 5 files changed, 87 insertions(+), 32 deletions(-) diff --git a/NEWS b/NEWS index b89254f7e..b7ec20085 100644 --- a/NEWS +++ b/NEWS @@ -2,6 +2,19 @@ GNU coreutils NEWS -*- outline -*- * Noteworthy changes in release ?.? (????-??-??) [?] +** 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. + [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/copy.c b/src/copy.c index 2a804945e..f63418bda 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" @@ -1858,7 +1859,7 @@ copy_internal (char const *src_name, char const *dst_name, mode_t dst_mode_bits; mode_t omitted_permissions; bool restore_dst_mode = false; - char *earlier_file = NULL; + char *earlier_file; char *dst_backup = NULL; bool delayed_ok; bool copied_as_regular = false; @@ -1907,6 +1908,21 @@ 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) { /* Regular files can be created by writing through symbolic @@ -1914,18 +1930,22 @@ copy_internal (char const *src_name, char const *dst_name, 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)) - != 0) + destination in that case. Finally, if mv -n then set + USE_STAT to -1, i.e., do not use either stat or lstat. */ + int use_stat = + ((rename_errno == EEXIST + && x->interactive == I_ALWAYS_NO) + ? -1 + : ((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 (0 <= use_stat + && (fstatat (AT_FDCWD, dst_name, &dst_sb, + use_stat ? 0 : AT_SYMLINK_NOFOLLOW) + != 0)) { if (errno != ENOENT) { @@ -1938,17 +1958,23 @@ copy_internal (char const *src_name, char const *dst_name, } } else - { /* Here, we know that dst_name exists, at least to the point - that it is stat'able or lstat'able. */ + { + /* The directory entry DST_NAME was found to exist. */ bool return_now; - have_dst_lstat = !use_stat; - if (! same_file_ok (src_name, &src_sb, dst_name, &dst_sb, - x, &return_now)) + if (use_stat < 0) + return_now = false; + else { - error (0, 0, _("%s and %s are the same file"), - quoteaf_n (0, src_name), quoteaf_n (1, dst_name)); - return false; + have_dst_lstat = !use_stat; + 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)); + return false; + } } if (!S_ISDIR (src_mode) && x->update) @@ -2233,7 +2259,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 +2347,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 +2387,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 +2422,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 +2434,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); @@ -2417,9 +2448,9 @@ copy_internal (char const *src_name, char const *dst_name, and operate on dst_name here as a tighter constraint and also because src_mode is readily available here. */ if ((S_ISDIR (src_mode) ? rmdir (dst_name) : unlink (dst_name)) != 0 - && errno != ENOENT) + && rename_errno != ENOENT) { - error (0, errno, + error (0, rename_errno, _("inter-device move failed: %s to %s; unable to remove target"), quoteaf_n (0, src_name), quoteaf_n (1, dst_name)); forget_created (src_sb.st_ino, src_sb.st_dev); 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
