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

From: Jeff King <peff@peff.net>
Date: May 29, 2026, 05:40 UTC
Message-ID: <20260529054024.GA1104383@coredump.intra.peff.net>
In-Reply-To: <20260529053659.GC1099450@coredump.intra.peff.net>

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

---

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 */
 	}