Subject: bug#18499: Possible mv race for hardlinks (rhbz #1141368 )

From: Pádraig Brady <[email-masked]>
Date: Fri, 21 Nov 2014 03:30:50 +0000

On 18/11/14 19:28, Boris Ranto wrote:
> On Tue, 2014-11-18 at 16:46 +0000, Pádraig Brady wrote:
>> On 18/11/14 16:29, Boris Ranto wrote:
>>> On Mon, 2014-11-17 at 00:28 +0000, Pádraig Brady wrote:
>>>> On 16/11/14 16:35, Paul Eggert wrote:
>>>>> Pádraig Brady wrote:
>>>>>> If we change this, it's much more likely that people will start complaining
>>>>>> about their non overlapping mv instances failing.
>>>>>
>>>>> I'd far rather deal with those complaints than deal with complaints about 'mv' silently discarding files.  Either the FreeBSD or the Solaris behavior would be a real improvement over what we're doing now.  Neither behavior is as good as having support in the kernel for doing the right thing, but one step at a time.
>>>>
>>>> Fair enough. That's 3 votes for changing this.
>>>> I'll work on a patch to fail in this case.
>>>>
>>>> thanks,
>>>> Pádraig.
>>>>
>>>
>>> I've looked at the code and I was able to identify the part that deals
>>> with the symlinks. I'm attaching the patch that makes mv fail in this
>>> case.
>>
>> I'm not sure symlinks should be treated differently here.
>> I.E. it may be best to remove the whole unlink_src logic.
>> I'll look later.
>>
>> thanks,
>> Pádraig.
> 
> You were right, the only other place that used the unlink_src logic was
> the case handling mv for symlinks that were hard links to the same file
> (this case was handled separetely from the normal files) -- i.e. the
> case where you do this:
> touch a; ln -s a b; ln b c; mv b c
> 
> I'm attaching the revised patch that removes the whole unlink_src logic
> altogether.

We want to leave the logic in place for cp and install though,
and I've adjusted your patch accordingly. I've also adjusted
the tests to pass and augmented the tests to cover one of
the cases missed in the previous patch.  I'll push this tomorrow.

thanks,
Pádraig.
From 222d7ac0c4f5f005438c534f3aba62fd94d96dc2 Mon Sep 17 00:00:00 2001
From: Boris Ranto <[email-masked]>
Date: Tue, 18 Nov 2014 20:20:50 +0100
Subject: [PATCH] mv: fail when moving a file to a hardlink

We may run into a race condition if we treat hard links to the same file
as distinct files.  If we do 'mv a b' and 'mv b a' in parallel, both a
and b can disappear from the file system.  The reason is that in this
case the unlink on src is called and the system calls can end up being
run in the order where unlink(a) and unlink(b) are the last two system
calls.  Therefore exit with an error code so that we avoid the potential
data loss.

* src/copy.c (same_file_ok): Don't set unlink_src that was used by mv,
and return false for two hardlinks to a file in move_mode.
*src/copy.c (copy_internal): No longer honor the unlink_src option,
used only by mv.
NEWS: Mention the change in behavior.
* tests/cp/same-file.sh: Augment to cover the `cp -a hlsl1 sl1` case.
* tests/mv/hard-verbose.sh: Remove no longer needed test.
* tests/local.mk: Remove the reference to hard-verbose.sh.
* tests/mv/hard-4.sh: Adjust so we fail in this case.
* tests/mv/i-4.sh: Likewise.
* tests/mv/symlink-onto-hardlink-to-self.sh: Likewise.
---
 NEWS                                      |  7 ++++
 src/copy.c                                | 53 ++++++++-----------------------
 tests/cp/same-file.sh                     | 28 +++++++++++++---
 tests/local.mk                            |  1 -
 tests/mv/force.sh                         | 21 ++++++------
 tests/mv/hard-4.sh                        | 17 ++++++----
 tests/mv/hard-verbose.sh                  | 33 -------------------
 tests/mv/i-4.sh                           | 14 ++++----
 tests/mv/symlink-onto-hardlink-to-self.sh | 35 ++++++++++++--------
 9 files changed, 92 insertions(+), 117 deletions(-)
 delete mode 100755 tests/mv/hard-verbose.sh

diff --git a/NEWS b/NEWS
index 5fbdc6a..6a84c48 100644
--- a/NEWS
+++ b/NEWS
@@ -30,6 +30,13 @@ GNU coreutils NEWS                                    -*- outline -*-
   dd accepts a new status=progress level to print data transfer statistics
   on stderr approximately every second.
 
+** Changes in behavior
+
+   mv no longer supports moving a file to a hardlink, as currently that
+   is only supported through unlinking the source file.  That's racy
+   in the presence of multiple mv instances, which could result in both
+   hardlinks being deleted.  This feature was added in coreutils-5.0.1.
+
 ** Improvements
 
   cp,install,mv will convert smaller runs of NULs in the input to holes,
diff --git a/src/copy.c b/src/copy.c
index 446e72d..01cee32 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -1407,20 +1407,12 @@ close_src_desc:
    copy a regular file onto a symlink that points to it.
    Try to minimize the cost of this function in the common case.
    Set *RETURN_NOW if we've determined that the caller has no more
-   work to do and should return successfully, right away.
-
-   Set *UNLINK_SRC if we've determined that the caller wants to do
-   'rename (a, b)' where 'a' and 'b' are distinct hard links to the same
-   file. In that case, the caller should try to unlink 'a' and then return
-   successfully.  Ideally, we wouldn't have to do that, and we'd be
-   able to rely on rename to remove the source file.  However, POSIX
-   mistakenly requires that such a rename call do *nothing* and return
-   successfully.  */
+   work to do and should return successfully, right away.  */
 
 static bool
 same_file_ok (char const *src_name, struct stat const *src_sb,
               char const *dst_name, struct stat const *dst_sb,
-              const struct cp_options *x, bool *return_now, bool *unlink_src)
+              const struct cp_options *x, bool *return_now)
 {
   const struct stat *src_sb_link;
   const struct stat *dst_sb_link;
@@ -1431,7 +1423,6 @@ same_file_ok (char const *src_name, struct stat const *src_sb,
   bool same = SAME_INODE (*src_sb, *dst_sb);
 
   *return_now = false;
-  *unlink_src = false;
 
   /* FIXME: this should (at the very least) be moved into the following
      if-block.  More likely, it should be removed, because it inhibits
@@ -1463,14 +1454,11 @@ same_file_ok (char const *src_name, struct stat const *src_sb,
               /* Here we have two symlinks that are hard-linked together,
                  and we're not making backups.  In this unusual case, simply
                  returning true would lead to mv calling "rename(A,B)",
-                 which would do nothing and return 0.  I.e., A would
-                 not be removed.  Hence, the solution is to tell the
-                 caller that all it must do is unlink A and return.  */
+                 which would do nothing and return 0.  */
               if (same_link)
                 {
-                  *unlink_src = true;
                   *return_now = true;
-                  return true;
+                  return ! x->move_mode;
                 }
             }
 
@@ -1558,27 +1546,21 @@ same_file_ok (char const *src_name, struct stat const *src_sb,
     return true;
 #endif
 
-  /* They may refer to the same file if we're in move mode and the
-     target is a symlink.  That is ok, since we remove any existing
-     destination file before opening it -- via 'rename' if they're on
-     the same file system, via 'unlink (DST_NAME)' otherwise.
-     It's also ok if they're distinct hard links to the same file.  */
   if (x->move_mode || x->unlink_dest_before_opening)
     {
+      /* They may refer to the same file if we're in move mode and the
+         target is a symlink.  That is ok, since we remove any existing
+         destination file before opening it -- via 'rename' if they're on
+         the same file system, via 'unlink (DST_NAME)' otherwise.  */
       if (S_ISLNK (dst_sb_link->st_mode))
         return true;
 
+      /* It's not ok if they're distinct hard links to the same file as
+         this causes a race condition and we may lose data in this case.  */
       if (same_link
           && 1 < dst_sb_link->st_nlink
           && ! same_name (src_name, dst_name))
-        {
-          if (x->move_mode)
-            {
-              *unlink_src = true;
-              *return_now = true;
-            }
-          return true;
-        }
+        return ! x->move_mode;
     }
 
   /* If neither is a symlink, then it's ok as long as they aren't
@@ -1939,11 +1921,10 @@ copy_internal (char const *src_name, char const *dst_name,
         { /* Here, we know that dst_name exists, at least to the point
              that it is stat'able or lstat'able.  */
           bool return_now;
-          bool unlink_src;
 
           have_dst_lstat = !use_stat;
           if (! same_file_ok (src_name, &src_sb, dst_name, &dst_sb,
-                              x, &return_now, &unlink_src))
+                              x, &return_now))
             {
               error (0, 0, _("%s and %s are the same file"),
                      quote_n (0, src_name), quote_n (1, dst_name));
@@ -2002,22 +1983,14 @@ copy_internal (char const *src_name, char const *dst_name,
              cp and mv treat -i and -f differently.  */
           if (x->move_mode)
             {
-              if (abandon_move (x, dst_name, &dst_sb)
-                  || (unlink_src && unlink (src_name) == 0))
+              if (abandon_move (x, dst_name, &dst_sb))
                 {
                   /* Pretend the rename succeeded, so the caller (mv)
                      doesn't end up removing the source file.  */
                   if (rename_succeeded)
                     *rename_succeeded = true;
-                  if (unlink_src && x->verbose)
-                    printf (_("removed %s\n"), quote (src_name));
                   return true;
                 }
-              if (unlink_src)
-                {
-                  error (0, errno, _("cannot remove %s"), quote (src_name));
-                  return false;
-                }
             }
           else
             {
diff --git a/tests/cp/same-file.sh b/tests/cp/same-file.sh
index f62a9a7..54d23a5 100755
--- a/tests/cp/same-file.sh
+++ b/tests/cp/same-file.sh
@@ -44,7 +44,8 @@ exec 3>&1 1> actual
 # FIXME: This should be bigger: like more than 8k
 contents=XYZ
 
-for args in 'foo symlink' 'symlink foo' 'foo foo' 'sl1 sl2' 'foo hardlink'; do
+for args in 'foo symlink' 'symlink foo' 'foo foo' 'sl1 sl2' \
+            'foo hardlink' 'hlsl sl2'; do
   for options in '' -d -f -df --rem -b -bd -bf -bdf \
                  -l -dl -fl -dfl -bl -bdl -bfl -bdfl; do
     case $args$options in
@@ -70,11 +71,11 @@ for args in 'foo symlink' 'symlink foo' 'foo foo' 'sl1 sl2' 'foo hardlink'; do
     # cont'd  Instead, skip them only on systems for which link does
     # dereference a symlink.  Detect and skip such tests here.
     case $hard_link_to_symlink_does_the_deref:$args:$options in
-      'yes:sl1 sl2:-fl')
+      yes:*sl2:-fl)
         continue ;;
-      'yes:sl1 sl2:-bl')
+      yes:*sl2:-bl)
         continue ;;
-      'yes:sl1 sl2:-bfl')
+      yes:*sl2:-bfl)
         continue ;;
     esac
 
@@ -86,6 +87,7 @@ for args in 'foo symlink' 'symlink foo' 'foo foo' 'sl1 sl2' 'foo hardlink'; do
     case "$args" in *hardlink*) ln foo hardlink ;; esac
     case "$args" in *sl1*) ln -s foo sl1;; esac
     case "$args" in *sl2*) ln -s foo sl2;; esac
+    case "$args" in *hlsl*) ln sl2 hlsl;; esac
     (
       (
         # echo 1>&2 cp $options $args
@@ -211,6 +213,24 @@ cat <<\EOF | sed "$remove_these_sed" > expected
 0 -bfl (foo hardlink)
 0 -bdfl (foo hardlink)
 
+1 [cp: 'hlsl' and 'sl2' are the same file] (foo hlsl -> foo sl2 -> foo)
+0 -d (foo hlsl -> foo sl2 -> foo)
+1 -f [cp: 'hlsl' and 'sl2' are the same file] (foo hlsl -> foo sl2 -> foo)
+0 -df (foo hlsl -> foo sl2 -> foo)
+0 --rem (foo hlsl -> foo sl2)
+0 -b (foo hlsl -> foo sl2 sl2.~1~ -> foo)
+0 -bd (foo hlsl -> foo sl2 -> foo sl2.~1~ -> foo)
+0 -bf (foo hlsl -> foo sl2 sl2.~1~ -> foo)
+0 -bdf (foo hlsl -> foo sl2 -> foo sl2.~1~ -> foo)
+1 -l [cp: cannot create hard link 'sl2' to 'hlsl'] (foo hlsl -> foo sl2 -> foo)
+0 -dl (foo hlsl -> foo sl2 -> foo)
+0 -fl (foo hlsl -> foo sl2)
+0 -dfl (foo hlsl -> foo sl2 -> foo)
+0 -bl (foo hlsl -> foo sl2 sl2.~1~ -> foo)
+0 -bdl (foo hlsl -> foo sl2 -> foo)
+0 -bfl (foo hlsl -> foo sl2 sl2.~1~ -> foo)
+0 -bdfl (foo hlsl -> foo sl2 -> foo)
+
 EOF
 
 exec 1>&3 3>&-
diff --git a/tests/local.mk b/tests/local.mk
index e01f4d8..22e8b86 100644
--- a/tests/local.mk
+++ b/tests/local.mk
@@ -605,7 +605,6 @@ all_tests =					\
   tests/mv/hard-3.sh				\
   tests/mv/hard-4.sh				\
   tests/mv/hard-link-1.sh			\
-  tests/mv/hard-verbose.sh			\
   tests/mv/i-1.pl				\
   tests/mv/i-2.sh				\
   tests/mv/i-3.sh				\
diff --git a/tests/mv/force.sh b/tests/mv/force.sh
index 05adabc..be97665 100755
--- a/tests/mv/force.sh
+++ b/tests/mv/force.sh
@@ -25,18 +25,19 @@ ff2=mvforce2
 echo force-contents > $ff || framework_failure_
 ln $ff $ff2 || framework_failure_
 
-# This mv command should exit nonzero.
-mv $ff $ff > out 2>&1 && fail=1
+# mv should fail for the same name, or separate hardlinks as in
+# both cases rename() will do nothing and return success.
+# One could unlink(src) in the hardlink case, but that would
+# introduce races with overlapping mv instances removing both hardlinks.
 
-cat > exp <<EOF
-mv: '$ff' and '$ff' are the same file
-EOF
+for dest in $ff $ff2; do
+  # This mv command should exit nonzero.
+  mv $ff $dest > out 2>&1 && fail=1
 
-compare exp out || fail=1
-test $(cat $ff) = force-contents || fail=1
+  printf "mv: '$ff' and '$dest' are the same file\n" > exp
+  compare exp out || fail=1
 
-# This should succeed, even though the source and destination
-# device and inodes are the same.
-mv $ff $ff2 || fail=1
+  test $(cat $ff) = force-contents || fail=1
+done
 
 Exit $fail
diff --git a/tests/mv/hard-4.sh b/tests/mv/hard-4.sh
index d518e3b..8b03c45 100755
--- a/tests/mv/hard-4.sh
+++ b/tests/mv/hard-4.sh
@@ -1,5 +1,5 @@
 #!/bin/sh
-# ensure that mv removes a in this case: touch a; ln a b; mv a b
+# ensure that mv maintains a in this case: touch a; ln a b; mv a b
 
 # Copyright (C) 2003-2014 Free Software Foundation, Inc.
 
@@ -21,15 +21,18 @@ print_ver_ mv
 touch a || framework_failure_
 ln a b || framework_failure_
 
+# Between coreutils-5.0 and coreutils-8.24, 'a' would be removed.
+# Before coreutils-5.0.1 the issue would not have been diagnosed.
+# We don't emulate the rename(a,b) with unlink(a) as that would
+# introduce races with overlapping mv instances removing both links.
+mv a b 2>err && fail=1
+printf "mv: 'a' and 'b' are the same file\n" > exp
+compare exp err || fail=1
 
-mv a b || fail=1
-
-# In coreutils-5.0 and earlier, a would not be removed.
-test -r a && fail=1
+test -r a || fail=1
 test -r b || fail=1
 
-# Make sure it works also with --backup.
-ln b a
+# Make sure it works with --backup.
 mv --backup=simple a b || fail=1
 test -r a && fail=1
 test -r b || fail=1
diff --git a/tests/mv/hard-verbose.sh b/tests/mv/hard-verbose.sh
deleted file mode 100755
index 45491ab..0000000
--- a/tests/mv/hard-verbose.sh
+++ /dev/null
@@ -1,33 +0,0 @@
-#!/bin/sh
-# ensure that mv's --verbose options works even in this unusual case
-
-# Copyright (C) 2006-2014 Free Software Foundation, Inc.
-
-# This program is free software: you can redistribute it and/or modify
-# it under the terms of the GNU General Public License as published by
-# the Free Software Foundation, either version 3 of the License, or
-# (at your option) any later version.
-
-# This program is distributed in the hope that it will be useful,
-# but WITHOUT ANY WARRANTY; without even the implied warranty of
-# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-# GNU General Public License for more details.
-
-# You should have received a copy of the GNU General Public License
-# along with this program.  If not, see <http://www.gnu.org/licenses/>.
-
-. "${srcdir=.}/tests/init.sh"; path_prepend_ ./src
-print_ver_ mv
-
-touch x || framework_failure_
-ln x y || framework_failure_
-
-
-mv --verbose x y > out || fail=1
-cat <<\EOF > exp || fail=1
-removed 'x'
-EOF
-
-compare exp out || fail=1
-
-Exit $fail
diff --git a/tests/mv/i-4.sh b/tests/mv/i-4.sh
index b366bc4..ad79389 100755
--- a/tests/mv/i-4.sh
+++ b/tests/mv/i-4.sh
@@ -23,6 +23,7 @@ for i in a b; do
   echo $i > $i || framework_failure_
 done
 echo y > y || framework_failure_
+echo n > n || framework_failure_
 
 mv -i a b < y >/dev/null 2>&1 || fail=1
 
@@ -32,18 +33,15 @@ case "$(cat b)" in
   *) fail=1 ;;
 esac
 
-# Ensure that mv -i a b works properly with 'n' and 'y'
-# responses, even when a and b are hard links to the same file.
-# This 'n' test would fail (no prompt) for coreutils-5.0.1 through 5.3.0.
-echo n > n
+# Ensure that mv -i a b works properly with 'n' and 'y' responses,
+# when a and b are hard links to the same file.
 rm -f a b
 echo a > a
 ln a b
-mv -i a b < n >/dev/null 2>&1 || fail=1
+mv -i a b < y 2>err && fail=1
 test -r a || fail=1
 test -r b || fail=1
-mv -i a b < y >/dev/null 2>&1 || fail=1
-test -r a && fail=1
-test -r b || fail=1
+printf "mv: 'a' and 'b' are the same file\n" > exp
+compare exp err || fail=1
 
 Exit $fail
diff --git a/tests/mv/symlink-onto-hardlink-to-self.sh b/tests/mv/symlink-onto-hardlink-to-self.sh
index f3e8ff9..1a567df 100755
--- a/tests/mv/symlink-onto-hardlink-to-self.sh
+++ b/tests/mv/symlink-onto-hardlink-to-self.sh
@@ -1,6 +1,6 @@
 #!/bin/sh
-# Demonstrate that when moving a symlink onto a hardlink-to-that-symlink, the
-# source symlink is removed.  Depending on your kernel (e.g., Linux, Solaris,
+# Demonstrate that when moving a symlink onto a hardlink-to-that-symlink,
+# an error is presented.  Depending on your kernel (e.g., Linux, Solaris,
 # but not NetBSD), prior to coreutils-8.16, the mv would successfully perform
 # a no-op.  I.e., surprisingly, mv s1 s2 would succeed, yet fail to remove s1.
 
@@ -26,27 +26,34 @@ print_ver_ mv
 touch f || framework_failure_
 ln -s f s2 || framework_failure_
 
-for opt in '' --backup; do
+# Attempt to create a hard link to that symlink.
+# On some systems, it's not possible: they create a hard link to the referent.
+ln s2 s1 || framework_failure_
 
-  # Attempt to create a hard link to that symlink.
-  # On some systems, it's not possible: they create a hard link to the referent.
-  ln s2 s1 || framework_failure_
+# If s1 is not a symlink, skip this test.
+test -h s1 \
+  || skip_ your kernel or file system cannot create a hard link to a symlink
 
-  # If s1 is not a symlink, skip this test.
-  test -h s1 \
-    || skip_ your kernel or file system cannot create a hard link to a symlink
+for opt in '' --backup; do
 
-  mv $opt s1 s2 > out 2>&1 || fail=1
-  compare /dev/null out || fail=1
+  if test "$opt" = --backup; then
+    mv $opt s1 s2 > out 2>&1 || fail=1
+    compare /dev/null out || fail=1
 
-  # Ensure that s1 is gone.
-  test -e s1 && fail=1
+    # Ensure that s1 is gone.
+    test -e s1 && fail=1
 
-  if test "$opt" = --backup; then
     # With --backup, ensure that the backup file was created.
     ref=$(readlink s2~) || fail=1
     test "$ref" = f || fail=1
   else
+    echo "mv: 's1' and 's2' are the same file" > exp
+    mv $opt s1 s2 2>err && fail=1
+    compare exp err || fail=1
+
+    # Ensure that s1 is still present.
+    test -e s1 || fail=1
+
     # Without --backup, ensure there is no backup file.
     test -e s2~ && fail=1
   fi
-- 
2.1.0