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

From: Illia Bobyr <[email-masked]>
Date: Fri, 04 May 2018 23:37:55 +0000

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
From 51d42245674dc864ac0dbb2f160a55cd38fc2458 Mon Sep 17 00:00:00 2001
From: Illia Bobyr <[email-masked]>
Date: Fri, 4 May 2018 09:41:27 -0700
Subject: [PATCH 2/2] cp: Overwrite symlinks on another device

When target of a copy operation is a symlink, it is not the same file as
the source, so it is safe to overwrite it, regardless of if it is the
same device as the source or a different device.

Previous check only allowed overwrite on the same device and produced an
error when run across different file systems.
---
 src/copy.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/src/copy.c b/src/copy.c
index dbfabf215..b35f9568b 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -1627,11 +1627,16 @@ 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 creating hard links.  */
-  if (x->symbolic_link
-      || (x->hard_link && S_ISLNK (dst_sb_link->st_mode)))
+  /* It's ok to remove a destination symlink - does not affect the source. */
+  if (x->symbolic_link)
+    return true;
+
+  /* When creating hard links it OK when the source and destination are on the
+     same file system.  TODO: The S_ISLNK() check is probably redundant, as we
+     should not be here if the source and the destination are the same file and
+     the destination is not a symbolic link.  The case when the source and the
+     destination are the same due to being hard links is checked above.  */
+  if (x->hard_link && S_ISLNK (dst_sb_link->st_mode))
     return dst_sb_link->st_dev == src_sb_link->st_dev;
 
   if (x->dereference == DEREF_NEVER)
-- 
2.17.0.441.gb46fe60e1d-goog

From a2b21f29edaa2c3448d6542870279d6217c43508 Mon Sep 17 00:00:00 2001
From: Illia Bobyr <[email-masked]>
Date: Thu, 3 May 2018 16:38:40 -0700
Subject: [PATCH 1/2] cp: No dup check for unlink_dest_after_failed_open

A case when unlink_dest_after_failed_open is set and the destination is
a symbolic link is checked in same_file_ok() in lines 1565-1572.  It
does not make sense to check again.
---
 src/copy.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/src/copy.c b/src/copy.c
index 4998c83e6..dbfabf215 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -1629,11 +1629,9 @@ 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.  */
+     are on the same file system and creating hard links.  */
   if (x->symbolic_link
-      || ((x->hard_link || x->unlink_dest_before_opening)
-          && S_ISLNK (dst_sb_link->st_mode)))
+      || (x->hard_link && S_ISLNK (dst_sb_link->st_mode)))
     return dst_sb_link->st_dev == src_sb_link->st_dev;
 
   if (x->dereference == DEREF_NEVER)
-- 
2.17.0.441.gb46fe60e1d-goog