{"thread":{"id":"63576","subject":"[PATCH] builtin/fetch-pack: cleanup before return error","startedAt":"2025-06-04T03:07:57Z","lastAt":"2025-06-04T07:58:49Z","messageCount":2,"participants":["Lidong Yan via GitGitGadget","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"519627","messageId":"pull.1990.git.git.1749006474694.gitgitgadget@gmail.com","threadId":"63576","inReplyTo":null,"subject":"[PATCH] builtin/fetch-pack: cleanup before return error","fromName":"Lidong Yan via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-06-04T03:07:54Z","receivedAt":"2025-06-04T03:07:57Z","isPatch":true,"sender":{"key":"yldhome2d2@gmail.com","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"From: Lidong Yan <502024330056@smail.nju.edu.cn>\n\nIn builtin/fetch-pack.c:cmd_fetch_pack(), if finish_connect() failed,\nit returns error code without cleanup which cause memory leak. Add\ncleanup label before frees in the end of cmd_fetch_pack(), and add\n`goto cleanup` if finish_connect() failed.\n\nSigned-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>\n---\n    builtin/fetch-pack: cleanup before return error\n    \n    In builtin/fetch-pack.c:cmd_fetch_pack(), if finish_connect() failed, it\n    returns error code without cleanup which cause memory leak. Add cleanup\n    label before frees in the end of cmd_fetch_pack(), and add goto cleanup\n    if finish_connect() failed.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1990%2Fbrandb97%2Ffix-cmd-fetch-pack-leak-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1990/brandb97/fix-cmd-fetch-pack-leak-v1\nPull-Request: https://github.com/git/git/pull/1990\n\n builtin/fetch-pack.c | 7 +++++--\n 1 file changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex d07eec9e555..d9e42bad584 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -274,8 +274,10 @@ int cmd_fetch_pack(int argc,\n \t}\n \tclose(fd[0]);\n \tclose(fd[1]);\n-\tif (finish_connect(conn))\n-\t\treturn 1;\n+\tif (finish_connect(conn)) {\n+\t\tret = 1;\n+\t\tgoto cleanup;\n+\t}\n \n \tret = !fetched_refs;\n \n@@ -291,6 +293,7 @@ int cmd_fetch_pack(int argc,\n \t\tprintf(\"%s %s\\n\",\n \t\t       oid_to_hex(&ref->old_oid), ref->name);\n \n+cleanup:\n \tfor (size_t i = 0; i < nr_sought; i++)\n \t\tfree_one_ref(sought_to_free[i]);\n \tfree(sought_to_free);\n\nbase-commit: 7014b55638da979331baf8dc31c4e1d697cf2d67\n-- \ngitgitgadget\n"},{"id":"519641","messageId":"aD_8tafSPGhtdQI0@pks.im","threadId":"63576","inReplyTo":"pull.1990.git.git.1749006474694.gitgitgadget@gmail.com","subject":"Re: [PATCH] builtin/fetch-pack: cleanup before return error","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-06-04T07:58:45Z","receivedAt":"2025-06-04T07:58:49Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Jun 04, 2025 at 03:07:54AM +0000, Lidong Yan via GitGitGadget wrote:\n> diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\n> index d07eec9e555..d9e42bad584 100644\n> --- a/builtin/fetch-pack.c\n> +++ b/builtin/fetch-pack.c\n> @@ -274,8 +274,10 @@ int cmd_fetch_pack(int argc,\n>  \t}\n>  \tclose(fd[0]);\n>  \tclose(fd[1]);\n> -\tif (finish_connect(conn))\n> -\t\treturn 1;\n> +\tif (finish_connect(conn)) {\n> +\t\tret = 1;\n> +\t\tgoto cleanup;\n> +\t}\n>  \n>  \tret = !fetched_refs;\n>  \n> @@ -291,6 +293,7 @@ int cmd_fetch_pack(int argc,\n>  \t\tprintf(\"%s %s\\n\",\n>  \t\t       oid_to_hex(&ref->old_oid), ref->name);\n>  \n> +cleanup:\n>  \tfor (size_t i = 0; i < nr_sought; i++)\n>  \t\tfree_one_ref(sought_to_free[i]);\n>  \tfree(sought_to_free);\n\n\nSensible again. My only worry is that some of the variables that get\nfree'd here may not have been initialized. But I went through all of\nthem and all of them are NULL- or zero-initialized indeed, so this looks\ngood to me. Thanks!\n\nPatrick\n"}]}