Subject: bug#29961: [PATCH] mv -n: do not overwrite the destination

From: Paul Eggert <[email-masked]>
Date: Thu, 4 Jan 2018 01:48:56 -0800

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-masked]>
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