Re: [External] Re: [PATCH v2 1/1] diffcore-break: prevent dangling pointer

From: Han Young <hanyang.tony@bytedance.com>
Date: Feb 13, 2026, 07:14 UTC
Message-ID: <CAG1j3zH0C0DA+V35A1e73wi41gmk9Xry6gmtZM3w3LT09etntQ@mail.gmail.com>
In-Reply-To: <xmqqseb5okl5.fsf@gitster.g>

On Fri, Feb 13, 2026 at 2:58 AM Junio C Hamano <gitster@pobox.com> wrote:

Show 6 quoted lines

> I sense that "This prevents ... later on" needs further be
> clarified, since it is totally unclear what "later on" refers to.
> We are done with the old filepair, and have no reason to revisit the
> q->queue[] item ourselves, but somebody later attempts to use it.
> Who is it and why does it do so?  That is a natural question readers
> of the above description would ask, isn't it?

Sorry, I'll try to describe the problem thoroughly in version 3 of the patch.

> > +     echo xyzz >server/foo &&
>
> The blank line above does not have to be doubled, I think.  So the
> first commit yas "xyz" in "foo", and 100 lines 1..100 in "bar/baz"

Yes, I wasn't being careful, I will ensure there are no double blank lines.

Show 6 quoted lines

> > +     rm server/bar/baz &&
>
> We are overwriting it, so I am not sure why this "rm" is needed.  Is
> it necessary to avoid reusing the same i-num for the file to avoid
> racily clean condition, or something?  I find it unlikely because
> the length of the new contents ...

This is an artifact from before I found the test_seq helper function. I will remove it.

Show 7 quoted lines

> > +     # Ensure baz has diff
> > +     git -C client reset --hard HEAD &&
>
> I am not sure what the comment wants to say.  Before this hard
> reset, we did have modification relative to HEAD in bar/baz; with a
> hard reset, we are ensuring that everything including bar/baz
> exactly match HEAD, aren't we?

This resets bar/baz to the HEAD's version. So that in the reset below, The `bar/baz` in the worktree is different from the `bar/baz` in HEAD~1. We rely on bar/baz to be broken into delete/create to trigger the use-after-free bug. I'll clarify the comment in v3.

Show 9 quoted lines

> > +     # reset's break-rewrites detection will trigger prefetch
>
> "reset's break-rewrites detection" -> "break-rewrites detction in reset"
> or something to avoid the "'"; otherwise you'd get
>
>     error: bug in the test script: not 2 or 3 parameters to test-expect-success
>
> You rewrote this line as a part of the last-minute change before you
> ran the test for the last time, or something?

Sorry, I only added the comments after finish writing the test, and forgot to run the test again.

Show 16 quoted lines

> ... and cause us to run the prefetch to obtain "foo", but it runs
> do_diff_cache() and makes it notice bar/baz has changed too much?
>
> Your "do not leave q->queue[] dangling, as other people may still
> look at them" fix certainly is a good hygiene, but I have to wonder
> why we are doing break detection in this case in the first place.
> For the internal "Let's figure out which path have changed, so that
> we re-read only those changed paths" invocation of diff machinery,
> we should not be doing so.  A break detection is to see if the
> change in the contents of a single path is a total rewrite, and
> regardless of the answer, the fact that the path was modified does
> not change, update_index_from_diff() would work on the path anyway.
> I also suspect that, if we are doing rename detection in this call
> to do_diff_cache(), it is a totally wasted effort.  We may want to
> take a deeper look at it, possibly outside the theme of this more
> focused fix.

I'm not familiar with reset and diff machinery; I encountered this bug during a real world mixed reset. The segmentfault calling stack is cmd_reset -> read_from_tree -> diffcore_std -> diffcore_break It looks like rename detection is indeed pointless.

Show 5 quoted lines

> By the way, I find it highly curious that with the following patch
> to revert the fix with a bit of extra output sprinkled to your
> tests, the problem does not reproduce reliably, which may indicate
> that your test may be flaky (i.e., timing dependent).  Am I doing
> something bogus in the patch?

It seems the problem does not reproduce reliably with or without your patch. I suspect that could be due to the freed memory on some occasions isn't reused by system, thus the access later on doesn't trigger a segment fault. On my macOS system, the test passes around 5% of the time. However, if I set q->queue[i] to a bogus memory location like 0x1 causes a Git segment fault every time. Is there a better way to write tests for this kind of situation?

Thanks.