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

[Pádraig Brady]

On 10/20/2013 02:11 AM, Bernhard Voelker wrote:
> Hello Gian,
> 
> On 08/23/2013 11:40 PM, Gian Piero Carrubba wrote:
>> 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.
> 
> thanks for your bug report, the thorough thoughts about it and the
> patch proposal.

This looks like a valid issue...

>> $ 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'.
> 
> I'd also say that the above is an error ... or at least contradicts
> to what is documented in the code (copy.c, line 2377):
> 
>    Yet cp, invoked with '--link --no-dereference',
>    should not follow the link.

Yes I would think that `ln -L` and `cp --link -L` should
be equivalent in this regard.

>> --- 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.  */
> 
> Instead of passing 'flags' to linkat (of which I don't know if it works
> on all platforms), I'd rather dereference the symlink manually:
> 
>   char *src_name_new = canonicalize_filename_mode (src_name, CAN_EXISTING);

Actually since linkat() has defined semantics,
_and thanks to gnulib, guaranteed semantics_
we should just leverage the gnulib magic here,
and use linkat with flags.

>> 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
> 
> Not ideal ...
> 
>> 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;
>>      }

Interesting. You'd nearly think it should be the
other way around, given linking to symlinks when
copying to other directories, may give invalid
symlinks if they link outside the copied hierarchy.


> Good analysis!
> My guess is that this because POSIX [1] allows either behavior:
> 
>   If source_file is a file of type symbolic link:
>   [...]
>   If the -R option was specified:
>   If none of the options -H, -L, nor -P were specified, it is
>   unspecified which of -H, -L, or -P will be used as a default.
> 
> [1] http://pubs.opengroup.org/onlinepubs/9699919799/utilities/cp.html
> 
> This means that coreutils has chosen in between what makes most sense
> and compatibility to other implementations.
> 
>> 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]
> 
> I think that is correct as POSIX requires this (see also in [1] above).
> 
> Instead, I'd change the above initialization of x.dereference in the
> case the --link option is specified.
> 
> I have wrapped it for you into a Git patch (below), yet without a test
> case.
> 
> This is the result:
> 
>   $ : > file; ln -s file link
>   $ for opt in "" -l -lH -lL -lP -r ; do cp $opt link cp$opt ; done
>   $ ls -ldogi link file cp*

Nice basis for a test.

>   15335993 -rw-r--r-- 1 0 Oct 20 03:09 cp
>   15335992 lrwxrwxrwx 3 4 Oct 20 03:08 cp-l -> file
>   15335991 -rw-r--r-- 3 0 Oct 20 03:08 cp-lH
>   15335991 -rw-r--r-- 3 0 Oct 20 03:08 cp-lL
>   15335992 lrwxrwxrwx 3 4 Oct 20 03:08 cp-lP -> file
>   15335994 lrwxrwxrwx 1 4 Oct 20 03:09 cp-r -> file
>   15335991 -rw-r--r-- 3 0 Oct 20 03:08 file
>   15335992 lrwxrwxrwx 3 4 Oct 20 03:08 link -> file

The above look correct.

For comparison on FreeBSD 9.1

1755429 -rw-r--r--  1 pbrady  pbrady  - 0 Oct 29 12:16 cp
1755427 -rw-r--r--  5 pbrady  pbrady  - 0 Oct 29 12:16 cp-l
1755427 -rw-r--r--  5 pbrady  pbrady  - 0 Oct 29 12:16 cp-lH
1755427 -rw-r--r--  5 pbrady  pbrady  - 0 Oct 29 12:16 cp-lL
1755427 -rw-r--r--  5 pbrady  pbrady  - 0 Oct 29 12:16 cp-lP
1755430 -rw-r--r--  1 pbrady  pbrady  - 0 Oct 29 12:16 cp-r
1755427 -rw-r--r--  5 pbrady  pbrady  - 0 Oct 29 12:16 file
1755428 lrwxr-xr-x  1 pbrady  pbrady  - 4 Oct 29 12:16 link -> file

And from older 8.10 GNU cp we have:

2368286 -rw-rw-r--. 1 0 Oct 29 18:19 cp
2368262 lrwxrwxrwx. 4 4 Oct 29 18:19 cp-l -> file
2368262 lrwxrwxrwx. 4 4 Oct 29 18:19 cp-lH -> file
2368262 lrwxrwxrwx. 4 4 Oct 29 18:19 cp-lL -> file
2368298 lrwxrwxrwx. 1 4 Oct 29 18:19 cp-lP -> file
2368299 lrwxrwxrwx. 1 4 Oct 29 18:19 cp-r -> file
2368218 -rw-rw-r--. 1 0 Oct 29 18:19 file
2368262 lrwxrwxrwx. 4 4 Oct 29 18:19 link -> file

In both the above we seem to be just getting the default
dereferencing behavior of link(1).

> WDYT?
> 
> BTW: tests/cp/same-file.sh now fails with the patch.

Yes better to consolidate the functionality,
before doing the tests to avoid awkward test churn.

> Have a nice day,
> Berny
> 
> 
>>From fca06077e11d6338b35029bc61fbee5dbfb5ab66 Mon Sep 17 00:00:00 2001
> From: Gian Piero Carrubba <address@hidden>
> Date: Sun, 20 Oct 2013 03:04:41 +0200
> Subject: [PATCH] cp: fix --link --dereference [DRAFT]
> 
> * src/copy.c (create_hard_link): Add a bool 'dereference' parameter,
> and canonicalize the given src_name when dereference is true.
> (copy_internal): Add a value for the above new parameter in all
> three calls to create_hard_link: true if the src_name should be
> dereferenced before creating the hard link, false otherwise.
> * src/cp.c (main): after parsing the options, if x.dereference is
> still DEFEF_UNDEFINED and the --links option was specified, initialize

s/--links/--link/

> x.dereference to DEREF_NEVER.
> * NEWS (Changes in behavior): Mention the change.
> 
> This fixes http://bugs.gnu.org/15173
> ---
>  NEWS       |  4 ++++
>  src/copy.c | 42 ++++++++++++++++++++++++++++++------------
>  src/cp.c   |  2 +-
>  3 files changed, 35 insertions(+), 13 deletions(-)
> 
> diff --git a/NEWS b/NEWS
> index 39dd3d6..c1d637c 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -70,6 +70,10 @@ GNU coreutils NEWS                                    -*- 
> outline -*-
> 
>  ** Changes in behavior
> 
> +  cp --link --dereference now dereferences a symbolic link as source files
> +  before creating the hard link in the destination.
> +  Previously, it would create a hard link of the symbolic link.
> +
>    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..fa4d734 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 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)
>  {
> +  char *src_deref = NULL;
> +  if (dereference)
> +    src_deref = canonicalize_filename_mode (src_name, CAN_EXISTING);

Probably best avoided as mentioned above.

>    /* We want to guarantee that symlinks are not followed.  */
> -  bool link_failed = (linkat (AT_FDCWD, src_name, AT_FDCWD, dst_name, 0) != 
> 0);
> +  bool link_failed = (linkat (AT_FDCWD, dereference ? src_deref : src_name,
> +                              AT_FDCWD, dst_name, 0) != 0);
> 
>    /* If the link failed because of an existing destination,
>       remove that file and then call link again.  */
> @@ -1578,13 +1583,17 @@ create_hard_link (char const *src_name, char const 
> *dst_name,
>        if (unlink (dst_name) != 0)
>          {
>            error (0, errno, _("cannot remove %s"), quote (dst_name));
> +          free (src_deref);
>            return false;
>          }
>        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, dereference ? src_deref : src_name,
> +                             AT_FDCWD, dst_name, 0) != 0);
>      }
> 
> +  free (src_deref);
> +
>    if (link_failed)
>      {
>        error (0, errno, _("cannot create hard link %s to %s"),
> @@ -1748,7 +1757,10 @@ 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,
> +                                              x->dereference == DEREF_ALWAYS
> +                                              || (x->dereference == 
> DEREF_COMMAND_LINE_ARGUMENTS
> +                                                  && command_line_arg)))
>                          {
>                            goto un_backup;
>                          }
> @@ -2078,7 +2090,10 @@ 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,
> +                                  x->dereference == DEREF_ALWAYS
> +                                  || (x->dereference == 
> DEREF_COMMAND_LINE_ARGUMENTS
> +                                      && command_line_arg)))
>              goto un_backup;
> 
>            return true;
> @@ -2389,7 +2404,10 @@ 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,
> +                              x->dereference == DEREF_ALWAYS
> +                              || (x->dereference == 
> DEREF_COMMAND_LINE_ARGUMENTS
> +                                  && command_line_arg)))

Lots and copy and paste there.
Maybe use a helper like:

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);
}

>          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
> 

Thanks to both of you for looking into this awkward issue.

Pádraig.