From: Lorenzo Pegorari <lorenzo.pegorari2002@gmail.com>
Date: Thu, 28 May 2026 01:22:49 GMT
Subject: Re: [PATCH] http: fix memory leak in fetch_and_setup_pack_index()
Message-ID: <aheY6bLM2gxtMDdr@lorenzo-VM>
In-Reply-To: <20260519191743.GA2269222@coredump.intra.peff.net>

On Tue, May 19, 2026 at 03:17:43PM -0400, Jeff King wrote:
> On Tue, May 19, 2026 at 04:54:45PM +0200, LorenzoPegorari wrote:
> 
> > Inside the function `fetch_and_setup_pack_index()`, when the pack
> > obtained using `fetch_pack_index()` fails to be verified by
> > `parse_pack_index()`, the function returns without closing and freeing
> > said pack.
> > 
> > Fix this by calling `close_pack_index()` to munmap the index file for
> > the leaking pack (which might have been mmapped by `fetch_pack_index()`
> > or `verify_pack_index()`), and then free it.
> 
> OK, I agree we are leaking here, but after reading the patch I'm left
> with a few questions.
> 
> >  	ret = verify_pack_index(new_pack);
> > -	if (!ret)
> > -		close_pack_index(new_pack);
> > +
> > +	close_pack_index(new_pack);
> 
> This part was a little confusing at first, because it looked like we are
> already closing the index. But we were doing so on _success_, not on
> failure. Which is a little funny since the point is to be able to read
> from it later, but OK.
> 
> At any rate, that is an existing oddity, and I agree that closing it
> before freeing the struct is obviously the right thing to do.

It is indeed weird that we are closing only on success, and not on
failure.

> >  	free(tmp_idx);
> > -	if (ret)
> > +	if (ret) {
> > +		free(new_pack);
> >  		return -1;
> > +	}
> 
> And here we free the actual struct. Good.
> 
> But this existing free(tmp_idx) is what puzzles me. We do not need the
> filename anymore regardless of success or failure, so freeing it makes
> sense. But earlier in the function we have:
> 
>           new_pack = parse_pack_index(the_repository, sha1, tmp_idx);
>           if (!new_pack) {
>                   unlink(tmp_idx);
>                   free(tmp_idx);
>   
>                   return -1; /* parse_pack_index() already issued error message */
>           }
> 
> So on parse failure we actually unlink it, but not on verification
> failure. Which seems like it would leave cruft after the process ends.
> And I suspect we probably we did prior to 63aca3f7f1 (dumb-http: store
> downloaded pack idx as tempfile, 2024-10-25), when we started
> registering it as a tempfile to be deleted at process exit.
> 
> So I _think_ we could get away with dropping the existing unlink() call
> and just let it get cleaned up at process exit. But if we are going to
> keep it, do we want to also unlink() in this error path? At which point
> it might make more sense to have an "out" label to consolidate all of
> this cleanup.
> 
> If we are going to unlink() here it may also make sense to just return
> the tempfile struct from fetch_pack_index(), and then we can call
> delete_tempfile() on it. See the in-code comment in 63aca3f7f1 which
> mentions this hackery.
> 
> So I dunno. I think your patch is doing the right thing as-is, but it
> may be worth taking a moment to clean this up a bit further.

The `unlink()` indeed is weird. Pointing me to the commit 63aca3f7f1
really helped me understand how the code changed and the current
situation. Thanks a lot for that.

I've tried testing as thoroughly as possible whether removing the
`unlink()` function call wouldn't change the expected behavior.
*I think* that it can be removed safely, but I'm not 100% sure yet.

If this is the case, I think adding a `goto` "cleanup label" is not
necessary.

> -Peff

Thank you so much Peff for going through this patch,

Lorenzo