[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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

OK, so if diff_unmerge() gives a NULL for a path outside out area of
interest, we of course do not fill the filepair and return.  We
won't do any further processing, like showing the entry or recursing
into it, and diff_queued_diff hasn't been told about this path (as
diff_unmerge() returns NULL before queuing the pair), so it is the
end of story for this path here.

Looks good to me.

> 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