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

On Wed, Mar 29, 2017 at 7:56 PM, Jeff King [off-list ref] wrote:

On Wed, Mar 29, 2017 at 10:06:52AM -0700, Junio C Hamano wrote:

quoted

quoted

This shows that we should be careful not to use git_path() in
freshen_shared_index(). It is using a shared buffer that can
too easily lead to races.

The impression I get from the symptom is that after git_path() is
called here, before check_and_freshen_file() uses that result, it
(or functions it calls) uses git_path(), and the number of times it
does so has changed since cc/split-index-config was written on the
mainline, and the rotating 4-element buffer get_pathname() gives is
now exhausted, leading to the failure you observed.  By the way,
that does not sound a race to me.

In any case, that explains why bisect says the merge is the first
bad one, and cures the confused reader ;-) The use of git_path() on
the topic was still safe; it was a timebomb waiting to go off.  The
mainline started using more calls and the merge result was unsafe.

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.

There's a reasonable chance it's some innocent-looking cleanup (possibly
one of my recent "stop using a fixed buffer" ones).

But in the end it doesn't really matter. I think code like:

  const char *filename = git_path(...);

or

  nontrivial_function(git_path(...));

is an anti-pattern. It _might_ be safe, but it's really hard to tell
without following the complete lifetime of the return value. I've been
tempted to suggest we should abolish git_path() entirely. But it's so
darn useful for things like unlink(git_path(...)), or other direct
system calls.

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()".