[PATCH] http: fix memory leak in fetch_and_setup_pack_index()

[PATCH] http: fix memory leak in fetch_and_setup_pack_index()

[LorenzoPegorari]

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.

Signed-off-by: LorenzoPegorari <lorenzo.pegorari2002@gmail.com>
---
 http.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/http.c b/http.c
index 67c9c6fc60..c28be26ad9 100644
--- a/http.c
+++ b/http.c
@@ -2545,11 +2545,13 @@ static int fetch_and_setup_pack_index(struct packfile_list *packs,
 	}
 
 	ret = verify_pack_index(new_pack);
-	if (!ret)
-		close_pack_index(new_pack);
+
+	close_pack_index(new_pack);
 	free(tmp_idx);
-	if (ret)
+	if (ret) {
+		free(new_pack);
 		return -1;
+	}
 
 	packfile_list_prepend(packs, new_pack);
 	return 0;
-- 
2.54.0.dirty

Re: [PATCH] http: fix memory leak in fetch_and_setup_pack_index()

[Jeff King]

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.

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

-Peff