Re: [PATCH] sha1_loose_object_info: handle errors from unpack_sha1_rest

From: Jeff King <peff@peff.net>
Date: Oct 6, 2017, 04:30 UTC
Message-ID: <20171006043008.2s6pg4aurwledf3s@sigill.intra.peff.net>
In-Reply-To: <xmqqr2ugykl2.fsf@gitster.mtv.corp.google.com>

On Fri, Oct 06, 2017 at 01:19:21PM +0900, Junio C Hamano wrote:

Show 12 quoted lines

> > 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.

-Peff