bug#10686: mv: moving hardlink of a softlink to the softlink does nothing

[Jim Meyering]

Jim Meyering wrote:
> The above wasn't quite right in that it failed to honor mv's --backup
> option.  mv --backup s f would not have created the required backup file.
> I've adjusted it to fix that, and added tests to cover both cases.
> This is still not quite ready (i.e., the FIXME comment, where I'll add
> some explanation), but otherwise should be fine.
>
> Date: Wed, 1 Feb 2012 21:42:45 +0100
> Subject: [PATCH] mv: "mv A B" would sometimes succeed, yet A would remain,
>  ...

I address the FIXME and tweaked the comment: (A,B) seemed a little
clearer than (s1,s2).  I've also tightened up the test, regarding
--backup and existence of backup files.


>From de817cfa6e780323b3eac1f92ac81e8c7871d088 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Wed, 1 Feb 2012 21:42:45 +0100
Subject: [PATCH] mv: "mv A B" would sometimes succeed, yet A would remain, ...

But only when both A and B were hard links to the same symlink.
* src/copy.c (same_file_ok): Handle another special case: the one
in which we are moving a symlink onto a hard link to itself.
In this case, we must explicitly tell the caller to unlink the
source file.  Otherwise, at least the linux-3.x kernel rename
function would do nothing, as mandated by POSIX 2008.
* tests/mv/symlink-onto-hardlink-to-self: New test.
* tests/Makefile.am (TESTS): Add it.
* NEWS (Bug fixes): Mention it.
Reported by Bernhard Voelker in http://bugs.gnu.org/10686
---
 NEWS                                   |    5 +++
 src/copy.c                             |   29 +++++++++++++++--
 tests/Makefile.am                      |    1 +
 tests/mv/symlink-onto-hardlink-to-self |   56 ++++++++++++++++++++++++++++++++
 4 files changed, 88 insertions(+), 3 deletions(-)
 create mode 100755 tests/mv/symlink-onto-hardlink-to-self

diff --git a/NEWS b/NEWS
index 9eebbf6..ca8c0b5 100644
--- a/NEWS
+++ b/NEWS
@@ -11,6 +11,11 @@ GNU coreutils NEWS                                    -*- 
outline -*-
   referent, there is no risk of data loss, since the symlink will
   typically still point to one of the hard links.

+  "mv A B" could succeed, yet A would remain.  This would happen only when
+  both A and B were hard links to the same symlink, and with a kernel for
+  which rename("A","B") would do nothing and return 0.  Now, in this
+  unusual case, mv does not call rename, and instead simply unlinks A.
+

 * Noteworthy changes in release 8.15 (2012-01-06) [stable]

diff --git a/src/copy.c b/src/copy.c
index 4810de8..541ca20 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -1226,10 +1226,33 @@ same_file_ok (char const *src_name, struct stat const 
*src_sb,
       same_link = same;

       /* If both the source and destination files are symlinks (and we'll
-         know this here IFF preserving symlinks), then it's ok -- as long
-         as they are distinct.  */
+         know this here IFF preserving symlinks), then it's usually ok
+         when they are distinct.  */
       if (S_ISLNK (src_sb->st_mode) && S_ISLNK (dst_sb->st_mode))
-        return ! same_name (src_name, dst_name);
+        {
+          bool sn = same_name (src_name, dst_name);
+          if ( ! sn)
+            {
+              /* It's fine when we're making any type of backup.  */
+              if (x->backup_type != no_backups)
+                return true;
+
+              /* 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.  */
+              if (same_link)
+                {
+                  *unlink_src = true;
+                  *return_now = true;
+                  return true;
+                }
+            }
+
+          return ! sn;
+        }

       src_sb_link = src_sb;
       dst_sb_link = dst_sb;
diff --git a/tests/Makefile.am b/tests/Makefile.am
index a94aaa2..c951f69 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -492,6 +492,7 @@ TESTS =                                             \
   mv/partition-perm                            \
   mv/perm-1                                    \
   mv/symlink-onto-hardlink                     \
+  mv/symlink-onto-hardlink-to-self             \
   mv/to-symlink                                        \
   mv/trailing-slash                            \
   mv/update                                    \
diff --git a/tests/mv/symlink-onto-hardlink-to-self 
b/tests/mv/symlink-onto-hardlink-to-self
new file mode 100755
index 0000000..efaff66
--- /dev/null
+++ b/tests/mv/symlink-onto-hardlink-to-self
@@ -0,0 +1,56 @@
+#!/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., with the linux
+# kernel), 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.
+
+# Copyright (C) 2012 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=.}/init.sh"; path_prepend_ ../src
+print_ver_ mv
+
+# Create a file f, and a symlink s1 to that file.
+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_
+
+  # 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
+
+  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
+
+  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
+    # Without --backup, ensure there is no backup file.
+    test -e s2~ && fail=1
+  fi
+
+done
+
+Exit $fail
--
1.7.9.190.gf1d33