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

From: Jeff King <peff@peff.net>
Date: Mar 29, 2017, 17:56 UTC
Message-ID: <20170329175614.uecvtvmzoj2wfle3@sigill.intra.peff.net>
In-Reply-To: <xmqqfuhwau6r.fsf@gitster.mtv.corp.google.com>

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

Show 16 quoted lines

> > 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]. 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.

As an aside, this kind of static-buffer reuse _used_ to mean you might see somebody else's buffer. Which is bad enough. But since the move to use strbufs underneath the hood of git_path(), it may produce that effect or it may be a use-after-free (if the strbuf had to reallocate to grow in the meantime).

Anyway. The fix in the patch is obviously the right thing.

-Peff

[1] I think we could pinpoint the upstream change that caused the bad
    interaction by bisecting between the merge-base and the first-parent
    of the broken merge. For each commit, cherry-pick the complete
    series on top of it, and test the result.