From: Stefan Sperling <stsp@stsp.name>
Subject: Re: bug/fix: segfault when diffing the root commit
To: Alexander Arkhipov <aa@manpager.net>
Cc: gameoftrees@openbsd.org
Date: Tue, 6 Jun 2023 08:47:03 +0200

On Tue, Jun 06, 2023 at 04:39:33AM +0100, Alexander Arkhipov wrote:
> Alexander Arkhipov <aa@manpager.net> wrote:
> 
> > Hello, gameoftrees@,
> > 
> > I noticed a bug in got, where typing `got -c <root commit> file' causes
> > a segfault. The issue is caused by the NULL pointer tree being accessed
> > in find_entry_by_name() in lib/object.c. I fixed the problem the obvious
> > way by immediately returning NULL if tree is NULL. Now, however, it
> > instead outputs error messages like this:
> > 
> > got: file: no such entry found in tree
> 
> Sorry, just realised my mistake here: the message only was like that
> because the file I was testing with was not in the first commit,
> although I assumed it was. Testing with the patch and a file that *was*
> added with the first commit actually produces the correct result -- a
> diff for that file!
> 
> Without the patch, however, the command always segfaults, regardless of
> the file's existence at the time of the commit.
> 
> Alexander
> 
> 

Thanks Alexander! Below is a patch that adds a test which triggers
the bug and includes a fix for it.

diff 1f8d584437c06db990ac352cce06402c80b1f6a4 7282212bb3ebd56cfdfb205dfd5ff91bdbcab710
commit - 1f8d584437c06db990ac352cce06402c80b1f6a4
commit + 7282212bb3ebd56cfdfb205dfd5ff91bdbcab710
blob - 5198b5ef5702bde4150b817019574f96e0b9f1f3
blob + f448440ff5880b5d6ec7ec9b7d45ef85b47ee2e6
--- lib/diff.c
+++ lib/diff.c
@@ -1098,15 +1098,18 @@ diff_paths(struct got_tree_object *tree1, struct got_t
 			got_object_blob_close(blob2);
 			blob2 = NULL;
 		}
-
-		err = got_object_tree_find_path(&id1, &mode1, repo, tree1,
-		    pe->path);
-		if (err && err->code != GOT_ERR_NO_TREE_ENTRY)
-			goto done;
-		err = got_object_tree_find_path(&id2, &mode2, repo, tree2,
-		    pe->path);
-		if (err && err->code != GOT_ERR_NO_TREE_ENTRY)
-			goto done;
+		if (tree1) {
+			err = got_object_tree_find_path(&id1, &mode1, repo,
+			    tree1, pe->path);
+			if (err && err->code != GOT_ERR_NO_TREE_ENTRY)
+				goto done;
+		}
+		if (tree2) {
+			err = got_object_tree_find_path(&id2, &mode2, repo,
+			    tree2, pe->path);
+			if (err && err->code != GOT_ERR_NO_TREE_ENTRY)
+				goto done;
+		}
 		if (id1 == NULL && id2 == NULL) {
 			err = got_error_path(pe->path, GOT_ERR_NO_TREE_ENTRY);
 			goto done;
blob - 82aab78d37c08ef751e9e73c8de8f1c9f772a4c3
blob + 03e05611a400a2ffc9c91e307d0d5c048fdc6cf3
--- regress/cmdline/diff.sh
+++ regress/cmdline/diff.sh
@@ -1962,6 +1962,66 @@ test_parseargs "$@"
 	test_done "$testroot" "$ret"
 }
 
+test_diff_path_in_root_commit() {
+	local testroot=`test_init diff_path_in_root_commit`
+	local commit_id=`git_show_head $testroot/repo`
+	local alpha_blobid=`get_blob_id $testroot/repo "" alpha`
+
+	got checkout $testroot/repo $testroot/wt > /dev/null
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	(cd $testroot/wt && got diff -c $commit_id alpha > $testroot/stdout)
+
+	cat <<EOF >$testroot/stdout.expected
+diff /dev/null $commit_id
+commit - /dev/null
+commit + $commit_id
+blob - /dev/null
+blob + $alpha_blobid (mode 644)
+--- /dev/null
++++ alpha
+@@ -0,0 +1 @@
++alpha
+EOF
+
+	cmp -s $testroot/stdout.expected $testroot/stdout
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		diff -u $testroot/stdout.expected $testroot/stdout
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	# diff non-existent path
+	(cd $testroot/wt && got diff -c $commit_id nonexistent \
+		> $testroot/stdout 2> $testroot/stderr)
+
+	echo -n > $testroot/stdout.expected
+	cmp -s $testroot/stdout.expected $testroot/stdout
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		diff -u $testroot/stdout.expected $testroot/stdout
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	echo "got: nonexistent: no such entry found in tree" \
+		> $testroot/stderr.expected
+	cmp -s $testroot/stderr.expected $testroot/stderr
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		diff -u $testroot/stderr.expected $testroot/stderr
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	test_done "$testroot" "$ret"
+}
+
 test_parseargs "$@"
 run_test test_diff_basic
 run_test test_diff_shows_conflict
@@ -1980,3 +2040,4 @@ run_test test_diff_dir_to_file
 run_test test_diff_worktree_diffstat
 run_test test_diff_file_to_dir
 run_test test_diff_dir_to_file
+run_test test_diff_path_in_root_commit