{"thread":{"id":"63457","subject":"[PATCH] sequencer: fix memory leak if `todo_list_rearrange_squash()` failed","startedAt":"2025-05-14T13:53:31Z","lastAt":"2025-05-15T16:37:17Z","messageCount":4,"participants":["Lidong Yan via GitGitGadget","Phillip Wood","lidongyan","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"518039","messageId":"pull.1965.git.git.1747230808770.gitgitgadget@gmail.com","threadId":"63457","inReplyTo":null,"subject":"[PATCH] sequencer: fix memory leak if `todo_list_rearrange_squash()` failed","fromName":"Lidong Yan via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-05-14T13:53:28Z","receivedAt":"2025-05-14T13:53:31Z","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 sequencer.c:todo_list_rearrange_squash, if it fails, memory\nallocated in `next`, `tail`, `subjects` and `subject2item` will leak.\nJump to cleanup label before return could fix this leak problem.\n\nSigned-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>\n---\n    sequencer: fix memory leak if todo_list_rearrange_squash() failed\n    \n    In sequencer.c:todo_list_rearrange_squash, if it fails, memory allocated\n    in next, tail, subjects and subject2item will leak. Jump to cleanup\n    label before return could fix this leak problem.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1965%2Fbrandb97%2Ffix-sequencer-todo-leak-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1965/brandb97/fix-sequencer-todo-leak-v1\nPull-Request: https://github.com/git/git/pull/1965\n\n sequencer.c | 8 +++++---\n 1 file changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex b5c4043757e..5fb7b68a7ab 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -6596,6 +6596,7 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)\n \tchar **subjects;\n \tstruct commit_todo_item commit_todo;\n \tstruct todo_item *items = NULL;\n+\tint ret = 0;\n \n \tinit_commit_todo_item(&commit_todo);\n \t/*\n@@ -6626,8 +6627,8 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)\n \t\t}\n \n \t\tif (is_fixup(item->command)) {\n-\t\t\tclear_commit_todo_item(&commit_todo);\n-\t\t\treturn error(_(\"the script was already rearranged.\"));\n+\t\t\tret = error(_(\"the script was already rearranged.\"));\n+\t\t\tgoto cleanup;\n \t\t}\n \n \t\trepo_parse_commit(the_repository, item->commit);\n@@ -6729,6 +6730,7 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)\n \t\ttodo_list->items = items;\n \t}\n \n+cleanup:\n \tfree(next);\n \tfree(tail);\n \tfor (i = 0; i < todo_list->nr; i++)\n@@ -6738,7 +6740,7 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)\n \n \tclear_commit_todo_item(&commit_todo);\n \n-\treturn 0;\n+\treturn ret;\n }\n \n int sequencer_determine_whence(struct repository *r, enum commit_whence *whence)\n\nbase-commit: 6f84262c44a89851c3ae5a6e4c1a9d06b2068d75\n-- \ngitgitgadget\n"},{"id":"518090","messageId":"52e0a717-f7f0-401d-b621-f1f6e028e595@gmail.com","threadId":"63457","inReplyTo":"pull.1965.git.git.1747230808770.gitgitgadget@gmail.com","subject":"Re: [PATCH] sequencer: fix memory leak if `todo_list_rearrange_squash()` failed","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-05-15T10:08:55Z","receivedAt":"2025-05-15T10:09:01Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 14/05/2025 14:53, Lidong Yan via GitGitGadget wrote:\n> From: Lidong Yan <502024330056@smail.nju.edu.cn>\n> \n> In sequencer.c:todo_list_rearrange_squash, if it fails, memory\n> allocated in `next`, `tail`, `subjects` and `subject2item` will leak.\n> Jump to cleanup label before return could fix this leak problem.\n\nYou could mention that you're adding the cleanup label.\n\nI suspect reaching this condition is a bug as well because we should \nonly rearrange the todo list when the rebase starts. However I'm not \n100% sure that it is impossible to trigger this condition so lets free \nit as you suggest. The code changes look good.\n\nOut of interest how are you finding these leaks?\n\nThanks\n\nPhillip\n\n> Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>\n> ---\n>      sequencer: fix memory leak if todo_list_rearrange_squash() failed\n>      \n>      In sequencer.c:todo_list_rearrange_squash, if it fails, memory allocated\n>      in next, tail, subjects and subject2item will leak. Jump to cleanup\n>      label before return could fix this leak problem.\n> \n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1965%2Fbrandb97%2Ffix-sequencer-todo-leak-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1965/brandb97/fix-sequencer-todo-leak-v1\n> Pull-Request: https://github.com/git/git/pull/1965\n> \n>   sequencer.c | 8 +++++---\n>   1 file changed, 5 insertions(+), 3 deletions(-)\n> \n> diff --git a/sequencer.c b/sequencer.c\n> index b5c4043757e..5fb7b68a7ab 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -6596,6 +6596,7 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)\n>   \tchar **subjects;\n>   \tstruct commit_todo_item commit_todo;\n>   \tstruct todo_item *items = NULL;\n> +\tint ret = 0;\n>   \n>   \tinit_commit_todo_item(&commit_todo);\n>   \t/*\n> @@ -6626,8 +6627,8 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)\n>   \t\t}\n>   \n>   \t\tif (is_fixup(item->command)) {\n> -\t\t\tclear_commit_todo_item(&commit_todo);\n> -\t\t\treturn error(_(\"the script was already rearranged.\"));\n> +\t\t\tret = error(_(\"the script was already rearranged.\"));\n> +\t\t\tgoto cleanup;\n>   \t\t}\n>   \n>   \t\trepo_parse_commit(the_repository, item->commit);\n> @@ -6729,6 +6730,7 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)\n>   \t\ttodo_list->items = items;\n>   \t}\n>   \n> +cleanup:\n>   \tfree(next);\n>   \tfree(tail);\n>   \tfor (i = 0; i < todo_list->nr; i++)\n> @@ -6738,7 +6740,7 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)\n>   \n>   \tclear_commit_todo_item(&commit_todo);\n>   \n> -\treturn 0;\n> +\treturn ret;\n>   }\n>   \n>   int sequencer_determine_whence(struct repository *r, enum commit_whence *whence)\n> \n> base-commit: 6f84262c44a89851c3ae5a6e4c1a9d06b2068d75\n\n"},{"id":"518097","messageId":"EEC15708-1EBA-45B7-BC79-980858634AE0@smail.nju.edu.cn","threadId":"63457","inReplyTo":"52e0a717-f7f0-401d-b621-f1f6e028e595@gmail.com","subject":"Re: [PATCH] sequencer: fix memory leak if `todo_list_rearrange_squash()` failed","fromName":"lidongyan","fromEmail":"502024330056@smail.nju.edu.cn","sentAt":"2025-05-15T10:45:27Z","receivedAt":"2025-05-15T10:45:51Z","isPatch":true,"sender":{"key":"502024330056@smail.nju.edu.cn","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"2025/5/15 18:08，Phillip Wood <phillip.wood123@gmail.com> 写道：\n> \n> On 14/05/2025 14:53, Lidong Yan via GitGitGadget wrote:\n>> From: Lidong Yan <502024330056@smail.nju.edu.cn>\n>> In sequencer.c:todo_list_rearrange_squash, if it fails, memory\n>> allocated in `next`, `tail`, `subjects` and `subject2item` will leak.\n>> Jump to cleanup label before return could fix this leak problem.\n> \n> You could mention that you're adding the cleanup label.\n\nUnderstood. I will pay attention to this in future patches.\n\n> Out of interest how are you finding these leaks?\n\nI am a graduate student at Nanjing University, working in \ncollaboration with Professor [Qingkai Shi](https://qingkaishi.github.io/). \nWe are currently developing a tool for statically analyzing\nmemory leaks in C programs, and I am testing this tool on Git.\nTo ensure the validity of our testing results, we are submitting\npatches to Git and seeking confirmation from you developers.\n\n"},{"id":"518149","messageId":"xmqqecwp3k5h.fsf@gitster.g","threadId":"63457","inReplyTo":"52e0a717-f7f0-401d-b621-f1f6e028e595@gmail.com","subject":"Re: [PATCH] sequencer: fix memory leak if `todo_list_rearrange_squash()` failed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-15T16:37:14Z","receivedAt":"2025-05-15T16:37:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood <phillip.wood123@gmail.com> writes:\n\n> On 14/05/2025 14:53, Lidong Yan via GitGitGadget wrote:\n>> From: Lidong Yan <502024330056@smail.nju.edu.cn>\n>> In sequencer.c:todo_list_rearrange_squash, if it fails, memory\n>> allocated in `next`, `tail`, `subjects` and `subject2item` will leak.\n>> Jump to cleanup label before return could fix this leak problem.\n>\n> You could mention that you're adding the cleanup label.\n>\n> I suspect reaching this condition is a bug as well because we should\n> only rearrange the todo list when the rebase starts. However I'm not\n> 100% sure that it is impossible to trigger this condition so lets free\n> it as you suggest. The code changes look good.\n\nI wonder if there is a way (sort of using BUG(\"\") but not as severe\nas killing the program) for us to mark \"we do not expect this code\npath to be taken\" and ask static analysis or fuzzers to disprove\nsuch assertions.\n\nThe message being an error(), without a silent return doing nothing,\nsmells like a good enough sign, at least to me, that the original\nauthor intended not to call this helper function on a list that was\nalready rearranged, so turning this into BUG(\"\") may be something we\nwould want to consider doing in the longer term.\n\n>> @@ -6626,8 +6627,8 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)\n>>   \t\t}\n>>     \t\tif (is_fixup(item->command)) {\n>> -\t\t\tclear_commit_todo_item(&commit_todo);\n>> -\t\t\treturn error(_(\"the script was already rearranged.\"));\n>> +\t\t\tret = error(_(\"the script was already rearranged.\"));\n>> +\t\t\tgoto cleanup;\n>>   \t\t}\n\nThanks.\n"}]}