Re: [PATCH 1/2] read_info_alternates: read contents into strbuf

From: Jeff King <peff@peff.net>
Date: Sep 19, 2017, 05:03 UTC
Message-ID: <20170919050316.nfbk6wckgdtmzvvd@sigill.intra.peff.net>
In-Reply-To: <20170919024253.GC175206@aiede.mtv.corp.google.com>

On Mon, Sep 18, 2017 at 07:42:53PM -0700, Jonathan Nieder wrote:

Show 9 quoted lines

> Jeff King wrote:
> 
> > Reported-by: Michael Haggerty <mhagger@alum.mit.edu>
> > Signed-off-by: Jeff King <peff@peff.net>
> > ---
> >  sha1_file.c | 29 +++++++++--------------------
> >  1 file changed, 9 insertions(+), 20 deletions(-)
> 
> Thanks for tracking it down.

To be fair, Michael did most of the work in identifying and bisecting the bug. He even wrote a very similar patch in parallel; I just swooped in at the end.

Show 13 quoted lines

> >  	path = xstrfmt("%s/info/alternates", relative_base);
> > -	fd = git_open(path);
> > -	free(path);
> > -	if (fd < 0)
> > -		return;
> > -	if (fstat(fd, &st) || (st.st_size == 0)) {
> > -		close(fd);
> > +	if (strbuf_read_file(&buf, path, 1024) < 0) {
> > +		free(path);
> >  		return;
> 
> strbuf_read_file is careful to release buf on failure, so this doesn't
> leak (but it's a bit subtle).

Yep. I didn't think it was worth calling out with a comment since the "don't allocate on failure" pattern is common to most of the strbuf functions.

> What happened to the !st.st_size case?  Is it eliminated for
> simplicity?

Good question, and the answer is yes. Obviously we can bail early on an empty file, but I don't think there's any reason to complicate the code with it (the original seems to come from d5a63b9983 (Alternate object pool mechanism updates., 2005-08-14), where it avoided a corner case that has long since been deleted.

-Peff