Re: [PATCH] sha1_loose_object_info: handle errors from unpack_sha1_rest

From: Jeff King <peff@peff.net>
Date: Oct 6, 2017, 04:38 UTC
Message-ID: <20171006043847.hja2yjlvatckgy4d@sigill.intra.peff.net>
In-Reply-To: <20171006043008.2s6pg4aurwledf3s@sigill.intra.peff.net>

On Fri, Oct 06, 2017 at 12:30:08AM -0400, Jeff King wrote:

Show 23 quoted lines

> 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@peff.net>
> > > ---
> > 
> > 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