Subject: bug#15173: [cp] --link overrides dereference settings

From: Bernhard Voelker <[email-masked]>
Date: Tue, 05 Nov 2013 00:02:47 +0100

On 11/04/2013 10:37 AM, Pádraig Brady wrote:
> On 11/04/2013 12:48 AM, Bernhard Voelker wrote:
>> It got late again. I'll have a look tomorrow again - maybe there
>> are no side effects with the following:
>>
>> diff --git a/src/cp.c b/src/cp.c
>> index 7bc8630..78c0a04 100644
>> --- a/src/cp.c
>> +++ b/src/cp.c
>> @@ -1135,7 +1135,7 @@ main (int argc, char **argv)
>>
>>    if (x.dereference == DEREF_UNDEFINED)
>>      {
>> -      if (x.recursive)
>> +      if (x.recursive && ! x.hard_link)
>>          /* This is compatible with FreeBSD.  */
>>          x.dereference = DEREF_NEVER;
>>        else
> 
> Yes I didn't consider -R in the table as I don't see any reason
> for it to behave differently when -l is specified (and -l aplies
> neither to POSIX or BSD (comment)).  So the above adjustment
> looks correct to me.

Indeed, that adjustment seems to be the right one - finally. ;-)
I have added it to the patch as well as a new test case. I've added
a "Co-authored-by:"-me line for that.

@Gian: please note that we use "double-blank after-dot" format in
comments. I've fixed that for you.

The patch passes make check and syntax-check.

I've also attached the test result cp-8.21 versus the new one.
These are the differences (I leave out the --preserve=links cases
as these are also affected but unrelated):

COMMAND                   | 8.21 | NEW
cp  -l   filelink ...     |  S   |  T
cp  -l -R  filelink ...   |  S   |  T
cp -L -l   filelink ...   |  S   |  T
cp -L -l -R  filelink ... |  S   |  T
cp -H -l   filelink ...   |  S   |  T
cp -H -l -R  filelink ... |  S   |  T
cp  -l -R  dirlink ...    |  S   |  NDH
cp  -l -R  danglink ...   |  S   |  ERR_DEREF

  NDH = New directory with hard links below
  S = hard link to symlink
  T = hard link to symlink target
  ERR_DEREF = failure because dereferencing fails

Now, the dereferencing behavior of cp --link is not related to -R anymore.
I hope we don't introduce a regression ...

Comments?

Have a nice day,
Berny
>From c29ed31b9bbdb8182758bc4e51604d19421bc1ec Mon Sep 17 00:00:00 2001
From: Bernhard Voelker <[email-masked]>
Date: Mon, 4 Nov 2013 23:37:01 +0100
Subject: [PATCH] cp: fix --link regarding the dereferencing of symbolic links

* src/copy.c (create_hard_link): Add a bool 'dereference' parameter,
and pass AT_SYMLINK_FOLLOW as 'flags' to linkat() when dereference
is true.
(should_dereference): Add new 'bool' function to determine if a
file should be dereferenced or not.
(copy_internal): Use the above new should_dereference() and remember
its return value in a new local bool 'dereference' variable.  Use that
in all three calls to create_hard_link().
* src/cp.c (main): after parsing the options, if x.dereference is
still DEFEF_UNDEFINED and the x.recursive is true, then only set
x.dereference to DEREF_NEVER iff --link was not specified.
* tests/cp/same-file.sh: Adapt the expected results for the -fl,
the -bl and the -bfl tests.
* tests/cp/link-deref.sh: Add a new test.
* tests/local.mk (all_tests): Reference the above new test.
* NEWS (Changes in behavior): Mention the change.

This fixes http://bugs.gnu.org/15173

Co-authored-by: Bernhard Voelker <[email-masked]>
---
 NEWS                   |   5 +++
 src/copy.c             |  46 +++++++++++++++------
 src/cp.c               |   2 +-
 tests/cp/link-deref.sh | 110 +++++++++++++++++++++++++++++++++++++++++++++++++
 tests/cp/same-file.sh  |   6 +--
 tests/local.mk         |   1 +
 6 files changed, 153 insertions(+), 17 deletions(-)
 create mode 100755 tests/cp/link-deref.sh

diff --git a/NEWS b/NEWS
index 39dd3d6..7333488 100644
--- a/NEWS
+++ b/NEWS
@@ -70,6 +70,11 @@ GNU coreutils NEWS                                    -*- outline -*-
 
 ** Changes in behavior
 
+  cp --link now dereferences a symbolic link as source before creating the
+  hard link in the destination unless explicitly requested via the -P option.
+  Previously, it would create a hard link of the symbolic link - even when one
+  of the dereferencing options -L or -H was specified, too.
+
   dd status=none now suppresses all non fatal diagnostic messages,
   not just the transfer counts.
 
diff --git a/src/copy.c b/src/copy.c
index f66ab46..bcae123 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -1558,18 +1558,23 @@ restore_default_fscreatecon_or_die (void)
            _("failed to restore the default file creation context"));
 }
 
-/* Create a hard link DST_NAME to SRC_NAME, honoring the REPLACE and
-   VERBOSE settings.  Return true upon success.  Otherwise, diagnose
-   the failure and return false.
-   If SRC_NAME is a symbolic link it will not be followed.  If the system
-   doesn't support hard links to symbolic links, then DST_NAME will
-   be created as a symbolic link to SRC_NAME.  */
+/* Create a hard link DST_NAME to SRC_NAME, honoring the REPLACE, VERBOSE and
+   DEREFERENCE settings.  Return true upon success.  Otherwise, diagnose the
+   failure and return false.  If SRC_NAME is a symbolic link, then it will not
+   be followed unless DEREFERENCE is true.
+   If the system doesn't support hard links to symbolic links, then DST_NAME
+   will be created as a symbolic link to SRC_NAME.  */
 static bool
 create_hard_link (char const *src_name, char const *dst_name,
-                  bool replace, bool verbose)
+                  bool replace, bool verbose, bool dereference)
 {
-  /* We want to guarantee that symlinks are not followed.  */
-  bool link_failed = (linkat (AT_FDCWD, src_name, AT_FDCWD, dst_name, 0) != 0);
+  /* We want to guarantee that symlinks are not followed, unless requested.  */
+  int flags = 0;
+  if (dereference)
+    flags = AT_SYMLINK_FOLLOW;
+
+  bool link_failed = (linkat (AT_FDCWD, src_name, AT_FDCWD, dst_name, flags)
+                      != 0);
 
   /* If the link failed because of an existing destination,
      remove that file and then call link again.  */
@@ -1582,7 +1587,8 @@ create_hard_link (char const *src_name, char const *dst_name,
         }
       if (verbose)
         printf (_("removed %s\n"), quote (dst_name));
-      link_failed = (linkat (AT_FDCWD, src_name, AT_FDCWD, dst_name, 0) != 0);
+      link_failed = (linkat (AT_FDCWD, src_name, AT_FDCWD, dst_name, flags)
+                     != 0);
     }
 
   if (link_failed)
@@ -1595,6 +1601,17 @@ create_hard_link (char const *src_name, char const *dst_name,
   return true;
 }
 
+/* Return true if the current file should be (tried to be) dereferenced:
+   either for DEREF_ALWAYS or for DEREF_COMMAND_LINE_ARGUMENTS in the case
+   where the current file is a COMMAND_LINE_ARG; otherwise return false.  */
+static inline bool _GL_ATTRIBUTE_PURE
+should_dereference (const struct cp_options *x, bool command_line_arg)
+{
+  return x->dereference == DEREF_ALWAYS
+         || (x->dereference == DEREF_COMMAND_LINE_ARGUMENTS
+             && command_line_arg);
+}
+
 /* Copy the file SRC_NAME to the file DST_NAME.  The files may be of
    any type.  NEW_DST should be true if the file DST_NAME cannot
    exist because its parent directory was just created; NEW_DST should
@@ -1670,6 +1687,8 @@ copy_internal (char const *src_name, char const *dst_name,
       record_file (x->src_info, src_name, &src_sb);
     }
 
+  bool dereference = should_dereference (x, command_line_arg);
+
   if (!new_dst)
     {
       /* Regular files can be created by writing through symbolic
@@ -1748,7 +1767,7 @@ copy_internal (char const *src_name, char const *dst_name,
                       /* Note we currently replace DST_NAME unconditionally,
                          even if it was a newer separate file.  */
                       if (! create_hard_link (earlier_file, dst_name, true,
-                                              x->verbose))
+                                              x->verbose, dereference))
                         {
                           goto un_backup;
                         }
@@ -2078,7 +2097,8 @@ copy_internal (char const *src_name, char const *dst_name,
         }
       else
         {
-          if (! create_hard_link (earlier_file, dst_name, true, x->verbose))
+          if (! create_hard_link (earlier_file, dst_name, true, x->verbose,
+                                  dereference))
             goto un_backup;
 
           return true;
@@ -2389,7 +2409,7 @@ copy_internal (char const *src_name, char const *dst_name,
            && !(LINK_FOLLOWS_SYMLINKS && S_ISLNK (src_mode)
                 && x->dereference == DEREF_NEVER))
     {
-      if (! create_hard_link (src_name, dst_name, false, false))
+      if (! create_hard_link (src_name, dst_name, false, false, dereference))
         goto un_backup;
     }
   else if (S_ISREG (src_mode)
diff --git a/src/cp.c b/src/cp.c
index 7bc8630..78c0a04 100644
--- a/src/cp.c
+++ b/src/cp.c
@@ -1135,7 +1135,7 @@ main (int argc, char **argv)
 
   if (x.dereference == DEREF_UNDEFINED)
     {
-      if (x.recursive)
+      if (x.recursive && ! x.hard_link)
         /* This is compatible with FreeBSD.  */
         x.dereference = DEREF_NEVER;
       else
diff --git a/tests/cp/link-deref.sh b/tests/cp/link-deref.sh
new file mode 100755
index 0000000..105c464
--- /dev/null
+++ b/tests/cp/link-deref.sh
@@ -0,0 +1,110 @@
+#!/bin/sh
+# Exercise cp --link's behavior regarding the dereferencing of symbolic links.
+
+# Copyright (C) 2013 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_ cp
+
+mkdir dir              || framework_failure_
+cp --help > file       || framework_failure_
+ln -s dir     dirlink  || framework_failure_
+ln -s file    filelink || framework_failure_
+ln -s nowhere danglink || framework_failure_
+
+# printf format of the output line.
+outformat='%s|result=%s|inode=%s|type=%s|error=%s\n'
+
+for src in dirlink filelink danglink; do
+  # Get symlink's target.
+  tgt=$(readlink $src) || framework_failure_
+  # Get inodes and file type of the symlink (src) and its target (tgt).
+  # Note: this will fail for 'danglink'; catch it.
+  ino_src="$(stat -c '%i' $src)" || framework_failure_
+  typ_src="$(stat -c '%F' $src)" || framework_failure_
+  ino_tgt="$(stat -c '%i' $tgt 2>/dev/null)" || ino_tgt=
+  typ_tgt="$(stat -c '%F' $tgt 2>/dev/null)" || typ_tgt=
+
+  for o in '' -L -H -P; do
+    for r in '' -R; do
+
+      command="cp --link $o $r $src dst"
+      $command 2> err
+      result=$?
+
+      # Get inode and file type of the destination (which may fail, too).
+      ino_dst="$(stat -c '%i' dst 2>/dev/null)" || ini_dst=
+      typ_dst="$(stat -c '%F' dst 2>/dev/null)" || typ_dst=
+
+      # Print the actual result in a certain format.
+      printf "$outformat" \
+        "$command"   \
+        "$result"   \
+        "$ino_dst"  \
+        "$typ_dst"  \
+        "$(< err)"  \
+        > out
+
+      # What was expected?
+      if [ "$o" = "-P" ]; then
+        # cp --link should not dereference if -P is given.
+        exp_result=0
+        exp_inode=$ino_src
+        exp_ftype=$typ_src
+        exp_error=
+      elif [ "$src" = 'danglink' ]; then
+        # Dereferencing should fail for the 'danglink'.
+        exp_result=1
+        exp_inode=
+        exp_ftype=
+        exp_error="cp: cannot stat 'danglink': No such file or directory"
+      elif [ "$src" = 'dirlink' ] && [ "$r" != '-R' ]; then
+        # Dereferencing should fail for the 'dirlink' without -R.
+        exp_result=1
+        exp_inode=
+        exp_ftype=
+        exp_error="cp: omitting directory 'dirlink'"
+      elif [ "$src" = 'dirlink' ]; then
+        # cp --link -R 'dirlink' should create a new directory.
+        exp_result=0
+        exp_inode=$ino_dst
+        exp_ftype=$typ_dst
+        exp_error=
+      else
+        # cp --link 'filelink' should create a hard link to the target.
+        exp_result=0
+        exp_inode=$ino_tgt
+        exp_ftype=$typ_tgt
+        exp_error=
+      fi
+
+      # Print the expected result in a certain format.
+      printf "$outformat" \
+        "$command"   \
+        "$exp_result" \
+        "$exp_inode"  \
+        "$exp_ftype"  \
+        "$exp_error"  \
+        > exp
+
+      compare exp out || { ls -lid $src $tgt dst; fail=1; }
+
+      rm -rf dst err exp out || framework_failure_
+    done
+  done
+done
+
+Exit $fail
diff --git a/tests/cp/same-file.sh b/tests/cp/same-file.sh
index 003a62b..326f9c6 100755
--- a/tests/cp/same-file.sh
+++ b/tests/cp/same-file.sh
@@ -189,9 +189,9 @@ cat <<\EOF | sed "$remove_these_sed" > expected
 0 -bf (foo sl1 -> foo sl2 sl2.~1~ -> foo)
 0 -bdf (foo sl1 -> foo sl2 -> foo sl2.~1~ -> foo)
 1 -l [cp: cannot create hard link 'sl2' to 'sl1'] (foo sl1 -> foo sl2 -> foo)
-0 -fl (foo sl1 -> foo sl2 -> foo)
-0 -bl (foo sl1 -> foo sl2 -> foo sl2.~1~ -> foo)
-0 -bfl (foo sl1 -> foo sl2 -> foo sl2.~1~ -> foo)
+0 -fl (foo sl1 -> foo sl2)
+0 -bl (foo sl1 -> foo sl2 sl2.~1~ -> foo)
+0 -bfl (foo sl1 -> foo sl2 sl2.~1~ -> foo)
 
 1 [cp: 'foo' and 'hardlink' are the same file] (foo hardlink)
 1 -d [cp: 'foo' and 'hardlink' are the same file] (foo hardlink)
diff --git a/tests/local.mk b/tests/local.mk
index e18deac..3c92425 100644
--- a/tests/local.mk
+++ b/tests/local.mk
@@ -427,6 +427,7 @@ all_tests =					\
   tests/cp/file-perm-race.sh			\
   tests/cp/into-self.sh				\
   tests/cp/link.sh				\
+  tests/cp/link-deref.sh			\
   tests/cp/link-no-deref.sh			\
   tests/cp/link-preserve.sh			\
   tests/cp/link-symlink.sh			\
-- 
1.8.3.1