From: Stefan Sperling <stsp@stsp.name>
Subject: fix bogus 'permission denied' error during updates
To: gameoftrees@openbsd.org
Date: Sat, 19 Jun 2021 18:16:53 +0200

I just ran into this:

$ got up -b iwx-txagg
Switching work tree from refs/heads/iwmfw to refs/heads/iwx-txagg
D  .gitignore
got: rmdir: /usr/src: Permission denied

When 'got update' deletes a file in the root directory of the work tree
and the parent directory of this work tree isn't writable then the call
to rmdir() results in EPERM.

The code which calls rmdir() only detects ENOTEMPTY and passes any other
error through. This is fine for paths within the work tree but we should
not attempt to delete the work tree root itself.

With the patch below we do not attempt to delete the work tree root anymore.
I have extended a corresponding test case to check for this bug.
This test now fails like above if lib/worktree.c is not patched.

ok?

diff 779e1159b25b2aa115e6b42f51003b7e2fa7c06b /home/stsp/src/got
blob - 77e9ecb628d437b68e991e2de6d272b5053ad9e0
file + lib/worktree.c
--- lib/worktree.c
+++ lib/worktree.c
@@ -2104,7 +2104,7 @@ static const struct got_error *
 remove_ondisk_file(const char *root_path, const char *path)
 {
 	const struct got_error *err = NULL;
-	char *ondisk_path = NULL;
+	char *ondisk_path = NULL, *parent = NULL;
 
 	if (asprintf(&ondisk_path, "%s/%s", root_path, path) == -1)
 		return got_error_from_errno("asprintf");
@@ -2114,23 +2114,28 @@ remove_ondisk_file(const char *root_path, const char *
 			err = got_error_from_errno2("unlink", ondisk_path);
 	} else {
 		size_t root_len = strlen(root_path);
-		do {
-			char *parent;
-			err = got_path_dirname(&parent, ondisk_path);
-			if (err)
-				break;
+		err = got_path_dirname(&parent, ondisk_path);
+		if (err)
+			goto done;
+		while (got_path_cmp(parent, root_path,
+		    strlen(parent), root_len) != 0) {
 			free(ondisk_path);
 			ondisk_path = parent;
+			parent = NULL;
 			if (rmdir(ondisk_path) == -1) {
 				if (errno != ENOTEMPTY)
 					err = got_error_from_errno2("rmdir",
 					    ondisk_path);
 				break;
 			}
-		} while (got_path_cmp(ondisk_path, root_path,
-		    strlen(ondisk_path), root_len) != 0);
+			err = got_path_dirname(&parent, ondisk_path);
+			if (err)
+				break;
+		}
 	}
+done:
 	free(ondisk_path);
+	free(parent);
 	return err;
 }
 
blob - c90dd13dc4e8de96ade487d14ace7bf99feecb82
file + regress/cmdline/update.sh
--- regress/cmdline/update.sh
+++ regress/cmdline/update.sh
@@ -98,7 +98,8 @@ test_update_adds_file() {
 test_update_deletes_file() {
 	local testroot=`test_init update_deletes_file`
 
-	got checkout $testroot/repo $testroot/wt > /dev/null
+	mkdir $testroot/wtparent
+	got checkout $testroot/repo $testroot/wtparent/wt > /dev/null
 	ret="$?"
 	if [ "$ret" != "0" ]; then
 		test_done "$testroot" "$ret"
@@ -113,7 +114,11 @@ test_update_deletes_file() {
 	git_show_head $testroot/repo >> $testroot/stdout.expected
 	echo >> $testroot/stdout.expected
 
-	(cd $testroot/wt && got update > $testroot/stdout)
+	# verify that no error occurs if the work tree's parent
+	# directory is not writable
+	chmod u-w $testroot/wtparent
+	(cd $testroot/wtparent/wt && got update > $testroot/stdout)
+	chmod u+w $testroot/wtparent
 
 	cmp -s $testroot/stdout.expected $testroot/stdout
 	ret="$?"
@@ -123,7 +128,7 @@ test_update_deletes_file() {
 		return 1
 	fi
 
-	if [ -e $testroot/wt/beta ]; then
+	if [ -e $testroot/wtparent/wt/beta ]; then
 		echo "removed file beta still exists on disk" >&2
 		test_done "$testroot" "1"
 		return 1