{"thread":{"id":"64694","subject":"[PATCH] diff: avoid segfault with freed entries","startedAt":"2025-12-29T21:45:00Z","lastAt":"2025-12-30T16:11:30Z","messageCount":3,"participants":["Derrick Stolee via GitGitGadget","Junio C Hamano","Kristoffer Haugsbakk"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"532815","messageId":"pull.2027.git.1767044697712.gitgitgadget@gmail.com","threadId":"64694","inReplyTo":null,"subject":"[PATCH] diff: avoid segfault with freed entries","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-12-29T21:44:57Z","receivedAt":"2025-12-29T21:45:00Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nWhen computing a diff in a partial clone, there is a chance that we\ncould trigger a prefetch of missing objects at the same time as we are\nfreeing entries from the global diff queue. This is difficult to\nreproduce, as we need to have some objects be freed from the queue\nbefore triggering the prefetch of missing objects. There is a new test\nin t4067 that does trigger the segmentation fault that results in this\ncase.\n\nThe fix is to set the queue pointer to NULL after it is freed, and then\nto be careful about NULL values in the prefetch.\n\nThe more elaborate explanation is that within diffcore_std(), we may\nskip the initial prefetch due to the output format (--name-only in the\ntest) and go straight to diffcore_skip_stat_unmatch(). In that method,\nthe index entries that have been invalidated by path changes show up as\nentries but may be deleted because they are not actually content diffs\nand only newer timestamps than expected. As those entries are deleted,\nlater entries are checked with diff_filespec_check_stat_unmatch(), which\nuses diff_queued_diff_prefetch() as the missing_object_cb in its diff\noptions. That can trigger downloading missing objects if the appropriate\nscenario occurs to trigger a call to diff_popoulate_filespec(). It's\nfinally within that callback to diff_queued_diff_prefetch() that the\nsegfault occurs.\n\nThe test was hard to find because it required some real differences,\nsome not-different files that had a newer modified time, and the order\nof those files alphabetically was important to trigger the deletion\nbefore the prefetch was triggered.\n\nI briefly considered a \"lock\" member for the diff queue, but it was a\nmuch larger diff and introduced many more possible error scenarios.\n\nSigned-off-by: Derrick Stolee \n---\n diff: avoid segfault with freed entries\n \n I found this segfault in the wild in a pipeline that was using the\n microsoft/git fork, but the error can be reproduced without that fork.\n It requires partial clone, though.\n \n Thanks, -Stolee\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-2027%2Fderrickstolee%2Fdiff-segfault-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2027/derrickstolee/diff-segfault-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/2027\n\n diff.c | 5 +++++\n t/t4067-diff-partial-clone.sh | 35 +++++++++++++++++++++++++++++++++++\n 2 files changed, 40 insertions(+)\n\ndiff --git a/diff.c b/diff.c\nindex a1961526c0..72661d635c 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -7046,6 +7046,7 @@ static void diffcore_skip_stat_unmatch(struct diff_options *diffopt)\n \t\t\tif (!diffopt->flags.no_index)\n \t\t\t\tdiffopt->skip_stat_unmatch++;\n \t\t\tdiff_free_filepair(p);\n+\t\t\tq->queue[i] = NULL;\n \t\t}\n \t}\n \tfree(q->queue);\n@@ -7089,6 +7090,10 @@ void diff_queued_diff_prefetch(void *repository)\n \n \tfor (i = 0; i < q->nr; i++) {\n \t\tstruct diff_filepair *p = q->queue[i];\n+\n+\t\tif (!p)\n+\t\t\tcontinue;\n+\n \t\tdiff_add_if_missing(repo, &to_fetch, p->one);\n \t\tdiff_add_if_missing(repo, &to_fetch, p->two);\n \t}\ndiff --git a/t/t4067-diff-partial-clone.sh b/t/t4067-diff-partial-clone.sh\nindex 581250dd2d..72f25de449 100755\n--- a/t/t4067-diff-partial-clone.sh\n+++ b/t/t4067-diff-partial-clone.sh\n@@ -132,6 +132,41 @@ test_expect_success 'diff with rename detection batches blobs' '\n \ttest_line_count = 1 done_lines\n '\n \n+test_expect_success 'diff succeeds even if entries are removed from queue' '\n+\ttest_when_finished \"rm -rf server client trace\" &&\n+\n+\ttest_create_repo server &&\n+\tfor l in a c e g i p\n+\tdo\n+\t\techo $l >server/$l &&\n+\t\tgit -C server add $l || return 1\n+\tdone &&\n+\tgit -C server commit -m x &&\n+\n+\tfor l in a e i\n+\tdo\n+\t\tgit -C server rm $l || return 1\n+\tdone &&\n+\n+\tfor l in b d f i\n+\t\tdo\n+\t\techo $l$l >server/$l &&\n+\t\tgit -C server add $l || return 1\n+\tdone &&\n+\tgit -C server commit -a -m x &&\n+\n+\ttest_config -C server uploadpack.allowfilter 1 &&\n+\ttest_config -C server uploadpack.allowanysha1inwant 1 &&\n+\tgit clone --filter=blob:limit=0 \"file://$(pwd)/server\" client &&\n+\n+\tfor file in $(ls client)\n+\tdo\n+\t\tcat client/$file >$file &&\n+\t\tmv $file client/$file || return 1\n+\tdone &&\n+\tgit -C client diff --name-only --relative HEAD^\n+'\n+\n test_expect_success 'diff does not fetch anything if inexact rename detection is not needed' '\n \ttest_when_finished \"rm -rf server client trace\" &&\n \n\nbase-commit: 9a2fb147f2c61d0cab52c883e7e26f5b7948e3ed\n-- \ngitgitgadget\n"},{"id":"532820","messageId":"xmqqtsx8h9dl.fsf@gitster.g","threadId":"64694","inReplyTo":"pull.2027.git.1767044697712.gitgitgadget@gmail.com","subject":"Re: [PATCH] diff: avoid segfault with freed entries","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-12-30T04:38:30Z","receivedAt":"2025-12-30T04:38:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> The more elaborate explanation is that within diffcore_std(), we may\n> skip the initial prefetch due to the output format (--name-only in the\n> test) and go straight to diffcore_skip_stat_unmatch().\n\nThat's very interesting. We have code to fetch on-demand when it\nturns out that the initial prefetch shouldn't have been skipped and\nwe need contents, so in that sense, the condition to skip the\ninitial prefetch does not have to be precise, but we may want to see\nif we can have a single helper function that exactly tells us if we\nneed to look at the contents or not. I think we had a few changes\nthat made the definition of \"diff status is based on contents\" in\nthe past few releases, not for the purpose of this prefetch skipping\nbut to set the exit status.\n"},{"id":"532849","messageId":"66032fff-654f-4c46-a7d4-e80d594f2df3@app.fastmail.com","threadId":"64694","inReplyTo":"pull.2027.git.1767044697712.gitgitgadget@gmail.com","subject":"Re: [PATCH] diff: avoid segfault with freed entries","fromName":"Kristoffer Haugsbakk","fromEmail":"kristofferhaugsbakk@fastmail.com","sentAt":"2025-12-30T16:11:09Z","receivedAt":"2025-12-30T16:11:30Z","isPatch":true,"sender":{"key":"kristofferhaugsbakk@fastmail.com","avatar":null},"body":"On Mon, Dec 29, 2025, at 22:44, Derrick Stolee via GitGitGadget wrote:\n> From: Derrick Stolee \n>\n>[snip]\n> The more elaborate explanation is that within diffcore_std(), we may\n> skip the initial prefetch due to the output format (--name-only in the\n> test) and go straight to diffcore_skip_stat_unmatch(). In that method,\n> the index entries that have been invalidated by path changes show up as\n> entries but may be deleted because they are not actually content diffs\n> and only newer timestamps than expected. As those entries are deleted,\n> later entries are checked with diff_filespec_check_stat_unmatch(), which\n> uses diff_queued_diff_prefetch() as the missing_object_cb in its diff\n> options. That can trigger downloading missing objects if the appropriate\n> scenario occurs to trigger a call to diff_popoulate_filespec(). It's\n\ns/diff_popoulate_filespec/diff_populate_filespec/\n\n> finally within that callback to diff_queued_diff_prefetch() that the\n> segfault occurs.\n>\n>[snip]\n"}]}