From 88efab5c3b7b7585a3b4f45db89c108fef9a04f2 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 15 Jul 2026 02:05:23 -0400 Subject: [PATCH 1/2] diff: ignore unmerged paths outside prefix with --relative --cached 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 Signed-off-by: Junio C Hamano --- 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 From 447126ed7df66b396235766547a57649f925aac2 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 28 Jul 2026 11:14:58 -0400 Subject: [PATCH 2/2] diff-lib: add idx/tree sanity check to oneway_diff When looking just at the code in oneway_diff(), it seems possible for both "idx" and "tree" to be NULL, in which case we'd potentially segfault while checking the relative prefix. But if you consider what these items actually mean, it shouldn't be possible for both to be NULL. Let's add an assertion and a comment documenting this. It might help human readers, but should also silence static analyzers like Coverity which complain about the potential segfault. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- diff-lib.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/diff-lib.c b/diff-lib.c index a23119b852..0e868b28b6 100644 --- a/diff-lib.c +++ b/diff-lib.c @@ -530,6 +530,16 @@ static int oneway_diff(const struct cache_entry * const *src, if (tree == o->df_conflict_entry) tree = NULL; + /* + * We should only see a NULL idx when the entry was present in the tree + * but deleted in the idx. In which case it should be impossible + * that a NULL tree was passed in (there would have been no entry at + * all) or that we got a df conflict above (you need a directory and a + * file to get such a conflict, which implies both sides are present). + */ + if (!idx && !tree) + BUG("oneway_diff with neither idx nor tree"); + if (ce_path_match(revs->diffopt.repo->index, idx ? idx : tree, &revs->prune_data, NULL)) {