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

Previous Next

Full log

Message #8 received at 31364 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Illia Bobyr <ibobyr <at> google.com>, 31364 <at> debbugs.gnu.org
Cc: Jason Smith <jasonmsmith <at> google.com>
Subject: Re: bug#31364: cp -rfs: Fails to overwrite a symlink when it is on a
 different device
Date: Sat, 5 May 2018 20:18:32 -0700

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

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.