Re: [PATCH] read-cache: avoid git_path() race in freshen_shared_index()

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 30, 2017, 16:00 UTC
Message-ID: <xmqq7f367o0s.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CAP8UFD29xWU5bHwewUSq26pVTwK-gy3uSVG2aGTCrDak3620=A@mail.gmail.com>

Christian Couder <christian.couder@gmail.com> writes:

Show 10 quoted lines

> On Wed, Mar 29, 2017 at 7:56 PM, Jeff King <peff@peff.net> wrote:
> ...
>> Yeah, it looks like that is what happened. I see that Christian bisected
>> the rebase to find the commit in the series that introduces the problem.
>> I'm mildly curious which commit upstream created the problem[1].
>
> I bisected it to 18633e1a22 (rebase -i: use the rebase--helper
> builtin, 2017-02-09).
> This commit is indeed changing how the interactive rebase works, but
> it is not easy to see how it impact git_path() usage.

That small series activates a large body of code that hasn't been exercised so it is not surprising.

Show 18 quoted lines

> Yeah, I am very tempted to just rewrite the commit message like this:
>
> ------------
>
> When performing an interactive rebase in split-index mode,
> the commit message that one should rework when squashing commits
> can contain some garbage instead of the usual concatenation of
> both of the commit messages.
>
> Bisecting shows that c3a0082502 (read-cache: use
> freshen_shared_index() in read_index_from(), 2017-03-06) is involved,
> which points to the unsafe use of git_path() in
> freshen_shared_index().
>
> ------------
>
> and change the subject to "read-cache: avoid using git_path() in
> freshen_shared_index()".

Sure. It may not be even worth mentioning a not-so-useful bisect result, and the potential riskiness of the original code (iow why this fix is the right thing) can be seen from the patch alone.

Thanks for following it through.