patchhttp: fix memory leak in fetch_and_setup_pack_index()
13 messages between May 19, 2026 and Jun 2, 2026, from LorenzoPegorari, Jeff King, Lorenzo Pegorari.
Plain Markdown or JSON for tools and agents. Diffs are folded; open one to read it.
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(-)
Show changes to http.c +5 −3
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()
On Tue, May 19, 2026 at 04:54:45PM +0200, LorenzoPegorari wrote:
Show 8 quoted lines
> 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.
Show 5 quoted lines
> 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.
Show 6 quoted lines
> 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()
On Tue, May 19, 2026 at 03:17:43PM -0400, Jeff King wrote:
Show 27 quoted lines
> 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.
Show 40 quoted lines
> > 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
[PATCH v2] http: fix memory leak in fetch_and_setup_pack_index()
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(-)
Show changes to http.c +5 −5
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
Re: [PATCH] http: fix memory leak in fetch_and_setup_pack_index()
On Thu, May 28, 2026 at 03:22:49AM +0200, Lorenzo Pegorari wrote:
Show 21 quoted lines
> > 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
Re: [PATCH v2] http: fix memory leak in fetch_and_setup_pack_index()
On Fri, May 29, 2026 at 01:49:44AM +0200, LorenzoPegorari wrote:
Show 15 quoted lines
> 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
Re: [PATCH v2] http: fix memory leak in fetch_and_setup_pack_index()
On Fri, May 29, 2026 at 01:36:59AM -0400, Jeff King wrote:
Show 10 quoted lines
> 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
---
Show changes to http.c +10 −10
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 */
}
Re: [PATCH v2] http: fix memory leak in fetch_and_setup_pack_index()
On Fri, May 29, 2026 at 01:36:59AM -0400, Jeff King wrote:
Show 38 quoted lines
> 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
Re: [PATCH v2] http: fix memory leak in fetch_and_setup_pack_index()
On Fri, May 29, 2026 at 01:40:24AM -0400, Jeff King wrote:
Show 91 quoted lines
> 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
[PATCH v3 0/2] http: fix memory leak in fetch_and_setup_pack_index()
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
[PATCH v3 1/2] http: cleanup function fetch_and_setup_pack_index()
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(-)
Show changes to http.c +0 −2
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
[PATCH v3 2/2] http: fix memory leak in fetch_and_setup_pack_index()
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(-)
Show changes to http.c +5 −3
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
Re: [PATCH v3 0/2] http: fix memory leak in fetch_and_setup_pack_index()
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