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