[PATCH] builtin/fetch-pack: cleanup before return error

[PATCH] builtin/fetch-pack: cleanup before return error

[Lidong Yan via GitGitGadget]

From: Lidong Yan <502024330056@smail.nju.edu.cn>

In builtin/fetch-pack.c:cmd_fetch_pack(), if finish_connect() failed,
it returns error code without cleanup which cause memory leak. Add
cleanup label before frees in the end of cmd_fetch_pack(), and add
`goto cleanup` if finish_connect() failed.

Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>
---
    builtin/fetch-pack: cleanup before return error
    
    In builtin/fetch-pack.c:cmd_fetch_pack(), if finish_connect() failed, it
    returns error code without cleanup which cause memory leak. Add cleanup
    label before frees in the end of cmd_fetch_pack(), and add goto cleanup
    if finish_connect() failed.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1990%2Fbrandb97%2Ffix-cmd-fetch-pack-leak-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1990/brandb97/fix-cmd-fetch-pack-leak-v1
Pull-Request: https://github.com/git/git/pull/1990

 builtin/fetch-pack.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index d07eec9e555..d9e42bad584 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -274,8 +274,10 @@ int cmd_fetch_pack(int argc,
 	}
 	close(fd[0]);
 	close(fd[1]);
-	if (finish_connect(conn))
-		return 1;
+	if (finish_connect(conn)) {
+		ret = 1;
+		goto cleanup;
+	}
 
 	ret = !fetched_refs;
 
@@ -291,6 +293,7 @@ int cmd_fetch_pack(int argc,
 		printf("%s %s\n",
 		       oid_to_hex(&ref->old_oid), ref->name);
 
+cleanup:
 	for (size_t i = 0; i < nr_sought; i++)
 		free_one_ref(sought_to_free[i]);
 	free(sought_to_free);

base-commit: 7014b55638da979331baf8dc31c4e1d697cf2d67
-- 
gitgitgadget

Re: [PATCH] builtin/fetch-pack: cleanup before return error

[Patrick Steinhardt]

On Wed, Jun 04, 2025 at 03:07:54AM +0000, Lidong Yan via GitGitGadget wrote:
> diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
> index d07eec9e555..d9e42bad584 100644
> --- a/builtin/fetch-pack.c
> +++ b/builtin/fetch-pack.c
> @@ -274,8 +274,10 @@ int cmd_fetch_pack(int argc,
>  	}
>  	close(fd[0]);
>  	close(fd[1]);
> -	if (finish_connect(conn))
> -		return 1;
> +	if (finish_connect(conn)) {
> +		ret = 1;
> +		goto cleanup;
> +	}
>  
>  	ret = !fetched_refs;
>  
> @@ -291,6 +293,7 @@ int cmd_fetch_pack(int argc,
>  		printf("%s %s\n",
>  		       oid_to_hex(&ref->old_oid), ref->name);
>  
> +cleanup:
>  	for (size_t i = 0; i < nr_sought; i++)
>  		free_one_ref(sought_to_free[i]);
>  	free(sought_to_free);


Sensible again. My only worry is that some of the variables that get
free'd here may not have been initialized. But I went through all of
them and all of them are NULL- or zero-initialized indeed, so this looks
good to me. Thanks!

Patrick