[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

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

[Lorenzo Pegorari]

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