From: Lorenzo Pegorari <lorenzo.pegorari2002@gmail.com>
Date: Mon, 01 Jun 2026 13:27:53 GMT
Subject: Re: [PATCH v2] http: fix memory leak in fetch_and_setup_pack_index()
Message-ID: <ah2I2b345uU6LKJI@lorenzo-VM>
In-Reply-To: <20260529053659.GC1099450@coredump.intra.peff.net>

On Fri, May 29, 2026 at 01:36:59AM -0400, Jeff King wrote:
> 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.

Yeah, this makes sense. I will separate it in 2 different patches.

> -Peff

Thanks,

Lorenzo