Re: [PATCHv4 3/4] bundle: don't leak an fd in case of early return

From: Jeff King <peff@peff.net>
Date: Apr 1, 2016, 17:15 UTC
Message-ID: <20160401171535.GB20345@sigill.intra.peff.net>
In-Reply-To: <xmqqzitdi7iq.fsf@gitster.mtv.corp.google.com>

On Fri, Apr 01, 2016 at 10:05:49AM -0700, Junio C Hamano wrote:

Show 15 quoted lines

> Stefan Beller <sbeller@google.com> writes:
> 
> > In successful operation `write_pack_data` will close the `bundle_fd`,
> > but when we exit early, we need to take care of the file descriptor
> > as well as the lock file ourselves. The lock file may be deleted at the
> > end of running the program, but we are in library code, so we should
> > not rely on that.
> >
> > Helped-by: Jeff King <peff@peff.net>
> > Signed-off-by: Stefan Beller <sbeller@google.com>
> > ---
> 
> Thanks.  I think this turned out to be the trickiest one to get
> right among the four and my reading of this round tells me that
> it addresses all the trickiness pointed out in the reviews.

You didn't see my response to the other patch yet. :)

> >  	/* write pack */
> >  	if (write_pack_data(bundle_fd, &revs))
> > -		return -1;
> > +		goto err;

This does still suffer from the double-close I mentioned earlier. I'm not sure if we want to address that or not.

-Peff