[PATCH] diff: ignore unmerged paths outside prefix with --relative --cached

[PATCH] diff: ignore unmerged paths outside prefix with --relative --cached

[Jeff King]

A diff using --relative ignores entries outside the current directory.
This results in a segfault when we try to process an unmerged entry
that's outside of our prefix, since we end up with a NULL diff_filepair
and use it without checking that it's valid.

I think this bug goes back to 76399c0195 (diff.c: return filepair from
diff_unmerge(), 2011-04-22). Prior to that, diff_unmerge() knew to skip
entries outside of our prefix, due to cd676a5136 (diff --relative:
output paths as relative to the current subdirectory, 2008-02-12). Back
then the caller didn't care that we hadn't added anything to the queue.
In 76399c0195 that changed; we now returned the pair (or NULL), and the
caller in do_oneway_diff() was then called fill_filespec() itself. And
it does so without checking for NULL, causing a segfault.

The obvious fix is to skip the fill_filespec() call (after which we just
return), which this patch does.

There's another call to diff_unmerge() in run_diff_files(). That case
was already fixed by 8174627b3d (diff-lib: ignore paths that are outside
$cwd if --relative asked, 2021-08-22), but of course it didn't help us
for --cached.

That commit also claims that checking the result of diff_unmerge() is
not enough, as we'd want other code paths to skip the entry, too (even
if they wouldn't segfault). But as far as I can tell, that is not true
for --cached. We eventually end up in diff_queue_addremove() or in
diff_queue_change(), both of which know to return early when we're
outside of the prefix.

Arguably we could be checking at the top of oneway_diff() whether the
path is interesting at all. That would not only avoid this code path
entirely, but would also possibly save a small amount of work. But since
everything else appears to work OK, I went for the smallest fix here to
avoid any regression.

Specifically, a comment in oneway_diff() claims we're supposed to
advance o->pos, which we might fail to do if we return early. Though
that "advance" seems to have gone away in da165f470e (unpack-trees.c:
prepare for looking ahead in the index, 2010-01-07), so it is possible
the comment is simply out of date.  We can explore that separately;
checking for a NULL return from diff_unmerge() seems like a sensible
thing to do regardless.

We can piggy-back on the tests added by 8174627b3d; we're just checking
the --cached variant.

Signed-off-by: Jeff King <peff@peff.net>
---
+cc Junio, as you may have some wisdom on that further exploration.

 diff-lib.c               | 2 +-
 t/t4045-diff-relative.sh | 9 +++++++++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/diff-lib.c b/diff-lib.c
index ae91027a02..a23119b852 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -467,7 +467,7 @@ static void do_oneway_diff(struct unpack_trees_options *o,
 	if (cached && idx && ce_stage(idx)) {
 		struct diff_filepair *pair;
 		pair = diff_unmerge(&revs->diffopt, idx->name);
-		if (tree)
+		if (pair && tree)
 			fill_filespec(pair->one, &tree->oid, 1,
 				      tree->ce_mode);
 		return;
diff --git a/t/t4045-diff-relative.sh b/t/t4045-diff-relative.sh
index 2c8493fe66..167be0bdcc 100755
--- a/t/t4045-diff-relative.sh
+++ b/t/t4045-diff-relative.sh
@@ -245,4 +245,13 @@ test_expect_failure 'diff --relative with change in subdir' '
 	test_cmp expected out
 '
 
+test_expect_success 'diff --relative --cached with change in subdir' '
+	git switch br3 &&
+	test_when_finished "git merge --abort" &&
+	test_must_fail git merge sub1 &&
+	echo file0 >expected &&
+	git -C subdir diff --relative --name-only --cached >out &&
+	test_cmp expected out
+'
+
 test_done
-- 
2.55.0.612.g68473ef936

Re: [PATCH] diff: ignore unmerged paths outside prefix with --relative --cached

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> A diff using --relative ignores entries outside the current directory.
> This results in a segfault when we try to process an unmerged entry
> that's outside of our prefix, since we end up with a NULL diff_filepair
> and use it without checking that it's valid.
> ...
> +cc Junio, as you may have some wisdom on that further exploration.

Will take a look at the history myself, but I would probably not
have much wisdom on a change from 2011.  I often do not even
remember what I ate for breakfast yesterday ;-).

>  diff-lib.c               | 2 +-
>  t/t4045-diff-relative.sh | 9 +++++++++
>  2 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/diff-lib.c b/diff-lib.c
> index ae91027a02..a23119b852 100644
> --- a/diff-lib.c
> +++ b/diff-lib.c
> @@ -467,7 +467,7 @@ static void do_oneway_diff(struct unpack_trees_options *o,
>  	if (cached && idx && ce_stage(idx)) {
>  		struct diff_filepair *pair;
>  		pair = diff_unmerge(&revs->diffopt, idx->name);
> -		if (tree)
> +		if (pair && tree)
>  			fill_filespec(pair->one, &tree->oid, 1,
>  				      tree->ce_mode);
>  		return;
> diff --git a/t/t4045-diff-relative.sh b/t/t4045-diff-relative.sh
> index 2c8493fe66..167be0bdcc 100755
> --- a/t/t4045-diff-relative.sh
> +++ b/t/t4045-diff-relative.sh
> @@ -245,4 +245,13 @@ test_expect_failure 'diff --relative with change in subdir' '
>  	test_cmp expected out
>  '
>  
> +test_expect_success 'diff --relative --cached with change in subdir' '
> +	git switch br3 &&
> +	test_when_finished "git merge --abort" &&
> +	test_must_fail git merge sub1 &&
> +	echo file0 >expected &&
> +	git -C subdir diff --relative --name-only --cached >out &&
> +	test_cmp expected out
> +'
> +
>  test_done