Git development
 help / color / mirror / Atom feed
* [PATCH] http: fix memory leak in fetch_and_setup_pack_index()
@ 2026-05-19 14:54 LorenzoPegorari
  2026-05-19 19:17 ` Jeff King
  2026-05-28 23:49 ` [PATCH v2] " LorenzoPegorari
  0 siblings, 2 replies; 13+ messages in thread
From: LorenzoPegorari @ 2026-05-19 14:54 UTC (permalink / raw)
  To: git; +Cc: Taylor Blau, Junio C Hamano, Patrick Steinhardt, fox, Jeff King

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


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH] http: fix memory leak in fetch_and_setup_pack_index()
  2026-05-19 14:54 [PATCH] http: fix memory leak in fetch_and_setup_pack_index() LorenzoPegorari
@ 2026-05-19 19:17 ` Jeff King
  2026-05-28  1:22   ` Lorenzo Pegorari
  2026-05-28 23:49 ` [PATCH v2] " LorenzoPegorari
  1 sibling, 1 reply; 13+ messages in thread
From: Jeff King @ 2026-05-19 19:17 UTC (permalink / raw)
  To: LorenzoPegorari; +Cc: git, Taylor Blau, Junio C Hamano, Patrick Steinhardt, fox

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] http: fix memory leak in fetch_and_setup_pack_index()
  2026-05-19 19:17 ` Jeff King
@ 2026-05-28  1:22   ` Lorenzo Pegorari
  2026-05-29  5:32     ` Jeff King
  0 siblings, 1 reply; 13+ messages in thread
From: Lorenzo Pegorari @ 2026-05-28  1:22 UTC (permalink / raw)
  To: Jeff King; +Cc: git, Taylor Blau, Junio C Hamano, Patrick Steinhardt, fox

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v2] http: fix memory leak in fetch_and_setup_pack_index()
  2026-05-19 14:54 [PATCH] http: fix memory leak in fetch_and_setup_pack_index() LorenzoPegorari
  2026-05-19 19:17 ` Jeff King
@ 2026-05-28 23:49 ` LorenzoPegorari
  2026-05-29  5:36   ` Jeff King
  2026-06-01 13:51   ` [PATCH v3 0/2] " LorenzoPegorari
  1 sibling, 2 replies; 13+ messages in thread
From: LorenzoPegorari @ 2026-05-28 23:49 UTC (permalink / raw)
  To: git; +Cc: Taylor Blau, Junio C Hamano, Patrick Steinhardt, fox, Jeff King

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.

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

diff --git a/http.c b/http.c
index 67c9c6fc60..99da4d7529 100644
--- a/http.c
+++ b/http.c
@@ -2538,18 +2538,18 @@ static int fetch_and_setup_pack_index(struct packfile_list *packs,
 
 	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 */
 	}
 
 	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.129.g2dffd77b94.dirty


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH] http: fix memory leak in fetch_and_setup_pack_index()
  2026-05-28  1:22   ` Lorenzo Pegorari
@ 2026-05-29  5:32     ` Jeff King
  0 siblings, 0 replies; 13+ messages in thread
From: Jeff King @ 2026-05-29  5:32 UTC (permalink / raw)
  To: Lorenzo Pegorari
  Cc: git, Taylor Blau, Junio C Hamano, Patrick Steinhardt, fox

On Thu, May 28, 2026 at 03:22:49AM +0200, Lorenzo Pegorari wrote:

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

I think the only behavior difference in removing the unlink() is whether
we immediately delete the downloaded packfile on error, or if we wait
until process exit. From the perspective of somebody calling git-fetch,
the outcome is roughly the same (when the process ends, the file is
gone). It would only differ if the system crashed before the process
ended.

-Peff

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] http: fix memory leak in fetch_and_setup_pack_index()
  2026-05-28 23:49 ` [PATCH v2] " LorenzoPegorari
@ 2026-05-29  5:36   ` Jeff King
  2026-05-29  5:40     ` Jeff King
  2026-06-01 13:27     ` Lorenzo Pegorari
  2026-06-01 13:51   ` [PATCH v3 0/2] " LorenzoPegorari
  1 sibling, 2 replies; 13+ messages in thread
From: Jeff King @ 2026-05-29  5:36 UTC (permalink / raw)
  To: LorenzoPegorari; +Cc: git, Taylor Blau, Junio C Hamano, Patrick Steinhardt, fox

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] http: fix memory leak in fetch_and_setup_pack_index()
  2026-05-29  5:36   ` Jeff King
@ 2026-05-29  5:40     ` Jeff King
  2026-06-01 13:34       ` Lorenzo Pegorari
  2026-06-01 13:27     ` Lorenzo Pegorari
  1 sibling, 1 reply; 13+ messages in thread
From: Jeff King @ 2026-05-29  5:40 UTC (permalink / raw)
  To: LorenzoPegorari; +Cc: git, Taylor Blau, Junio C Hamano, Patrick Steinhardt, fox

On Fri, May 29, 2026 at 01:36:59AM -0400, Jeff King wrote:

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

BTW, for (2) I wondered about going in the opposite direction. If we
actually passed the tempfile back up, like in the patch below, then we
could use delete_tempfile() to do the unlink (and remove it from the
tempfile list).

And then your patch would want to similarly delete_tempfile() in its
error path.

But I don't think it really buys us much. _If_ we were going to keep
passing the tempfile struct up the call stack on success, then we could
store it and call delete_tempfile() as soon as we had ran index-pack on
it. But that's even more surgery, for again little gain (we delete our
tempfiles a little earlier, rather than at process end).

So I'm inclined to go in the direction that shortens the code. ;)

-Peff

---
diff --git a/http.c b/http.c
index ea9b16861b..e83a3857b3 100644
--- a/http.c
+++ b/http.c
@@ -2546,9 +2546,10 @@ int http_fetch_ref(const char *base, struct ref *ref)
 }
 
 /* Helpers for fetching packs */
-static char *fetch_pack_index(unsigned char *hash, const char *base_url)
+static struct tempfile *fetch_pack_index(unsigned char *hash, const char *base_url)
 {
 	char *url, *tmp;
+	struct tempfile *ret;
 	struct strbuf buf = STRBUF_INIT;
 
 	if (http_is_verbose)
@@ -2575,23 +2576,24 @@ static char *fetch_pack_index(unsigned char *hash, const char *base_url)
 	tmp = xstrfmt("%s/tmp_pack_%s.idx",
 		      repo_get_object_directory(the_repository),
 		      hash_to_hex(hash));
-	register_tempfile(tmp);
+	ret = register_tempfile(tmp);
+	free(tmp);
 
-	if (http_get_file(url, tmp, NULL) != HTTP_OK) {
+	if (http_get_file(url, ret->filename.buf, NULL) != HTTP_OK) {
 		error("Unable to get pack index %s", url);
-		FREE_AND_NULL(tmp);
+		delete_tempfile(&ret);
 	}
 
 	free(url);
-	return tmp;
+	return ret;
 }
 
 static int fetch_and_setup_pack_index(struct packfile_list *packs,
 				      unsigned char *sha1,
 				      const char *base_url)
 {
 	struct packed_git *new_pack, *p;
-	char *tmp_idx = NULL;
+	struct tempfile *tmp_idx;
 	int ret;
 
 	/*
@@ -2607,11 +2609,9 @@ static int fetch_and_setup_pack_index(struct packfile_list *packs,
 	if (!tmp_idx)
 		return -1;
 
-	new_pack = parse_pack_index(the_repository, sha1, tmp_idx);
+	new_pack = parse_pack_index(the_repository, sha1, tmp_idx->filename.buf);
 	if (!new_pack) {
-		unlink(tmp_idx);
-		free(tmp_idx);
-
+		delete_tempfile(&tmp_idx);
 		return -1; /* parse_pack_index() already issued error message */
 	}
 

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] http: fix memory leak in fetch_and_setup_pack_index()
  2026-05-29  5:36   ` Jeff King
  2026-05-29  5:40     ` Jeff King
@ 2026-06-01 13:27     ` Lorenzo Pegorari
  1 sibling, 0 replies; 13+ messages in thread
From: Lorenzo Pegorari @ 2026-06-01 13:27 UTC (permalink / raw)
  To: Jeff King; +Cc: git, Taylor Blau, Junio C Hamano, Patrick Steinhardt, fox

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] http: fix memory leak in fetch_and_setup_pack_index()
  2026-05-29  5:40     ` Jeff King
@ 2026-06-01 13:34       ` Lorenzo Pegorari
  0 siblings, 0 replies; 13+ messages in thread
From: Lorenzo Pegorari @ 2026-06-01 13:34 UTC (permalink / raw)
  To: Jeff King; +Cc: git, Taylor Blau, Junio C Hamano, Patrick Steinhardt, fox

On Fri, May 29, 2026 at 01:40:24AM -0400, Jeff King wrote:
> On Fri, May 29, 2026 at 01:36:59AM -0400, Jeff King wrote:
> 
> > 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).
> 
> BTW, for (2) I wondered about going in the opposite direction. If we
> actually passed the tempfile back up, like in the patch below, then we
> could use delete_tempfile() to do the unlink (and remove it from the
> tempfile list).
> 
> And then your patch would want to similarly delete_tempfile() in its
> error path.
> 
> But I don't think it really buys us much. _If_ we were going to keep
> passing the tempfile struct up the call stack on success, then we could
> store it and call delete_tempfile() as soon as we had ran index-pack on
> it. But that's even more surgery, for again little gain (we delete our
> tempfiles a little earlier, rather than at process end).
> 
> So I'm inclined to go in the direction that shortens the code. ;)
> 
> -Peff
> 
> ---
> diff --git a/http.c b/http.c
> index ea9b16861b..e83a3857b3 100644
> --- a/http.c
> +++ b/http.c
> @@ -2546,9 +2546,10 @@ int http_fetch_ref(const char *base, struct ref *ref)
>  }
>  
>  /* Helpers for fetching packs */
> -static char *fetch_pack_index(unsigned char *hash, const char *base_url)
> +static struct tempfile *fetch_pack_index(unsigned char *hash, const char *base_url)
>  {
>  	char *url, *tmp;
> +	struct tempfile *ret;
>  	struct strbuf buf = STRBUF_INIT;
>  
>  	if (http_is_verbose)
> @@ -2575,23 +2576,24 @@ static char *fetch_pack_index(unsigned char *hash, const char *base_url)
>  	tmp = xstrfmt("%s/tmp_pack_%s.idx",
>  		      repo_get_object_directory(the_repository),
>  		      hash_to_hex(hash));
> -	register_tempfile(tmp);
> +	ret = register_tempfile(tmp);
> +	free(tmp);
>  
> -	if (http_get_file(url, tmp, NULL) != HTTP_OK) {
> +	if (http_get_file(url, ret->filename.buf, NULL) != HTTP_OK) {
>  		error("Unable to get pack index %s", url);
> -		FREE_AND_NULL(tmp);
> +		delete_tempfile(&ret);
>  	}
>  
>  	free(url);
> -	return tmp;
> +	return ret;
>  }
>  
>  static int fetch_and_setup_pack_index(struct packfile_list *packs,
>  				      unsigned char *sha1,
>  				      const char *base_url)
>  {
>  	struct packed_git *new_pack, *p;
> -	char *tmp_idx = NULL;
> +	struct tempfile *tmp_idx;
>  	int ret;
>  
>  	/*
> @@ -2607,11 +2609,9 @@ static int fetch_and_setup_pack_index(struct packfile_list *packs,
>  	if (!tmp_idx)
>  		return -1;
>  
> -	new_pack = parse_pack_index(the_repository, sha1, tmp_idx);
> +	new_pack = parse_pack_index(the_repository, sha1, tmp_idx->filename.buf);
>  	if (!new_pack) {
> -		unlink(tmp_idx);
> -		free(tmp_idx);
> -
> +		delete_tempfile(&tmp_idx);
>  		return -1; /* parse_pack_index() already issued error message */
>  	}

Yeah, I also explored the possibility (as you suggested in your first
reply to v1) of manually deleting the tempfile. In my opinion, this
isn't worth the effort, and it's complicating the code for no reason, so
in the end I opted for keeping it as simple and minimal as possible.

Thanks,

Lorenzo


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v3 0/2] http: fix memory leak in fetch_and_setup_pack_index()
  2026-05-28 23:49 ` [PATCH v2] " LorenzoPegorari
  2026-05-29  5:36   ` Jeff King
@ 2026-06-01 13:51   ` LorenzoPegorari
  2026-06-01 13:52     ` [PATCH v3 1/2] http: cleanup function fetch_and_setup_pack_index() LorenzoPegorari
                       ` (2 more replies)
  1 sibling, 3 replies; 13+ messages in thread
From: LorenzoPegorari @ 2026-06-01 13:51 UTC (permalink / raw)
  To: git; +Cc: Taylor Blau, Junio C Hamano, Patrick Steinhardt, fox, Jeff King

Patch series that does some cleanup and fixes a memory leak present
inside the function `fetch_and_setup_pack_index()`.

LorenzoPegorari (2):
  http: cleanup function fetch_and_setup_pack_index()
  http: fix memory leak in fetch_and_setup_pack_index()

 http.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

-- 
2.54.0.129.g2dffd77b94.dirty


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v3 1/2] http: cleanup function fetch_and_setup_pack_index()
  2026-06-01 13:51   ` [PATCH v3 0/2] " LorenzoPegorari
@ 2026-06-01 13:52     ` LorenzoPegorari
  2026-06-01 13:52     ` [PATCH v3 2/2] http: fix memory leak in fetch_and_setup_pack_index() LorenzoPegorari
  2026-06-02  6:24     ` [PATCH v3 0/2] " Jeff King
  2 siblings, 0 replies; 13+ messages in thread
From: LorenzoPegorari @ 2026-06-01 13:52 UTC (permalink / raw)
  To: git; +Cc: Taylor Blau, Junio C Hamano, Patrick Steinhardt, fox, Jeff King

Cleanup the function `fetch_and_setup_pack_index()` 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.

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

diff --git a/http.c b/http.c
index 67c9c6fc60..b8443b1ef4 100644
--- a/http.c
+++ b/http.c
@@ -2538,9 +2538,7 @@ static int fetch_and_setup_pack_index(struct packfile_list *packs,
 
 	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 */
 	}
 
-- 
2.54.0.129.g2dffd77b94.dirty


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v3 2/2] http: fix memory leak in fetch_and_setup_pack_index()
  2026-06-01 13:51   ` [PATCH v3 0/2] " LorenzoPegorari
  2026-06-01 13:52     ` [PATCH v3 1/2] http: cleanup function fetch_and_setup_pack_index() LorenzoPegorari
@ 2026-06-01 13:52     ` LorenzoPegorari
  2026-06-02  6:24     ` [PATCH v3 0/2] " Jeff King
  2 siblings, 0 replies; 13+ messages in thread
From: LorenzoPegorari @ 2026-06-01 13:52 UTC (permalink / raw)
  To: git; +Cc: Taylor Blau, Junio C Hamano, Patrick Steinhardt, fox, Jeff King

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.

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 b8443b1ef4..99da4d7529 100644
--- a/http.c
+++ b/http.c
@@ -2543,11 +2543,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.129.g2dffd77b94.dirty


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 0/2] http: fix memory leak in fetch_and_setup_pack_index()
  2026-06-01 13:51   ` [PATCH v3 0/2] " LorenzoPegorari
  2026-06-01 13:52     ` [PATCH v3 1/2] http: cleanup function fetch_and_setup_pack_index() LorenzoPegorari
  2026-06-01 13:52     ` [PATCH v3 2/2] http: fix memory leak in fetch_and_setup_pack_index() LorenzoPegorari
@ 2026-06-02  6:24     ` Jeff King
  2 siblings, 0 replies; 13+ messages in thread
From: Jeff King @ 2026-06-02  6:24 UTC (permalink / raw)
  To: LorenzoPegorari; +Cc: git, Taylor Blau, Junio C Hamano, Patrick Steinhardt, fox

On Mon, Jun 01, 2026 at 03:51:43PM +0200, LorenzoPegorari wrote:

> Patch series that does some cleanup and fixes a memory leak present
> inside the function `fetch_and_setup_pack_index()`.

Thanks, this version looks great to me.

-Peff

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2026-06-02  6:24 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-05-19 14:54 [PATCH] http: fix memory leak in fetch_and_setup_pack_index() LorenzoPegorari
2026-05-19 19:17 ` Jeff King
2026-05-28  1:22   ` Lorenzo Pegorari
2026-05-29  5:32     ` Jeff King
2026-05-28 23:49 ` [PATCH v2] " LorenzoPegorari
2026-05-29  5:36   ` Jeff King
2026-05-29  5:40     ` Jeff King
2026-06-01 13:34       ` Lorenzo Pegorari
2026-06-01 13:27     ` Lorenzo Pegorari
2026-06-01 13:51   ` [PATCH v3 0/2] " LorenzoPegorari
2026-06-01 13:52     ` [PATCH v3 1/2] http: cleanup function fetch_and_setup_pack_index() LorenzoPegorari
2026-06-01 13:52     ` [PATCH v3 2/2] http: fix memory leak in fetch_and_setup_pack_index() LorenzoPegorari
2026-06-02  6:24     ` [PATCH v3 0/2] " Jeff King

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox