On Mon, Sep 18, 2017 at 07:42:53PM -0700, Jonathan Nieder wrote:
> Jeff King wrote:
>
> > Reported-by: Michael Haggerty <mhagger@xxxxxxxxxxxx>
> > Signed-off-by: Jeff King <peff@xxxxxxxx>
> > ---
> > 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.
> > 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
