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

[Jim Meyering]

Jim Meyering wrote:

> Eric Blake wrote:
>
>> On 02/01/2012 05:47 AM, Jim Meyering wrote:
>>> Bernhard Voelker wrote:
>>>> Playing around with the latest mv checkin,
>>>> I noticed another corner case:
>>>>
>>>> Create a file 'f', a symlink 'l' to it, and
>>>> then a hardlink 's' to that symlink:
>>>>
>>>>   $ touch f && ln -s f l && ln l s && ls -ogi
>>>>   total 0
>>>>   6444 -rw-r--r-- 1 0 Feb  1 08:52 f
>>>>   6462 lrwxrwxrwx 2 1 Feb  1 08:52 l -> f
>>>>   6462 lrwxrwxrwx 2 1 Feb  1 08:52 s -> f
>>>
>>> Hi Bernhard,
>>> Thanks for the report.
>>>
>>> Here, you've already gotten into questionable territory, since the
>>> idea of a hard link to a symbolic link is not standardized.
>>
>> Actually, POSIX 2008 _did_ standardize the notion of a hard link to a
>> symlink, thanks to linkat().
>>
>>> For example, on FreeBSD 9.0, you cannot even create one:
>>
>> That's a POSIX-compliance bug in FreeBSD.  In that case, the best we can
>> do is emulate it by creating a new symlink with identical contents and
>> owner and as much of the timestamp as lutimens will allow.
>>
>>>
>>> It's a standards and kernel issue.
>>> POSIX explicitly says (of rename)
>>>
>>>     If the old argument and the new argument resolve to the same existing
>>>     file, rename( ) shall return successfully and perform no other action.
>>>
>>> though that particular wording might be slated to change with POSIX 2008,
>>> to allow different (more desirable) behavior.  Search the austin-group
>>> archives if you need to be sure.
>>
>> The wording for rename(2) did not change, but the wording for mv(1)
>> _did_ change to allow slightly more desirable behavior.  Here's the
>> latest wording, as modified by the latest bug report:
>>
>> http://austingroupbugs.net/view.php?id=534
>>
>> If the source_file operand and destination path resolve to either the
>> same existing directory entry or different directory entries for the
>> same existing file, then the destination path shall not be removed, and
>> one of the following shall occur:
>>  a. No change is made to source_file, no error occurs, and no diagnostic
>> is issued.
>>  b. No change is made to source_file, a diagnostic is issued to standard
>> error identifying the two names, and the exit status is affected.
>>  c. If the source_file operand and destination path name distinct
>> directory entries, then the source_file operand is removed, no error
>> occurs, and no diagnostic is issued.
>>
>> The mv utility shall do nothing more with the current source_file, and
>> go on to any remaining source_files.
>>
>>> Also, I don't know whether the above wording applies to this particular
>>> case of two hard links to the same symlink.  Again, I think we're in
>>> unspecified territory.
>>
>> POSIX actually specifies this quite well - two hardlinks to the same
>> symlink qualify as an instance of two file names that resolve to the
>> same inode after path resolution as checked with lstat().
>
> Thanks for the clarification and quotes, Eric.
>
> Bernhard, here's a better patch.
> With it, mv simply unlinks the "s" in your example:
>
> diff --git a/src/copy.c b/src/copy.c
> index 4810de8..73f5cc5 100644
> --- a/src/copy.c
> +++ b/src/copy.c
> @@ -1229,7 +1229,17 @@ same_file_ok (char const *src_name, struct stat const 
> *src_sb,
>           know this here IFF preserving symlinks), then it's ok -- as long
>           as 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 && same_link)
> +            {
> +              *unlink_src = true;
> +              *return_now = true;
> +              return true;
> +            }
> +
> +          return ! sn;
> +        }

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.


>From 646456b8ce053b85d30174462620492df648e0e2 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                             |   26 ++++++++++++++--
 tests/Makefile.am                      |    1 +
 tests/mv/symlink-onto-hardlink-to-self |   53 ++++++++++++++++++++++++++++++++
 4 files changed, 82 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..0764b8e 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -1226,10 +1226,30 @@ 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;
+
+              /* This is an unusual case.
+                 FIXME: explain
+               */
+              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..c8adc0b
--- /dev/null
+++ b/tests/mv/symlink-onto-hardlink-to-self
@@ -0,0 +1,53 @@
+#!/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
+
+  # With --backup, ensure that the backup file was created.
+  if test "$opt" = --backup; then
+    ref=$(readlink s2~) || fail=1
+    test "$ref" = f || fail=1
+  fi
+
+done
+
+Exit $fail
--
1.7.9.112.gb85f2