From: Mark Jamsek <mark@jamsek.com>
Subject: Re: bug/fix: segfault when diffing the root commit
To: Alexander Arkhipov <aa@manpager.net>, gameoftrees@openbsd.org
Date: Tue, 6 Jun 2023 17:24:27 +1000

On 23-06-06 08:47AM, Stefan Sperling wrote:
> 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.

ok

> 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
> 
> 

-- 
Mark Jamsek <fnc.bsdbox.org|got.bsdbox.org>
GPG: F2FF 13DE 6A06 C471 CA80  E6E2 2930 DC66 86EE CF68