From 0729d1e8fd90afc2f19e9b9834533181caf6ddee Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dag-Erling=20Sm=C3=B8rgrav?= <des@FreeBSD.org>
Date: Tue, 9 Apr 2024 00:41:33 +0200
Subject: [PATCH] cp: Never follow symbolic links in destination.

Historically, BSD cp has followed symbolic links in the destination
when copying recursively, while GNU cp has not.  POSIX is somewhat
vague on the topic, but both interpretations are within bounds.  In
33ad990ce974, cp was changed to apply the same logic for symbolic
links in the destination as for symbolic links in the source: follow
if not recursing (which is moot, as this situation can only arise
while recursing) or if the `-L` option was given.  There is no support
for this in POSIX.  We can either switch back, or go all the way.

Having carefully weighed the kind of trouble you can run into by
following unexpected symlinks up against the kind of trouble you can
run into by not following symlinks you expected to follow, we choose
to go all the way.

Note that this means we need to stat the destination twice: once,
following links, to check if it is or references the same file as the
source, and a second time, not following links, to set the dne flag
and determine the destination's type.

While here, remove a needless complication in the dne logic.  We don't
need to explicitly reject overwriting a directory with a non-directory,
because it will fail anyway.

Finally, add test cases for copying a directory to a symlink and
overwriting a directory with a non-directory.

MFC after:	never
Relnotes: 	yes
Sponsored by:	Klara, Inc.
Reviewed by:	kevans
Differential Revision:	https://reviews.freebsd.org/D44578
---
 bin/cp/cp.c             | 52 +++++++++++------------------------------
 bin/cp/tests/cp_test.sh | 31 ++++++++++++++++++++++++
 2 files changed, 44 insertions(+), 39 deletions(-)

diff --git a/bin/cp/cp.c b/bin/cp/cp.c
index 02b879006a4f..cee412e57264 100644
--- a/bin/cp/cp.c
+++ b/bin/cp/cp.c
@@ -260,22 +260,6 @@ main(int argc, char *argv[])
 	    &to_stat)));
 }
 
-/* Does the right thing based on -R + -H/-L/-P */
-static int
-copy_stat(const char *path, struct stat *sb)
-{
-
-	/*
-	 * For -R -H/-P, we need to lstat() instead; copy() cares about the link
-	 * itself rather than the target if we're not following links during the
-	 * traversal.
-	 */
-	if (!Rflag || Lflag)
-		return (stat(path, sb));
-	return (lstat(path, sb));
-}
-
-
 static int
 copy(char *argv[], enum op type, int fts_options, struct stat *root_stat)
 {
@@ -403,7 +387,6 @@ copy(char *argv[], enum op type, int fts_options, struct stat *root_stat)
 					continue;
 				}
 
-
 				if (asprintf(&recurse_path, "%s/%s", to.p_path,
 				    rootname) == -1)
 					err(1, "asprintf");
@@ -452,30 +435,21 @@ copy(char *argv[], enum op type, int fts_options, struct stat *root_stat)
 			continue;
 		}
 
-		/* Not an error but need to remember it happened. */
-		if (copy_stat(to.p_path, &to_stat) == -1)
-			dne = 1;
-		else {
-			if (to_stat.st_dev == curr->fts_statp->st_dev &&
-			    to_stat.st_ino == curr->fts_statp->st_ino) {
-				warnx("%s and %s are identical (not copied).",
-				    to.p_path, curr->fts_path);
-				badcp = rval = 1;
-				if (S_ISDIR(curr->fts_statp->st_mode))
-					(void)fts_set(ftsp, curr, FTS_SKIP);
-				continue;
-			}
-			if (!S_ISDIR(curr->fts_statp->st_mode) &&
-			    S_ISDIR(to_stat.st_mode)) {
-				warnx("cannot overwrite directory %s with "
-				    "non-directory %s",
-				    to.p_path, curr->fts_path);
-				badcp = rval = 1;
-				continue;
-			}
-			dne = 0;
+		/* Check if source and destination are identical. */
+		if (stat(to.p_path, &to_stat) == 0 &&
+		    to_stat.st_dev == curr->fts_statp->st_dev &&
+		    to_stat.st_ino == curr->fts_statp->st_ino) {
+			warnx("%s and %s are identical (not copied).",
+			    to.p_path, curr->fts_path);
+			badcp = rval = 1;
+			if (S_ISDIR(curr->fts_statp->st_mode))
+				(void)fts_set(ftsp, curr, FTS_SKIP);
+			continue;
 		}
 
+		/* Not an error but need to remember it happened. */
+		dne = lstat(to.p_path, &to_stat) != 0;
+
 		switch (curr->fts_statp->st_mode & S_IFMT) {
 		case S_IFLNK:
 			/* Catch special case of a non-dangling symlink. */
diff --git a/bin/cp/tests/cp_test.sh b/bin/cp/tests/cp_test.sh
index 397c06d75bbb..5c581e06ab8e 100755
--- a/bin/cp/tests/cp_test.sh
+++ b/bin/cp/tests/cp_test.sh
@@ -363,6 +363,35 @@ symlink_exists_force_body()
 	atf_check -o inline:"foo\n" readlink bar
 }
 
+atf_test_case directory_to_symlink
+directory_to_symlink_body()
+{
+	mkdir -p foo
+	ln -s .. foo/bar
+	mkdir bar
+	touch bar/baz
+	atf_check -s not-exit:0 -e match:"Not a directory" \
+	    cp -R bar foo
+	atf_check -s not-exit:0 -e match:"Not a directory" \
+	    cp -r bar foo
+}
+
+atf_test_case overwrite_directory
+overwrite_directory_body()
+{
+	mkdir -p foo/bar/baz
+	touch bar
+	atf_check -s not-exit:0 -e match:"Is a directory" \
+	    cp bar foo
+	rm bar
+	mkdir bar
+	touch bar/baz
+	atf_check -s not-exit:0 -e match:"Is a directory" \
+	    cp -R bar foo
+	atf_check -s not-exit:0 -e match:"Is a directory" \
+	    cp -r bar foo
+}
+
 atf_init_test_cases()
 {
 	atf_add_test_case basic
@@ -388,4 +417,6 @@ atf_init_test_cases()
 	atf_add_test_case symlink
 	atf_add_test_case symlink_exists
 	atf_add_test_case symlink_exists_force
+	atf_add_test_case directory_to_symlink
+	atf_add_test_case overwrite_directory
 }