Subject: [cp] --link overrides dereference settings

From: Gian Piero Carrubba <[email-masked]>
Date: Fri, 23 Aug 2013 23:40:34 +0200

Please keep me in Cc: as I'm not subscribed.

I failed to find references to discussions about this (intended?) 
behaviour, so I'm filing this report. Please forgive me if I've missed 
something elementary.

$ echo testfile > file; ln -s file link; opts="-l -Ll -Hl";
for o in $opts; do cp $o link cp${o}; done;
ls -li

total 4
3358742 lrwxrwxrwx 4 gpiero gpiero 4 Aug 23 22:27 cp-Hl -> file
3358742 lrwxrwxrwx 4 gpiero gpiero 4 Aug 23 22:27 cp-Ll -> file
3358742 lrwxrwxrwx 4 gpiero gpiero 4 Aug 23 22:27 cp-l -> file
3358741 -rw-r--r-- 1 gpiero gpiero 9 Aug 23 22:27 file
3358742 lrwxrwxrwx 4 gpiero gpiero 4 Aug 23 22:27 link -> file

I would have expected both 'cp-Hl' and 'cp-Ll' to be hardlinked to 
'file', not to 'link'.

$ cp --version | head -n1
cp (GNU coreutils) 8.21

$ uname -a
Linux caimano 3.10-2-amd64 #1 SMP Debian 3.10.5-1 (2013-08-07) x86_64 GNU/Linux

$ readlink -f /lib/x86_64-linux-gnu/libc.so.6                                                               
/lib/x86_64-linux-gnu/libc-2.17.so

A tentative draft patch is attached. It still lacks unit tests and a way 
to determine if the implementation of linkat() supports flags.
But most of all, please note that I'm not fully aware of the 
implications (including portability issues) of such a change.

AFAICS, at least in one case the patch changes the behaviour of cp:

$ echo testfile > file; ln -s file link;
cp -l link cp-l.old; ../src/cp -l link cp-l.new;
ls -li

total 8
8912975 -rw-r--r-- 2 gpiero gpiero 9 Aug 23 22:58 cp-l.new
8913010 lrwxrwxrwx 2 gpiero gpiero 4 Aug 23 22:58 cp-l.old -> file
8912975 -rw-r--r-- 2 gpiero gpiero 9 Aug 23 22:58 file
8913010 lrwxrwxrwx 2 gpiero gpiero 4 Aug 23 22:58 link -> file

This is caused by the following code:

$ tail -n+1135 src/cp.c | head -n8
   if (x.dereference == DEREF_UNDEFINED)
     {
       if (x.recursive)
         /* This is compatible with FreeBSD.  */
         x.dereference = DEREF_NEVER;
       else
         x.dereference = DEREF_ALWAYS;
     }

Not sure why this snippet is there[0], but it seems to me that it leads 
to inconsistent behaviour:

$ echo testfile > file; ln -s file link;
cp link cp; cp -r link cp-r;
ls -li

total 8
3358743 -rw-r--r-- 1 gpiero gpiero 9 Aug 23 23:06 cp
3358744 lrwxrwxrwx 1 gpiero gpiero 4 Aug 23 23:06 cp-r -> file
3358741 -rw-r--r-- 1 gpiero gpiero 9 Aug 23 23:06 file
3358742 lrwxrwxrwx 1 gpiero gpiero 4 Aug 23 23:06 link -> file

I think it is counterintuitive that '--recursive' had effects on the 
referentiation of the link.[1]

Anyway, given that x.dereference is set to DEREF_ALWAYS, I think the 
modified behaviour of `cp -l` could be considered correct.

Thanks,
Gian Piero.

[0] I suspect this is for backward compatibility. In principle, I think 
DEREF_UNDEF should always set DEREF_NEVER, but this change would modify 
a common, and well spread, behaviour.

[1] The same stands for '--link', as demonstrated in the first example, 
when 'cp-l' has been created as a hardlink to the symlink (even if for 
different reasons: in that case DEREF_ALWAYS was in place, but linkat() 
didn't follow the symlink).
Fri Aug 23 20:29:12 CEST 2013  Gian Piero Carrubba <[email-masked]>
  * DRAFT
diff -rN -u old-coreutils/src/copy.c new-coreutils/src/copy.c
--- old-coreutils/src/copy.c	2013-08-23 22:16:15.938940800 +0200
+++ new-coreutils/src/copy.c	2013-08-23 22:16:15.942940823 +0200
@@ -1566,10 +1566,15 @@
    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 +1587,7 @@
         }
       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)
@@ -1748,7 +1753,10 @@
                       /* 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,
+                                              x->dereference == DEREF_ALWAYS
+                                              || (x->dereference == DEREF_COMMAND_LINE_ARGUMENTS
+                                                  && command_line_arg)))
                         {
                           goto un_backup;
                         }
@@ -2078,7 +2086,10 @@
         }
       else
         {
-          if (! create_hard_link (earlier_file, dst_name, true, x->verbose))
+          if (! create_hard_link (earlier_file, dst_name, true, x->verbose,
+                                  x->dereference == DEREF_ALWAYS
+                                  || (x->dereference == DEREF_COMMAND_LINE_ARGUMENTS
+                                      && command_line_arg)))
             goto un_backup;
 
           return true;
@@ -2389,7 +2400,10 @@
            && !(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,
+                              x->dereference == DEREF_ALWAYS
+                              || (x->dereference == DEREF_COMMAND_LINE_ARGUMENTS
+                                  && command_line_arg)))
         goto un_backup;
     }
   else if (S_ISREG (src_mode)