cp: Don't rely on FTS_DP to keep track of depth.

In normal operation, we get an FTS_D entry when we enter a directory
and a matching FTS_DP entry when we leave it.  However, if an error
occurs either changing to or reading a directory, we may get an FTS_D
entry followed by FTS_DNR or even FTS_ERR instead.  Since FTS_ERR can
also occur for non-directory entries, the only reliable way to keep
track of when we leave a directory is to compare fts_level to our own
depth counter.

This fixes a rare assertion when attempting to recursively copy a
directory tree containing a directory which is either not readable or
not searchable.

While here, also add a test case for directory loops.

Fixes:		82fc0d09e8
Sponsored by:	Klara, Inc.
Reviewed by:	kevans
Differential Revision:	https://reviews.freebsd.org/D51096

bin/cp/cp.c

@@ -270,10 +270,9 @@ copy(char *argv[], enum op type, int fts_options, struct stat *root_stat)
 	FTS *ftsp;
 	FTSENT *curr;
 	char *recpath = NULL, *sep;
-	int atflags, dne, badcp, len, rval;
+	int atflags, dne, badcp, len, level, rval;
 	mode_t mask, mode;
 	bool beneath = Rflag && type != FILE_TO_FILE;
-	bool skipdp = false;
 
 	/*
 	 * Keep an inverted copy of the umask, for use in correcting
@@ -305,6 +304,7 @@ copy(char *argv[], enum op type, int fts_options, struct stat *root_stat)
 		to.dir = -1;
 	}
 
+	level = FTS_ROOTLEVEL;
 	if ((ftsp = fts_open(argv, fts_options, NULL)) == NULL)
 		err(1, "fts_open");
 	for (badcp = rval = 0;
@@ -315,6 +315,20 @@ copy(char *argv[], enum op type, int fts_options, struct stat *root_stat)
 		case FTS_NS:
 		case FTS_DNR:
 		case FTS_ERR:
+			if (level > curr->fts_level) {
+				/* leaving a directory; remove its name from to.path */
+				if (type == DIR_TO_DNE &&
+				    curr->fts_level == FTS_ROOTLEVEL) {
+					/* this is actually our created root */
+				} else {
+					while (to.end > to.path && *to.end != '/')
+						to.end--;
+					assert(strcmp(to.end + (*to.end == '/'),
+					    curr->fts_name) == 0);
+					*to.end = '\0';
+				}
+				level--;
+			}
 			warnc(curr->fts_errno, "%s", curr->fts_path);
 			badcp = rval = 1;
 			continue;
@@ -335,14 +349,6 @@ copy(char *argv[], enum op type, int fts_options, struct stat *root_stat)
 				strlcpy(rootname, curr->fts_name,
 				    sizeof(rootname));
 			}
-			/*
-			 * If we FTS_SKIP while handling FTS_D, we will
-			 * immediately get FTS_DP for the same directory.
-			 * If this happens before we've appended the name
-			 * to to.path, we need to remember not to perform
-			 * the reverse operation.
-			 */
-			skipdp = true;
 			/* we must have a destination! */
 			if (type == DIR_TO_DNE &&
 			    curr->fts_level == FTS_ROOTLEVEL) {
@@ -410,7 +416,7 @@ copy(char *argv[], enum op type, int fts_options, struct stat *root_stat)
 				}
 				to.end += len;
 			}
-			skipdp = false;
+			level++;
 			/*
 			 * We're on the verge of recursing on ourselves.
 			 * Either we need to stop right here (we knowingly
@@ -477,18 +483,19 @@ copy(char *argv[], enum op type, int fts_options, struct stat *root_stat)
 					rval = 1;
 				}
 			}
-			/* are we leaving a directory we failed to enter? */
-			if (skipdp)
-				continue;
-			/* leaving a directory; remove its name from to.path */
-			if (type == DIR_TO_DNE &&
-			    curr->fts_level == FTS_ROOTLEVEL) {
-				/* this is actually our created root */
-			} else {
-				while (to.end > to.path && *to.end != '/')
-					to.end--;
-				assert(strcmp(to.end + (*to.end == '/'), curr->fts_name) == 0);
-				*to.end = '\0';
+			if (level > curr->fts_level) {
+				/* leaving a directory; remove its name from to.path */
+				if (type == DIR_TO_DNE &&
+				    curr->fts_level == FTS_ROOTLEVEL) {
+					/* this is actually our created root */
+				} else {
+					while (to.end > to.path && *to.end != '/')
+						to.end--;
+					assert(strcmp(to.end + (*to.end == '/'),
+					    curr->fts_name) == 0);
+					*to.end = '\0';
+				}
+				level--;
 			}
 			continue;
 		default:
@@ -638,6 +645,7 @@ copy(char *argv[], enum op type, int fts_options, struct stat *root_stat)
 		if (vflag && !badcp)
 			(void)printf("%s -> %s%s\n", curr->fts_path, to.base, to.path);
 	}
+	assert(level == FTS_ROOTLEVEL);
 	if (errno)
 		err(1, "fts_read");
 	(void)fts_close(ftsp);

bin/cp/tests/cp_test.sh

@@ -618,6 +618,76 @@ to_root_cleanup()
 	(dst=$(cat dst) && rm "/$dst") 2>/dev/null || true
 }
 
+atf_test_case dirloop
+dirloop_head()
+{
+	atf_set "descr" "Test cycle detection when recursing"
+}
+dirloop_body()
+{
+	mkdir -p src/a src/b
+	ln -s ../b src/a
+	ln -s ../a src/b
+	atf_check \
+	    -s exit:1 \
+	    -e match:"src/a/b/a: directory causes a cycle" \
+	    -e match:"src/b/a/b: directory causes a cycle" \
+	    cp -r src dst
+	atf_check test -d dst
+	atf_check test -d dst/a
+	atf_check test -d dst/b
+	atf_check test -d dst/a/b
+	atf_check test ! -e dst/a/b/a
+	atf_check test -d dst/b/a
+	atf_check test ! -e dst/b/a/b
+}
+
+atf_test_case unrdir
+unrdir_head()
+{
+	atf_set "descr" "Test handling of unreadable directories"
+}
+unrdir_body()
+{
+	for d in a b c ; do
+		mkdir -p src/$d
+		echo "$d" >src/$d/f
+	done
+	chmod 0 src/b
+	atf_check \
+	    -s exit:1 \
+	    -e match:"^cp: src/b: Permission denied" \
+	    cp -R src dst
+	atf_check test -d dst/a
+	atf_check cmp src/a/f dst/a/f
+	atf_check test -d dst/b
+	atf_check test ! -e dst/b/f
+	atf_check test -d dst/c
+	atf_check cmp src/c/f dst/c/f
+}
+
+atf_test_case unrfile
+unrdir_head()
+{
+	atf_set "descr" "Test handling of unreadable files"
+}
+unrfile_body()
+{
+	mkdir src
+	for d in a b c ; do
+		echo "$d" >src/$d
+	done
+	chmod 0 src/b
+	atf_check \
+	    -s exit:1 \
+	    -e match:"^cp: src/b: Permission denied" \
+	    cp -R src dst
+	atf_check test -d dst
+	atf_check cmp src/a dst/a
+	atf_check test ! -e dst/b
+	atf_check cmp src/c dst/c
+}
+
 atf_init_test_cases()
 {
 	atf_add_test_case basic
@@ -656,4 +726,7 @@ atf_init_test_cases()
 	atf_add_test_case to_link_outside
 	atf_add_test_case dstmode
 	atf_add_test_case to_root
+	atf_add_test_case dirloop
+	atf_add_test_case unrdir
+	atf_add_test_case unrfile
 }