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

From: Jeff King <peff@peff.net>
Date: Apr 1, 2017, 08:20 UTC
Message-ID: <20170401082059.wdgvcv2qv56b75b4@sigill.intra.peff.net>
In-Reply-To: <CACsJy8BR9=Ju_7tSmZEH+UDqWG3Ud1KXY-m6r_jHb-aQUoUMAg@mail.gmail.com>

On Thu, Mar 30, 2017 at 04:24:40PM +0700, Duy Nguyen wrote:

Show 16 quoted lines

> On Thu, Mar 30, 2017 at 12:56 AM, Jeff King <peff@peff.net> wrote:
> > 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 thought we killed most of those (was it your patches?).

Yes, after fixing a bug where static buffer reuse caused git to randomly delete a ref, I rage-converted most of the dangerous looking cases.

Show 5 quoted lines

> I had a quick look at "git grep -w git_path" again. The ones in
> builtin/am.c, builtin/grep.c and submodule.c look very much like that
> anti-pattern. The one in read_index_from() probably should be replaced
> with git_pathdup() as well. Sorry no patches (I'm very slow these
> days).

Yeah, I think a number of them are actually OK if you dig (e.g., passing it to am_state_init() immediately duplicates the result), but it's a bad pattern if you have to dig to see if it's right. It's hard to tell when a sub-function may reuse the buffer. For instance, git-init passes the result to adjust_shared_perm(), which might lazily load the config from disk. I don't know if that calls git_path() or not, but it's an awful lot of code to run.

A lot of the cases look like they could be fixed by using git_path_foo() instead of git_path("FOO"). (And in many cases we even have git_path_foo() defined already!).

My favorite is the one in add_worktree(), which calls strbuf_addstr() on the result of git_path(0. That one's _not_ dangerous, but surely it would be simpler to just write directly into the strbuf. :)

-Peff