bug#31364: cp -rfs: Fails to overwrite a symlink when it is on a different device

[Pádraig Brady]

On 04/05/18 16:37, Illia Bobyr wrote:
> Hello,
> 
> I have found a bug in "cp -rfs".
> 
> Steps to reproduce:
> 
> 1. Given "path1" and "path2" are on different devices.
> 2. $ touch "path1/file"
> 3. $ cd path2/; ln -s path1/file
> 4. $ cp --symbolic-link --force --recursive path1/file .
> 
> Expected:
> The link is overwritten with an exact copy.
> 
> Actual result:
> cp shows an error:
>     cp: 'path1/file' and './file' are the same file
> 
> This bug was introduced in
> 
> http://git.savannah.gnu.org/cgit/coreutils.git/commit/?id=376967889ed7ed561e46ff6d88a66779db62737a
> 
> Specifically this hunk:
> 
> diff --git a/src/copy.c b/src/copy.c
> index e3832c2..9dbd536 100644
> --- a/src/copy.c
> <http://git.savannah.gnu.org/cgit/coreutils.git/tree/src/copy.c?id=2f69dba5df8caaf9eda658c1808b1379e9949f22>
> +++ b/src/copy.c
> <http://git.savannah.gnu.org/cgit/coreutils.git/tree/src/copy.c?id=376967889ed7ed561e46ff6d88a66779db62737a>
> @@ -46,6 +46,7 @@
> #include "file-set.h"
> #include "filemode.h"
> #include "filenamecat.h"
> +#include "force-link.h"
> #include "full-write.h"
> #include "hash.h"
> #include "hash-triple.h"
> @@ -1623,11 +1624,13 @@ same_file_ok (char const *src_name, struct stat
> const *src_sb,
> }
> }
> - /* It's ok to remove a destination symlink. But that works only when we
> - unlink before opening the destination and when the source and destination
> - files are on the same partition. */
> - if (x->unlink_dest_before_opening
> - && S_ISLNK (dst_sb_link->st_mode))
> + /* It's ok to remove a destination symlink. But that works only
> + when creating symbolic links, or when the source and destination
> + are on the same file system and when creating hard links or when
> + unlinking before opening the destination. */
> + if (x->symbolic_link
> + || ((x->hard_link || x->unlink_dest_before_opening)
> + && S_ISLNK (dst_sb_link->st_mode)))
> return dst_sb_link->st_dev == src_sb_link->st_dev;
> if (x->dereference == DEREF_NEVER)
> 
> Two patches that fix the issue are attached.
> They are against the current master in
> https://github.com/coreutils/coreutils
> The changes are also here:
> 
> https://github.com/coreutils/coreutils/compare/master...ilya-bobyr:master
> 
> Thank you, Illia Bobyr
> 
Thanks for the careful analysis of this hairy code.

Your use case works with --remove, which is similar
to the very recent https://bugs.gnu.org/31335
which also requests --force to replace symlinks.
Your case is slightly different and a change from previous behavior.

Note the specific reason for the change is not the
hunk you mentioned, but this hunk in cp.c which used to
make --force --symlink also imply --remove.

  /* If --force (-f) was specified and we're in link-creation mode,
     first remove any existing destination file.  */
  if (x.unlink_dest_after_failed_open && (x.hard_link || x.symbolic_link))
    x.unlink_dest_before_opening = true;

That was also changed in the commit you referenced,
as we no longer unconditionally unlink() for atomicity reasons.
I.E. we now create a temp symlink and rename it over
the existing destination.  If you don't require these new
guarantees then --remove will work for your use case.

So back to the hunk you mentioned.
I think the logic in the hunk you referenced was suspect
before commit 3769678 and only became significant as now
hit due to --remove no longer being set unconditionally for -sf.
Your analysis wrt x->symbolic_link being independent
of the src and dst devices is sound.

Though things aren't quite right as one can now nuke a file like:

  $ touch file
  $ ln -s file l1
  $ cp -s -f l1 file

That would be a regression in commit 3769678 not in your change.

What I've currently have to address both these cases is:

diff --git a/src/copy.c b/src/copy.c
index 4998c83..806323e 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -1627,14 +1627,17 @@ same_file_ok (char const *src_name, struct stat const 
*src_sb,
         }
     }

-  /* It's ok to remove a destination symlink.  But that works only
-     when creating symbolic links, or when the source and destination
-     are on the same file system and when creating hard links or when
-     unlinking before opening the destination.  */
-  if (x->symbolic_link
-      || ((x->hard_link || x->unlink_dest_before_opening)
-          && S_ISLNK (dst_sb_link->st_mode)))
-    return dst_sb_link->st_dev == src_sb_link->st_dev;
+  if (S_ISLNK (dst_sb_link->st_mode))
+    {
+      /* It's ok to replace a destination symlink. */
+      if (x->symbolic_link)
+        return true;
+
+      /* Or when hard links are possible.
+         TODO: Analyze this case further.  */
+      if (x->hard_link)
+        return dst_sb_link->st_dev == src_sb_link->st_dev;
+    }

   if (x->dereference == DEREF_NEVER)
     {

I'll test a few more cases, and add tests and NEWS.

Thanks again for pinpointing these issues!

Pádraig