On Fri, Oct 06, 2017 at 12:30:08AM -0400, Jeff King wrote:
> On Fri, Oct 06, 2017 at 01:19:21PM +0900, Junio C Hamano wrote:
>
> > > But note that the leak in (2) is actually older than that.
> > > The original unpack_sha1_file() directly returned the result
> > > of unpack_sha1_rest() to its caller, when it should have
> > > been closing the zlib stream itself on error.
> > >
> > > Signed-off-by: Jeff King <peff@xxxxxxxx>
> > > ---
> >
> > Obviously correct. (2) is as old as Git itself; it eventually
> > blames down to e83c5163 ("Initial revision of "git", the information
> > manager from hell", 2005-04-07), where read-cache.c::unpack_sha1_file()
> > liberally returns NULL without cleaning up the zstream.
>
> Thanks, I as too lazy to dig down further, but I'm always interested to
> see the roots of these things (especially "bug in the original" versus
> "introduced by a careless refactor").
>
> I have a feeling that the world would be a better place if
> unpack_sha1_rest() just always promised to close the zstream, since no
> callers seem to want to look at it in the error case. But I wanted to go
> for the minimal fix first.
Actually, there are only two callers left these days. One of them leaks,
and the other immediately closes the zstream. So something like:
diff --git a/sha1_file.c b/sha1_file.c
index 09ad64ce55..cea003d182 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -978,10 +978,10 @@ static void *unpack_sha1_rest(git_zstream *stream, void *buffer, unsigned long s
while (status == Z_OK)
status = git_inflate(stream, Z_FINISH);
}
- if (status == Z_STREAM_END && !stream->avail_in) {
- git_inflate_end(stream);
+ git_inflate_end(stream);
+
+ if (status == Z_STREAM_END && !stream->avail_in)
return buf;
- }
if (status < 0)
error("corrupt loose object '%s'", sha1_to_hex(sha1));
@@ -2107,7 +2107,6 @@ int read_loose_object(const char *path,
*contents = unpack_sha1_rest(&stream, hdr, *size, expected_sha1);
if (!*contents) {
error("unable to unpack contents of %s", path);
- git_inflate_end(&stream);
goto out;
}
if (check_sha1_signature(expected_sha1, *contents,
seems reasonable. Doing it that (with my other patch on top) splits the
leak-fix and the not-yet-a-bug-but-confusing-error-return problems into
two separate patches.
I dunno. There aren't that many callers of unpack_sha1_rest(), so it may
not matter that much, but while we're here...
-Peff
