Re: [PATCH 0/17] removing questionable uses of git_path

From: Jeff King <peff@peff.net>
Date: Aug 10, 2015, 17:47 UTC
Message-ID: <20150810174743.GB20546@sigill.intra.peff.net>
In-Reply-To: <xmqqr3nbuk23.fsf@gitster.dls.corp.google.com>

On Mon, Aug 10, 2015 at 10:31:32AM -0700, Junio C Hamano wrote:

Show 8 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > The problem is that git_path uses a static buffer that gets overwritten
> > by subsequent calls.
> 
> As the rotating static buffer pattern used in get_pathname() was
> modeled after sha1_to_hex(), we have the same issue there.  They are
> troubles waiting to happen unless the callers are careful.

Yeah, I think Michael mentioned that to me off-list. I don't _think_ sha1_to_hex is nearly such a problem, because we tend to store sha1s in their binary form. So sha1_to_hex is almost always an argument to fprintf() or similar.

Of course there are some exceptions. :) I notice we pass one return value to add_pending_object, which was almost certainly horribly broken before 31faeb2 started strdup'ing object_array names.

So certainly it would be nice to audit them all, but there are over 600 calls. Given the likelihood of finding a useful bug, I'm not sure it's the greatest use of developer time.

Show 7 quoted lines

> > producing a fairly tame-looking set of function calls. It's OK to pass
> > the result of git_path() to a system call, or something that is a thin
> > wrapper around one (e.g., strbuf_read_file).
> 
> That is a short and good rule to follow, but the problem is that not
> everybody has good taste to interpret the above rule and apply it with
> an eye toward maintainability X-<.

Yeah. Part of me wants to eradicate git_path entirely. My series takes out over half of the calls, but there are still close to 100, I think. I think it would make the code worse to convert all of them naively. We could provide format-aware wrappers for some filesystem functions, like:

  git_stat(&st, "%s", refname);

or something. That feels horribly coupled compared to:

  stat(git_path("%s", refname), &st);

but it makes it very clear what the memory ownership/lifetime rules are.

Anyway, part of my goal with the series was not just to clean up the existing suspicious spots, but to raise awareness of the issue. At least for me, I know it's something I will look for more carefully when reviewing patches. Once bitten, twice shy. :)

-Peff