Subject: Re: bug#31335: Fwd: bug#31335: unexpected cp -f behavior

From: Pádraig Brady <[email-masked]>
Date: Wed, 16 May 2018 00:03:00 -0700

On 15/05/18 17:11, Paul Eggert wrote:
> On 05/15/2018 10:05 AM, Pádraig Brady wrote:
>>> It's also what I would expect, if I use -f, I expect cp will do everything
>>> it can to force the operation and succeed if all possible.
>> Maybe, though that's worth further consideration.
> 
> POSIX is arguably ambiguous about what 'cp -f S D' should do if D is a 
> symlink to itself. POSIX says that if D "exists", then cp must try to 
> unlink and then re-create D; and that if D does not "exist", cp must 
> fail. So, if one considers a self-symlink as "existing", then GNU cp 
> doesn't conform to POSIX; if one considers such a symlink as not 
> "existing", then GNU cp conforms. Unfortunately, as far as I can tell 
> POSIX never exactly defines what "exists" means in this context.

Well `test -e self-symlink` => does not exist

> That being said, POSIX uses nearly the same wording for 'ln -f' that it 
> does for 'cp -f', which implies that cp should be consistent with ln, 
> and GNU ln (like most ln implementations) treat self-symlinks as 
> "existing" in this case. Also, other implementations of cp seem act like 
> ln does here, so they interpret the ambiguity in POSIX the opposite way 
> that GNU cp does. Furthermore, I think that users by and large expect 
> the non-GNU interpretation where 'cp -f' is like 'ln -f'. For all these 
> reasons, I'm inclined to think that GNU cp should fall into line here.

Agreed. I can't think of a case where replacing a self symlink
is not what you'd want to happen.

Proposed patch is attached.

cheers,
Pádraig
From f3cf3716c70e4680f9b3ed2180c38de632c06010 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= <[email-masked]>
Date: Tue, 15 May 2018 23:41:36 -0700
Subject: [PATCH] cp: with --force; replace self referential symlinks

* src/copy.c (copy_internal): Don't fail immediately upon
getting ELOOP when running stat() on the destination,
rather proceeding if -f specified, allowing the link
to be removed.  If the loop is not in the final component
of the destination path, we still fail but at the
subsequent unlink() stage.
* doc/coreutils.texi (cp invocation): Adjust wording to say
that --force doesn't work with dangling links, rather than
all links that can't be traversed.
* tests/cp/thru-dangling.sh: Add a test case.
* NEWS: Mention the change in behavior.
Discussed in https://bugs.gnu.org/31335
---
 NEWS                      |  4 ++++
 doc/coreutils.texi        |  2 +-
 src/copy.c                |  7 +++++--
 tests/cp/thru-dangling.sh | 13 +++++++++++--
 4 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/NEWS b/NEWS
index f981596..4c63b6d 100644
--- a/NEWS
+++ b/NEWS
@@ -39,6 +39,10 @@ GNU coreutils NEWS                                    -*- outline -*-
   now silently does nothing if A exists.
   [bug introduced with coreutils-7.1]
 
+** Changes in behavior
+
+  'cp --force file symlink' now removes the symlink even if
+  it is self referential.
 
 ** Improvements
 
diff --git a/doc/coreutils.texi b/doc/coreutils.texi
index c8d9bd9..c28b8d0 100644
--- a/doc/coreutils.texi
+++ b/doc/coreutils.texi
@@ -8535,7 +8535,7 @@ When copying without this option and an existing destination file cannot
 be opened for writing, the copy fails.  However, with @option{--force},
 when a destination file cannot be opened, @command{cp} then
 tries to recreate the file by first removing it.  Note @option{--force}
-alone will not remove symlinks that can't be traversed.
+alone will not remove dangling symlinks.
 When this option is combined with
 @option{--link} (@option{-l}) or @option{--symbolic-link}
 (@option{-s}), the destination link is replaced, and unless
diff --git a/src/copy.c b/src/copy.c
index 0407c56..f4c92d7 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -1949,12 +1949,15 @@ copy_internal (char const *src_name, char const *dst_name,
             }
           else
             {
-              if (errno != ENOENT)
+              if (errno == ELOOP && x->unlink_dest_after_failed_open)
+                /* leave new_dst=false so we unlink later.  */;
+              else if (errno != ENOENT)
                 {
                   error (0, errno, _("cannot stat %s"), quoteaf (dst_name));
                   return false;
                 }
-              new_dst = true;
+              else
+                new_dst = true;
             }
         }
 
diff --git a/tests/cp/thru-dangling.sh b/tests/cp/thru-dangling.sh
index 8fd452e..e433525 100755
--- a/tests/cp/thru-dangling.sh
+++ b/tests/cp/thru-dangling.sh
@@ -28,15 +28,24 @@ echo "cp: not writing through dangling symlink 'dangle'" \
 
 # Starting with 6.9.90, this usage fails, by default:
 for opt in '' '-f'; do
-  cp $opt f dangle > err 2>&1 && fail=1
+  returns_ 1 cp $opt f dangle > err 2>&1 || fail=1
   compare exp-err err || fail=1
   test -f no-such && fail=1
 done
 
+
 # But you can set POSIXLY_CORRECT to get the historical behavior.
 env POSIXLY_CORRECT=1 cp f dangle > out 2>&1 || fail=1
 cat no-such >> out || fail=1
-
 compare exp out || fail=1
 
+
+# Starting with 8.30 we treat ELOOP as existing and so
+# remove the symlink
+ln -s loop loop || framework_failure_
+cp -f f loop > err 2>&1 || fail=1
+compare /dev/null err || fail=1
+compare exp loop || fail=1
+test -f loop || fail=1
+
 Exit $fail
-- 
2.9.3