From: Jeff King <peff@peff.net>
Date: Fri, 29 May 2026 05:36:59 GMT
Subject: Re: [PATCH v2] http: fix memory leak in fetch_and_setup_pack_index()
Message-ID: <20260529053659.GC1099450@coredump.intra.peff.net>
In-Reply-To: <ahjUmMCKxREamQE-@lorenzo-VM>

On Fri, May 29, 2026 at 01:49:44AM +0200, LorenzoPegorari wrote:

> Inside the function `fetch_and_setup_pack_index()`, when the pack
> obtained using `parse_pack_index()` fails to be verified by
> `verify_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, when the verification
> fails.
> 
> Also, do some more cleanup by removing the useless call to the function
> `unlink()`. This is not necessary anymore since 63aca3f7f1 (dumb-http:
> store downloaded pack idx as tempfile, 2024-10-25), when
> `fetch_pack_index()` started registering its return value (in this case
> `tmp_idx`) as a tempfile to be deleted at process exit.

I think the patch as-is is OK. But when I see this kind of "also, do
this..." in a commit message it is a good time to consider whether that
should happen in a separate patch.

Here it does not make sense to remove the unlink() afterwards; you'd
wonder why it was not present in the cleanup added by your patch.

But it _could_ be done as a preparatory patch. And the rationale for
doing that on its own I think is roughly:

  1. It is mostly doing nothing, because 63aca3f7f1 registered it as a
     tempfile, so it will be cleaned up at process end anyway (whether
     we succeed in fetching it or not).

  2. It is maybe a little harmful, because we are going to unlink() it
     now, and then later the tempfile code will try to unlink() it again
     (so a simultaneous fetch could have created the same file).

For something this small, though, I am OK just lumping it together.
There are diminishing returns from polishing it further.

-Peff