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

From: Bernhard Voelker <[email-masked]>
Date: Thu, 31 Oct 2013 03:07:33 +0100

On 10/30/2013 01:13 PM, Bernhard Voelker wrote:
> Thanks again for looking into this.  It's really quite complex.
> I'll come up with the test (old vs. new) cases soon.

Okay, here we go:

the attached patch is updated regarding the issues you mentioned.

I furthermore tweaked the NEWS entry:

* instead of just mentioning the --dereference option,
mention both the -L and -H options explicitly.

* add a note that "cp -l dangling" and "cp -l symlink-to-dir"
now create hard links as expected - cp doesn't try to dereference
the src anymore which would lead to different error diagnostics
in the two cases [*].

Then, I created a few test cases to check the changes in behaviour
of cp-8.21 against the changed cp.  It runs various combinations of
the options -l, -s, -R, -H, -L, -P and --preserve=links on symlinks
to a file ('filelink'), to a directory ('dirlink') and on a dangling
symlink ('danglink').
The script is already ugly enough, so I omitted checking the
files, dir and links in the subdirectory for changes.
The changes so far - 12 out of 144 - are:

  $ grep "DIFFERENCE in Test: " testit.log | sed 's/^.*: //'
  cp -L -l   filelink ...
  cp -L -l  --preserve=links filelink ...
  cp -L -l -R  filelink ...
  cp -L -l -R --preserve=links filelink ...
  cp -H -l   filelink ...
  cp -H -l  --preserve=links filelink ...
  cp -H -l -R  filelink ...
  cp -H -l -R --preserve=links filelink ...
  cp  -l   dirlink ...
  cp  -l  --preserve=links dirlink ...
  cp  -l   danglink ...
  cp  -l  --preserve=links danglink ...

I think these are all okay.

[*] IMO even the 4 latter changes are okay and don't contradict to
POSIX: --link is not specified by POSIX.

BTW: The failures "can make relative symbolic links only in current directory"
for e.g. "cp -L -s -R  dirlink" are interesting but a different case, i.e.,
cp's behavior didn't change.

WDYT?

Finally, the testsuite is still broken (tests/cp/same-file), and new
tests for "cp -l[LH]" are also still missing.

Have a nice day,
Berny
>From 49d2687d0f961f7df3d8e32995ff054c72fbf1d9 Mon Sep 17 00:00:00 2001
From: Bernhard Voelker <[email-masked]>
Date: Thu, 31 Oct 2013 02:24:03 +0100
Subject: [PATCH] cp: fix --link with dereferencing options -L or -H [DRAFT]

* 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 pass its
return value as the new parameter 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 --link option was specified, initialize
x.dereference to DEREF_NEVER.
* NEWS (Changes in behavior): Mention the change.

This fixes http://bugs.gnu.org/15173
---
 NEWS       |  7 +++++++
 src/copy.c | 47 ++++++++++++++++++++++++++++++++++-------------
 src/cp.c   |  2 +-
 3 files changed, 42 insertions(+), 14 deletions(-)

diff --git a/NEWS b/NEWS
index 39dd3d6..cbd837d 100644
--- a/NEWS
+++ b/NEWS
@@ -70,6 +70,13 @@ GNU coreutils NEWS                                    -*- outline -*-
 
 ** Changes in behavior
 
+  cp --link together with -H or -L now dereferences a symbolic link as source
+  before creating the hard link in the destination.  Previously, it would create
+  a hard link of the symbolic link.
+  Similarly, cp --link for a dangling symbolic link or for a symbolic link to a
+  directory is now hard linked correctly.  Previously, cp would fail with an
+  error diagnostic.
+
   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..e19fb34 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -1558,18 +1558,24 @@ 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 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);
+  int flags;
+  if (dereference)
+    flags = AT_SYMLINK_FOLLOW;
+  else
+    flags = 0; /* We want to guarantee that symlinks are not followed.  */
+
+  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 +1588,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 +1602,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 CLI_ARG; otherwise return false.  */
+static inline bool _GL_ATTRIBUTE_PURE
+should_dereference (const struct cp_options *x, bool cli_arg)
+{
+  return x->dereference == DEREF_ALWAYS
+         || (x->dereference == DEREF_COMMAND_LINE_ARGUMENTS
+             && cli_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
@@ -1747,8 +1765,9 @@ 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.  */
+                      bool deref = should_dereference (x, command_line_arg);
                       if (! create_hard_link (earlier_file, dst_name, true,
-                                              x->verbose))
+                                              x->verbose, deref))
                         {
                           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,
+                                  should_dereference (x, command_line_arg)))
             goto un_backup;
 
           return true;
@@ -2389,7 +2409,8 @@ 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,
+                              should_dereference (x, command_line_arg)))
         goto un_backup;
     }
   else if (S_ISREG (src_mode)
diff --git a/src/cp.c b/src/cp.c
index 7bc8630..6efb152 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
-- 
1.8.3.1