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

[Lorenzo Pegorari <lorenzo.pegorari2002@gmail.com>]

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