{"thread":{"id":"64976","subject":"[PATCH 0/1] diffcore-break: prevent dangling pointer","startedAt":"2026-02-11T04:11:36Z","lastAt":"2026-02-24T15:22:27Z","messageCount":11,"participants":["Han Young","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"535732","messageId":"20260211041128.48412-1-hanyang.tony@bytedance.com","threadId":"64976","inReplyTo":null,"subject":"[PATCH 0/1] diffcore-break: prevent dangling pointer","fromName":"Han Young","fromEmail":"hanyang.tony@bytedance.com","sentAt":"2026-02-11T04:11:27Z","receivedAt":"2026-02-11T04:11:36Z","isPatch":true,"sender":{"key":"hanyang.tony@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/108711387?v=4"},"body":"The diffcore_break function in diffcore-break.c forgets to set the queue\nreference to NULL after freeing it. In a blobless cloned repository,\nthe queue could be accessed by prefetch and result in a segmentation fault.\n\nThis bug is only triggered if:\n* the repository is partially cloned\n* the diff operation triggers prefetch\n* a diff is split into delete and create before prefetching\n\nI've prepared a example repository that triggers this bug.\n\n git clone git@github.com:hanyang-tony/dangle_sanitize.git --filter=blob:none\n cd dangle_sanitize\n\n # download the old version of the file\n # to ensure the splited diff exists in local repository\n git checkout HEAD~1 .iac\n\n # reset the file so we have a diff \n git reset --hard HEAD\n\n # segmentation fault\n git reset HEAD~1\n\nHere is how to create the example repository:\n\n mkdir example && cd example && git init\n mkdir -p .iac/configs/devops\n cat >.iac/configs/devops/config.yml <> foo.c\n git add -A && git commit -m init\n\n echo baz >> foo.c\ncat >.iac/configs/devops/config.yml <queue[i] = NULL;\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t}\n-- \n2.52.0\n\n"},{"id":"535790","messageId":"xmqqfr77upxj.fsf@gitster.g","threadId":"64976","inReplyTo":"20260211041128.48412-2-hanyang.tony@bytedance.com","subject":"Re: [PATCH 1/1] diffcore-break: prevent dangling pointer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-02-11T17:54:00Z","receivedAt":"2026-02-11T17:54:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Han Young writes:\n\n> After we have freed the file pair, we should set the queue reference to null.\n> This prevents us from encountering a dangling pointer later on.\n> ---\n> diffcore-break.c | 1 +\n> 1 file changed, 1 insertion(+)\n\nMissing are sign-off and tests.\n\nThis reminds me of 56d388e6 (diff: avoid segfault with freed\nentries, 2025-12-29).\n>\n> diff --git a/diffcore-break.c b/diffcore-break.c\n> index c4c2173f30..9b11fe2fa0 100644\n> --- a/diffcore-break.c\n> +++ b/diffcore-break.c\n> @@ -222,6 +222,7 @@ void diffcore_break(struct repository *r, int break_score)\n> \t\t\t\tfree(p); /* not diff_free_filepair(), we are\n> \t\t\t\t\t * reusing one and two here.\n> \t\t\t\t\t */\n> +\t\t\t\tq->queue[i] = NULL;\n> \t\t\t\tcontinue;\n> \t\t\t}\n> \t\t}\n\n"},{"id":"535833","messageId":"20260212072002.2347-1-hanyang.tony@bytedance.com","threadId":"64976","inReplyTo":"20260211041128.48412-1-hanyang.tony@bytedance.com","subject":"[PATCH v2 0/1] diffcore-break: prevent dangling pointer","fromName":"Han Young","fromEmail":"hanyang.tony@bytedance.com","sentAt":"2026-02-12T07:20:01Z","receivedAt":"2026-02-12T07:20:10Z","isPatch":true,"sender":{"key":"hanyang.tony@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/108711387?v=4"},"body":"This bug is difficult to trigger, git-diff won't trigger the bug because\nin git-diff, the prefetching happens before the break-rewrites.\nThe test uses git reset to trigger it.\n\nChanges since v1:\n* Added the test\n\nHan Young (1):\n diffcore-break: prevent dangling pointer\n\n diffcore-break.c | 1 +\n t/t4067-diff-partial-clone.sh | 30 ++++++++++++++++++++++++++++++\n 2 files changed, 31 insertions(+)\n\n-- \n2.52.0\n\n"},{"id":"535834","messageId":"20260212072002.2347-2-hanyang.tony@bytedance.com","threadId":"64976","inReplyTo":"20260212072002.2347-1-hanyang.tony@bytedance.com","subject":"[PATCH v2 1/1] diffcore-break: prevent dangling pointer","fromName":"Han Young","fromEmail":"hanyang.tony@bytedance.com","sentAt":"2026-02-12T07:20:02Z","receivedAt":"2026-02-12T07:20:14Z","isPatch":true,"sender":{"key":"hanyang.tony@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/108711387?v=4"},"body":"After we have freed the file pair, we should set the queue reference to null.\nThis prevents us from encountering a dangling pointer later on.\n\nThe test uses git reset to trigger prefetching after break-rewrites have freed\nthe file pair.\n\nSigned-off-by: Han Young \n---\n diffcore-break.c | 1 +\n t/t4067-diff-partial-clone.sh | 30 ++++++++++++++++++++++++++++++\n 2 files changed, 31 insertions(+)\n\ndiff --git a/diffcore-break.c b/diffcore-break.c\nindex c4c2173f30..9b11fe2fa0 100644\n--- a/diffcore-break.c\n+++ b/diffcore-break.c\n@@ -222,6 +222,7 @@ void diffcore_break(struct repository *r, int break_score)\n \t\t\t\tfree(p); /* not diff_free_filepair(), we are\n \t\t\t\t\t * reusing one and two here.\n \t\t\t\t\t */\n+\t\t\t\tq->queue[i] = NULL;\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t}\ndiff --git a/t/t4067-diff-partial-clone.sh b/t/t4067-diff-partial-clone.sh\nindex 72f25de449..a980cd30a0 100755\n--- a/t/t4067-diff-partial-clone.sh\n+++ b/t/t4067-diff-partial-clone.sh\n@@ -132,6 +132,36 @@ 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 prefetch triggered by break-rewrites' '\n+\ttest_when_finished \"rm -rf server client trace\" &&\n+\n+\ttest_create_repo server &&\n+\techo xyz >server/foo &&\n+\tmkdir server/bar &&\n+\ttest_seq -f \"line %d\" 1 100 >server/bar/baz &&\n+\tgit -C server add -A &&\n+\tgit -C server commit -m x &&\n+\n+\n+\techo xyzz >server/foo &&\n+\trm server/bar/baz &&\n+\ttest_seq -f \"line %d\" 90 190 >server/bar/baz &&\n+\tgit -C server add -A &&\n+\tgit -C server commit -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+\t# Fetch bar/baz without fetching foo.\n+\tgit -C client checkout HEAD~1 bar &&\n+\t# Ensure baz has diff\n+\tgit -C client reset --hard HEAD &&\n+\n+\t# reset's break-rewrites detection will trigger prefetch\n+\tgit -C client reset HEAD~1\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-- \n2.52.0\n\n"},{"id":"535880","messageId":"xmqqseb5okl5.fsf@gitster.g","threadId":"64976","inReplyTo":"20260212072002.2347-2-hanyang.tony@bytedance.com","subject":"Re: [PATCH v2 1/1] diffcore-break: prevent dangling pointer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-02-12T18:58:14Z","receivedAt":"2026-02-12T18:58:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Han Young writes:\n\n> After we have freed the file pair, we should set the queue reference to null.\n> This prevents us from encountering a dangling pointer later on.\n\nI sense that \"This prevents ... later on\" needs further be\nclarified, since it is totally unclear what \"later on\" refers to.\nWe are done with the old filepair, and have no reason to revisit the\nq->queue[] item ourselves, but somebody later attempts to use it.\nWho is it and why does it do so? That is a natural question readers\nof the above description would ask, isn't it?\n\n Side note: I am guessiong that this is similar to the problem\n fixed with recent 56d388e6 (diff: avoid segfault with freed\n entries, 2025-12-29), where pointers in diff_queue->queue[] that\n point at file pairs that have been freed were mistakenly used by\n diff_queued_diff_prefetch() to populate them. Is a prefetch\n happening in \"git reset --mixed\" that calls read_from_tree()\n which calls update_index_from_diff() via do_diff_cache()?\n\n> The test uses git reset to trigger prefetching after break-rewrites have freed\n> the file pair.\n>\n> Signed-off-by: Han Young \n> ---\n> diffcore-break.c | 1 +\n> t/t4067-diff-partial-clone.sh | 30 ++++++++++++++++++++++++++++++\n> 2 files changed, 31 insertions(+)\n>\n> diff --git a/diffcore-break.c b/diffcore-break.c\n> index c4c2173f30..9b11fe2fa0 100644\n> --- a/diffcore-break.c\n> +++ b/diffcore-break.c\n> @@ -222,6 +222,7 @@ void diffcore_break(struct repository *r, int break_score)\n> \t\t\t\tfree(p); /* not diff_free_filepair(), we are\n> \t\t\t\t\t * reusing one and two here.\n> \t\t\t\t\t */\n> +\t\t\t\tq->queue[i] = NULL;\n> \t\t\t\tcontinue;\n> \t\t\t}\n> \t\t}\n> diff --git a/t/t4067-diff-partial-clone.sh b/t/t4067-diff-partial-clone.sh\n> index 72f25de449..a980cd30a0 100755\n> --- a/t/t4067-diff-partial-clone.sh\n> +++ b/t/t4067-diff-partial-clone.sh\n> @@ -132,6 +132,36 @@ 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 prefetch triggered by break-rewrites' '\n> +\ttest_when_finished \"rm -rf server client trace\" &&\n> +\n> +\ttest_create_repo server &&\n> +\techo xyz >server/foo &&\n> +\tmkdir server/bar &&\n> +\ttest_seq -f \"line %d\" 1 100 >server/bar/baz &&\n> +\tgit -C server add -A &&\n> +\tgit -C server commit -m x &&\n> +\n> +\n> +\techo xyzz >server/foo &&\n\nThe blank line above does not have to be doubled, I think. So the\nfirst commit yas \"xyz\" in \"foo\", and 100 lines 1..100 in \"bar/baz\".\n\n> +\trm server/bar/baz &&\n\nWe are overwriting it, so I am not sure why this \"rm\" is needed. Is\nit necessary to avoid reusing the same i-num for the file to avoid\nracily clean condition, or something? I find it unlikely because\nthe length of the new contents ...\n\n> +\ttest_seq -f \"line %d\" 90 190 >server/bar/baz &&\n\n... is different from the original.\n\n> +\tgit -C server add -A &&\n> +\tgit -C server commit -m x &&\n\nIn any case, the second commit has \"xyzz\" in \"foo\", and different\n100 lines 90..190 in \"bar/baz\".\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\nAnd we grab both commits and their trees, but not any blob.\n\n> +\t# Fetch bar/baz without fetching foo.\n> +\tgit -C client checkout HEAD~1 bar &&\n\nAnd we move \"bar/baz\" back to the 1..100 (the tip of the history\nimmediately after cloning had 90..190).\n\n> +\t# Ensure baz has diff\n> +\tgit -C client reset --hard HEAD &&\n\nI am not sure what the comment wants to say. Before this hard\nreset, we did have modification relative to HEAD in bar/baz; with a\nhard reset, we are ensuring that everything including bar/baz\nexactly match HEAD, aren't we?\n\n> +\t# reset's break-rewrites detection will trigger prefetch\n\n\"reset's break-rewrites detection\" -> \"break-rewrites detction in reset\"\nor something to avoid the \"'\"; otherwise you'd get\n\n error: bug in the test script: not 2 or 3 parameters to test-expect-success\n\nYou rewrote this line as a part of the last-minute change before you\nran the test for the last time, or something?\n\nAnyway, starting from the \"everything clean wrt HEAD\" state, we try\nto move to the HEAD~1. We already have both 1..100 and 90..190\nversions of bar/baz (and HEAD: and HEAD~1: trees), and we have the\ncontents for foo in HEAD, which is \"xyzz\", but we have never seen\nHEAD~1:foo so far. \"reset --mixed HEAD~1\" requires us to obtain\nit...\n\n> +\tgit -C client reset HEAD~1\n\n... and cause us to run the prefetch to obtain \"foo\", but it runs\ndo_diff_cache() and makes it notice bar/baz has changed too much?\n\nYour \"do not leave q->queue[] dangling, as other people may still\nlook at them\" fix certainly is a good hygiene, but I have to wonder\nwhy we are doing break detection in this case in the first place.\nFor the internal \"Let's figure out which path have changed, so that\nwe re-read only those changed paths\" invocation of diff machinery,\nwe should not be doing so. A break detection is to see if the\nchange in the contents of a single path is a total rewrite, and\nregardless of the answer, the fact that the path was modified does\nnot change, update_index_from_diff() would work on the path anyway.\nI also suspect that, if we are doing rename detection in this call\nto do_diff_cache(), it is a totally wasted effort. We may want to\ntake a deeper look at it, possibly outside the theme of this more\nfocused fix.\n\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\nBy the way, I find it highly curious that with the following patch\nto revert the fix with a bit of extra output sprinkled to your\ntests, the problem does not reproduce reliably, which may indicate\nthat your test may be flaky (i.e., timing dependent). Am I doing\nsomething bogus in the patch?\n\n * revert of the fix in diffcore-break.c is a deliberate attempt to\n reproduce the problem, and \"echo/cd client/ls\" are for inspection\n so you should not take it in your final version.\n\n * removal of double blank lines is a style fix I'd want you to take.\n\n * \"reset's\" fix is a shell script syntax fix I'd want you to take.\n\nThanks.\n\n\ndiff --git c/diffcore-break.c w/diffcore-break.c\nindex 9b11fe2fa0..c4c2173f30 100644\n--- c/diffcore-break.c\n+++ w/diffcore-break.c\n@@ -222,7 +222,6 @@ void diffcore_break(struct repository *r, int break_score)\n \t\t\t\tfree(p); /* not diff_free_filepair(), we are\n \t\t\t\t\t * reusing one and two here.\n \t\t\t\t\t */\n-\t\t\t\tq->queue[i] = NULL;\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t}\ndiff --git c/t/t4067-diff-partial-clone.sh w/t/t4067-diff-partial-clone.sh\nindex a980cd30a0..b5a35ded99 100755\n--- c/t/t4067-diff-partial-clone.sh\n+++ w/t/t4067-diff-partial-clone.sh\n@@ -142,7 +142,6 @@ test_expect_success 'diff succeeds even if prefetch triggered by break-rewrites'\n \tgit -C server add -A &&\n \tgit -C server commit -m x &&\n \n-\n \techo xyzz >server/foo &&\n \trm server/bar/baz &&\n \ttest_seq -f \"line %d\" 90 190 >server/bar/baz &&\n@@ -153,12 +152,22 @@ test_expect_success 'diff succeeds even if prefetch triggered by break-rewrites'\n \ttest_config -C server uploadpack.allowanysha1inwant 1 &&\n \tgit clone --filter=blob:limit=0 \"file://$(pwd)/server\" client &&\n \n+echo after clone &&\n+(cd client && ls -l foo bar/baz) &&\n+\n \t# Fetch bar/baz without fetching foo.\n \tgit -C client checkout HEAD~1 bar &&\n+\n+echo after checkout HEAD~1 bar &&\n+(cd client && ls -l foo bar/baz) &&\n+\n \t# Ensure baz has diff\n \tgit -C client reset --hard HEAD &&\n \n-\t# reset's break-rewrites detection will trigger prefetch\n+echo after reset --hard HEAD &&\n+(cd client && ls -l foo bar/baz) &&\n+\n+\t# break-rewrites detection in reset will trigger prefetch\n \tgit -C client reset HEAD~1\n '\n \n"},{"id":"535912","messageId":"CAG1j3zH0C0DA+V35A1e73wi41gmk9Xry6gmtZM3w3LT09etntQ@mail.gmail.com","threadId":"64976","inReplyTo":"xmqqseb5okl5.fsf@gitster.g","subject":"Re: [External] Re: [PATCH v2 1/1] diffcore-break: prevent dangling pointer","fromName":"Han Young","fromEmail":"hanyang.tony@bytedance.com","sentAt":"2026-02-13T07:14:03Z","receivedAt":"2026-02-13T07:14:16Z","isPatch":true,"sender":{"key":"hanyang.tony@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/108711387?v=4"},"body":"On Fri, Feb 13, 2026 at 2:58 AM Junio C Hamano wrote:\n> I sense that \"This prevents ... later on\" needs further be\n> clarified, since it is totally unclear what \"later on\" refers to.\n> We are done with the old filepair, and have no reason to revisit the\n> q->queue[] item ourselves, but somebody later attempts to use it.\n> Who is it and why does it do so? That is a natural question readers\n> of the above description would ask, isn't it?\n\nSorry, I'll try to describe the problem thoroughly in version 3 of the patch.\n\n> > + echo xyzz >server/foo &&\n>\n> The blank line above does not have to be doubled, I think. So the\n> first commit yas \"xyz\" in \"foo\", and 100 lines 1..100 in \"bar/baz\"\n\nYes, I wasn't being careful, I will ensure there are no double blank lines.\n\n> > + rm server/bar/baz &&\n>\n> We are overwriting it, so I am not sure why this \"rm\" is needed. Is\n> it necessary to avoid reusing the same i-num for the file to avoid\n> racily clean condition, or something? I find it unlikely because\n> the length of the new contents ...\n\nThis is an artifact from before I found the test_seq helper function.\nI will remove it.\n\n> > + # Ensure baz has diff\n> > + git -C client reset --hard HEAD &&\n>\n> I am not sure what the comment wants to say. Before this hard\n> reset, we did have modification relative to HEAD in bar/baz; with a\n> hard reset, we are ensuring that everything including bar/baz\n> exactly match HEAD, aren't we?\n\nThis resets bar/baz to the HEAD's version. So that in the reset below,\nThe `bar/baz` in the worktree is different from the `bar/baz` in HEAD~1.\nWe rely on bar/baz to be broken into delete/create to trigger the\nuse-after-free bug. I'll clarify the comment in v3.\n\n> > + # reset's break-rewrites detection will trigger prefetch\n>\n> \"reset's break-rewrites detection\" -> \"break-rewrites detction in reset\"\n> or something to avoid the \"'\"; otherwise you'd get\n>\n> error: bug in the test script: not 2 or 3 parameters to test-expect-success\n>\n> You rewrote this line as a part of the last-minute change before you\n> ran the test for the last time, or something?\n\nSorry, I only added the comments after finish writing the test, and\nforgot to run the test again.\n\n> ... and cause us to run the prefetch to obtain \"foo\", but it runs\n> do_diff_cache() and makes it notice bar/baz has changed too much?\n>\n> Your \"do not leave q->queue[] dangling, as other people may still\n> look at them\" fix certainly is a good hygiene, but I have to wonder\n> why we are doing break detection in this case in the first place.\n> For the internal \"Let's figure out which path have changed, so that\n> we re-read only those changed paths\" invocation of diff machinery,\n> we should not be doing so. A break detection is to see if the\n> change in the contents of a single path is a total rewrite, and\n> regardless of the answer, the fact that the path was modified does\n> not change, update_index_from_diff() would work on the path anyway.\n> I also suspect that, if we are doing rename detection in this call\n> to do_diff_cache(), it is a totally wasted effort. We may want to\n> take a deeper look at it, possibly outside the theme of this more\n> focused fix.\n\nI'm not familiar with reset and diff machinery; I encountered this bug\nduring a real world mixed reset. The segmentfault calling stack is\ncmd_reset -> read_from_tree -> diffcore_std -> diffcore_break\nIt looks like rename detection is indeed pointless.\n\n> By the way, I find it highly curious that with the following patch\n> to revert the fix with a bit of extra output sprinkled to your\n> tests, the problem does not reproduce reliably, which may indicate\n> that your test may be flaky (i.e., timing dependent). Am I doing\n> something bogus in the patch?\n\nIt seems the problem does not reproduce reliably with or without your\npatch. I suspect that could be due to the freed memory on some\noccasions isn't reused by system, thus the access later on doesn't\ntrigger a segment fault. On my macOS system, the test passes around\n5% of the time. However, if I set q->queue[i] to a bogus memory\nlocation like 0x1 causes a Git segment fault every time.\nIs there a better way to write tests for this kind of situation?\n\nThanks.\n"},{"id":"535945","messageId":"xmqqwm0gmumn.fsf@gitster.g","threadId":"64976","inReplyTo":"CAG1j3zH0C0DA+V35A1e73wi41gmk9Xry6gmtZM3w3LT09etntQ@mail.gmail.com","subject":"Re: [External] Re: [PATCH v2 1/1] diffcore-break: prevent dangling pointer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-02-13T17:16:32Z","receivedAt":"2026-02-13T17:16:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Han Young writes:\n\n>> Your \"do not leave q->queue[] dangling, as other people may still\n>> look at them\" fix certainly is a good hygiene, but I have to wonder\n>> why we are doing break detection in this case in the first place.\n>> For the internal \"Let's figure out which path have changed, so that\n>> we re-read only those changed paths\" invocation of diff machinery,\n>> we should not be doing so. A break detection is to see if the\n>> change in the contents of a single path is a total rewrite, and\n>> regardless of the answer, the fact that the path was modified does\n>> not change, update_index_from_diff() would work on the path anyway.\n>> I also suspect that, if we are doing rename detection in this call\n>> to do_diff_cache(), it is a totally wasted effort. We may want to\n>> take a deeper look at it, possibly outside the theme of this more\n>> focused fix.\n>\n> I'm not familiar with reset and diff machinery; I encountered this bug\n> during a real world mixed reset. The segmentfault calling stack is\n> cmd_reset -> read_from_tree -> diffcore_std -> diffcore_break\n> It looks like rename detection is indeed pointless.\n>\n>> By the way, I find it highly curious that with the following patch\n>> to revert the fix with a bit of extra output sprinkled to your\n>> tests, the problem does not reproduce reliably, which may indicate\n>> that your test may be flaky (i.e., timing dependent). Am I doing\n>> something bogus in the patch?\n>\n> It seems the problem does not reproduce reliably with or without your\n> patch. I suspect that could be due to the freed memory on some\n> occasions isn't reused by system, thus the access later on doesn't\n> trigger a segment fault. On my macOS system, the test passes around\n> 5% of the time. However, if I set q->queue[i] to a bogus memory\n> location like 0x1 causes a Git segment fault every time.\n> Is there a better way to write tests for this kind of situation?\n\nIf it is flaky because it depends on the way the system allocator\nhappens to reuse or not reuse a piece of memory, as long as we do\nnot get hit by false positives, it would be OK to leave it as-is if\nwe do not find a good solution, because running tests under asan\nwould catch such problems, I think.\n\nThanks.\n"},{"id":"536916","messageId":"20260224061329.60364-1-hanyang.tony@bytedance.com","threadId":"64976","inReplyTo":"20260212072002.2347-1-hanyang.tony@bytedance.com","subject":"[PATCH v3 0/1] diffcore-break: avoid segfault with freed entries","fromName":"Han Young","fromEmail":"hanyang.tony@bytedance.com","sentAt":"2026-02-24T06:13:28Z","receivedAt":"2026-02-24T06:13:39Z","isPatch":true,"sender":{"key":"hanyang.tony@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/108711387?v=4"},"body":"Changes since v2:\n* fixed the ' in test, and removed unused lines\n* clarify the cause of segfault in commit message\n\nHan Young (1):\n diffcore-break: avoid segfault with freed entries\n\n diffcore-break.c | 1 +\n t/t4067-diff-partial-clone.sh | 31 +++++++++++++++++++++++++++++++\n 2 files changed, 32 insertions(+)\n\n-- \n2.52.0\n\n"},{"id":"536917","messageId":"20260224061329.60364-2-hanyang.tony@bytedance.com","threadId":"64976","inReplyTo":"20260224061329.60364-1-hanyang.tony@bytedance.com","subject":"[PATCH v3 1/1] diffcore-break: avoid segfault with freed entries","fromName":"Han Young","fromEmail":"hanyang.tony@bytedance.com","sentAt":"2026-02-24T06:13:29Z","receivedAt":"2026-02-24T06:13:41Z","isPatch":true,"sender":{"key":"hanyang.tony@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/108711387?v=4"},"body":"After we have freed the file pair, we should set the queue reference to null.\nWhen computing a diff in a partial clone, there is a chance that we\ncould trigger a prefetch of missing objects when there are freed entries in\nthe global diff queue due to break-rewrites detection. The segfault only occurs\nif an entry has been freed by break-rewrites and there is an entry\nto be prefetched.\n\nThere is a new test in t4067 that trigger the segmentation fault that results\nin this case. The test explicitly fetch the necessary blobs to trigger the\nbreak rewrites, some blobs are left to be prefetched.\n\nThe fix is to set the queue pointer to NULL after it is freed, the prefetch\nwill skip NULL entries.\n\nSigned-off-by: Han Young \n---\n diffcore-break.c | 1 +\n t/t4067-diff-partial-clone.sh | 31 +++++++++++++++++++++++++++++++\n 2 files changed, 32 insertions(+)\n\ndiff --git a/diffcore-break.c b/diffcore-break.c\nindex c4c2173f30..9b11fe2fa0 100644\n--- a/diffcore-break.c\n+++ b/diffcore-break.c\n@@ -222,6 +222,7 @@ void diffcore_break(struct repository *r, int break_score)\n \t\t\t\tfree(p); /* not diff_free_filepair(), we are\n \t\t\t\t\t * reusing one and two here.\n \t\t\t\t\t */\n+\t\t\t\tq->queue[i] = NULL;\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t}\ndiff --git a/t/t4067-diff-partial-clone.sh b/t/t4067-diff-partial-clone.sh\nindex 72f25de449..30813109ac 100755\n--- a/t/t4067-diff-partial-clone.sh\n+++ b/t/t4067-diff-partial-clone.sh\n@@ -132,6 +132,37 @@ 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 prefetch triggered by break-rewrites' '\n+\ttest_when_finished \"rm -rf server client trace\" &&\n+\n+\ttest_create_repo server &&\n+\techo xyz >server/foo &&\n+\tmkdir server/bar &&\n+\ttest_seq -f \"line %d\" 1 100 >server/bar/baz &&\n+\tgit -C server add -A &&\n+\tgit -C server commit -m x &&\n+\n+\techo xyzz >server/foo &&\n+\ttest_seq -f \"line %d\" 90 190 >server/bar/baz &&\n+\tgit -C server add -A &&\n+\tgit -C server commit -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+\t# Fetch bar/baz without fetching foo.\n+\t# Foo will be lazily fetched during break rewrites detection.\n+\tgit -C client checkout HEAD~1 bar &&\n+\n+\t# Ensure baz in the working tree is different from baz in HEAD~1.\n+\t# We need baz to trigger break-rewrites detection.\n+\tgit -C client reset --hard HEAD &&\n+\n+\t# break-rewrites detction in reset.\n+\tgit -C client reset HEAD~1\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-- \n2.52.0\n\n"},{"id":"536979","messageId":"xmqq8qcijhe6.fsf@gitster.g","threadId":"64976","inReplyTo":"20260224061329.60364-1-hanyang.tony@bytedance.com","subject":"Re: [PATCH v3 0/1] diffcore-break: avoid segfault with freed entries","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-02-24T15:22:25Z","receivedAt":"2026-02-24T15:22:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Han Young writes:\n\n> Changes since v2:\n> * fixed the ' in test, and removed unused lines\n> * clarify the cause of segfault in commit message\n>\n> Han Young (1):\n> diffcore-break: avoid segfault with freed entries\n>\n> diffcore-break.c | 1 +\n> t/t4067-diff-partial-clone.sh | 31 +++++++++++++++++++++++++++++++\n> 2 files changed, 32 insertions(+)\n\nThanks, will replace.\n"}]}