{"thread":{"id":"62120","subject":"[PATCH 00/23] Memory leak fixes (pt.7)","startedAt":"2024-09-16T11:45:25Z","lastAt":"2024-09-26T11:59:06Z","messageCount":62,"participants":["Patrick Steinhardt","Justin Tobler","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":23},"messages":[{"id":"502845","messageId":"cover.1726484308.git.ps@pks.im","threadId":"62120","inReplyTo":null,"subject":"[PATCH 00/23] Memory leak fixes (pt.7)","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-16T11:45:21Z","receivedAt":"2024-09-16T11:45:25Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nas usual, whenever the previous part of memory leak fixes has landed,\nhere's the next one. This series brings us down to 80 leaking test\nsuites. The 8th part is almost ready, too, bringing us down to 48\nleaking suites. So the end is near, and I hope that our test suites are\nleak free after the 10th part has landed.\n\nThe patch series is built on top of ed155187b4 (Sync with Git 2.46.1,\n2024-09-13) with ps/leakfixes-part-6 at 46f6ca2a68 (builtin/repack: fix\nleaking keep-pack list, 2024-09-05) merged into it.\n\nThanks!\n\nPatrick\n\nPatrick Steinhardt (23):\n builtin/help: fix dangling reference to `html_path`\n builtin/help: fix leaking `html_path` when reading config multiple\n times\n git: fix leaking argv when handling builtins\n submodule: fix leaking update strategy\n builtin/submodule--helper: clear child process when not running it\n builtin/submodule--helper: fix leaking error buffer\n t/helper: fix leaking subrepo in nested submodule config helper\n builtin/submodule--helper: fix leaking remote ref on errors\n dir: fix off by one errors for ignored and untracked entries\n builtin/pull: fix leaking \"ff\" option\n diff: fix leaking orderfile option\n parse-options: free previous value of `OPTION_FILENAME`\n diffcore-order: fix leaking buffer when parsing orderfiles\n builtin/repack: fix leaking configuration\n builtin/difftool: plug several trivial memory leaks\n trace2: destroy context stored in thread-local storage\n submodule: fix leaking submodule ODB paths\n grep: fix leaking grep pattern\n promisor-remote: fix leaking partial clone filter\n builtin/maintenance: fix leaking config string\n builtin/maintenance: fix leak in `get_schedule_cmd()`\n revision: fix leaking parents when simplifying commits\n diffcore-break: fix leaking filespecs when merging broken pairs\n\n builtin/difftool.c | 6 +\n builtin/gc.c | 131 +++++++++++-------\n builtin/help.c | 16 ++-\n builtin/pull.c | 11 +-\n builtin/repack.c | 57 ++++++--\n builtin/submodule--helper.c | 26 +++-\n combine-diff.c | 3 +-\n diff.c | 7 +-\n diff.h | 2 +-\n diffcore-break.c | 4 +-\n diffcore-order.c | 19 +--\n dir.c | 6 +-\n git.c | 22 ++-\n grep.c | 2 +-\n parse-options.c | 22 +--\n promisor-remote.c | 2 +\n revision.c | 5 +\n submodule-config.c | 2 +-\n submodule.c | 9 +-\n submodule.h | 6 +-\n t/helper/test-submodule-nested-repo-config.c | 2 +-\n t/t0012-help.sh | 1 +\n t/t1414-reflog-walk.sh | 1 +\n ...common-prefixes-and-directory-traversal.sh | 1 +\n t/t4008-diff-break-rewrite.sh | 2 +\n t/t4022-diff-rewrite.sh | 1 +\n t/t4023-diff-rename-typechange.sh | 1 +\n t/t4031-diff-rewrite-binary.sh | 1 +\n t/t4056-diff-order.sh | 1 +\n t/t4204-patch-id.sh | 1 +\n t/t5310-pack-bitmaps.sh | 1 +\n t/t5326-multi-pack-bitmaps.sh | 2 +\n t/t5329-pack-objects-cruft.sh | 2 +\n t/t6004-rev-list-path-optim.sh | 1 +\n t/t6019-rev-list-ancestry-path.sh | 1 +\n t/t6111-rev-list-treesame.sh | 1 +\n t/t7061-wtstatus-ignore.sh | 1 +\n t/t7406-submodule-update.sh | 1 +\n t/t7407-submodule-foreach.sh | 1 +\n t/t7408-submodule-reference.sh | 2 +\n t/t7411-submodule-config.sh | 1 +\n t/t7420-submodule-set-url.sh | 1 +\n t/t7521-ignored-mode.sh | 1 +\n t/t7524-commit-summary.sh | 2 +\n t/t7601-merge-pull-config.sh | 1 +\n t/t7700-repack.sh | 1 +\n t/t7800-difftool.sh | 1 +\n t/t7814-grep-recurse-submodules.sh | 1 +\n t/t7900-maintenance.sh | 1 +\n trace2/tr2_tls.c | 10 +-\n 50 files changed, 279 insertions(+), 124 deletions(-)\n\n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"502846","messageId":"e3bed973afacaec801cff1e77aeea6050cb34f57.1726484308.git.ps@pks.im","threadId":"62120","inReplyTo":"cover.1726484308.git.ps@pks.im","subject":"[PATCH 01/23] builtin/help: fix dangling reference to `html_path`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-16T11:45:24Z","receivedAt":"2024-09-16T11:45:26Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In `get_html_page_path()` we may end up assigning the return value of\n`system_path()` to the global `html_path` variable. But as we also\nassign the returned value to `to_free`, we will deallocate its memory\nupon returning from the function. Consequently, `html_path` will now\npoint to deallocated memory.\n\nFix this issue by instead assigning the value to a separate local\nvariable.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/help.c | 13 +++++++------\n 1 file changed, 7 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/help.c b/builtin/help.c\nindex dc1fbe2b986..282ea5721fa 100644\n--- a/builtin/help.c\n+++ b/builtin/help.c\n@@ -513,23 +513,24 @@ static void show_info_page(const char *page)\n static void get_html_page_path(struct strbuf *page_path, const char *page)\n {\n \tstruct stat st;\n+\tconst char *path = html_path;\n \tchar *to_free = NULL;\n \n-\tif (!html_path)\n-\t\thtml_path = to_free = system_path(GIT_HTML_PATH);\n+\tif (!path)\n+\t\tpath = to_free = system_path(GIT_HTML_PATH);\n \n \t/*\n \t * Check that the page we're looking for exists.\n \t */\n-\tif (!strstr(html_path, \"://\")) {\n-\t\tif (stat(mkpath(\"%s/%s.html\", html_path, page), &st)\n+\tif (!strstr(path, \"://\")) {\n+\t\tif (stat(mkpath(\"%s/%s.html\", path, page), &st)\n \t\t || !S_ISREG(st.st_mode))\n \t\t\tdie(\"'%s/%s.html': documentation file not found.\",\n-\t\t\t\thtml_path, page);\n+\t\t\t\tpath, page);\n \t}\n \n \tstrbuf_init(page_path, 0);\n-\tstrbuf_addf(page_path, \"%s/%s.html\", html_path, page);\n+\tstrbuf_addf(page_path, \"%s/%s.html\", path, page);\n \tfree(to_free);\n }\n \n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"502847","messageId":"4a59fe15ae30548164d1433d1d96fb2726512cb2.1726484308.git.ps@pks.im","threadId":"62120","inReplyTo":"cover.1726484308.git.ps@pks.im","subject":"[PATCH 02/23] builtin/help: fix leaking `html_path` when reading config multiple times","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-16T11:45:27Z","receivedAt":"2024-09-16T11:45:28Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `html_path` variable gets populated via `git_help_config()`, which\nputs an allocated string into it if its value has been configured. We do\nnot clear the old value though, which causes a memory leak in case the\nconfig exists multiple times.\n\nPlug this leak. The leak is exposed by t0012, but plugging it alone is\nnot sufficient to make the test suite pass.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/help.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/help.c b/builtin/help.c\nindex 282ea5721fa..2c249cbca4a 100644\n--- a/builtin/help.c\n+++ b/builtin/help.c\n@@ -52,7 +52,7 @@ static enum help_action {\n \tHELP_ACTION_CONFIG_SECTIONS_FOR_COMPLETION,\n } cmd_mode;\n \n-static const char *html_path;\n+static char *html_path;\n static int verbose = 1;\n static enum help_format help_format = HELP_FORMAT_NONE;\n static int exclude_guides;\n@@ -409,6 +409,7 @@ static int git_help_config(const char *var, const char *value,\n \tif (!strcmp(var, \"help.htmlpath\")) {\n \t\tif (!value)\n \t\t\treturn config_error_nonbool(var);\n+\t\tfree(html_path);\n \t\thtml_path = xstrdup(value);\n \t\treturn 0;\n \t}\n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"502848","messageId":"ea3dd851add1eeb5d53063847b73480b5f6245ed.1726484308.git.ps@pks.im","threadId":"62120","inReplyTo":"cover.1726484308.git.ps@pks.im","subject":"[PATCH 03/23] git: fix leaking argv when handling builtins","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-16T11:45:29Z","receivedAt":"2024-09-16T11:45:31Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In `handle_builtin()` we may end up creating an ad-hoc argv array in\ncase we see that the command line contains the \"--help\" parameter. In\nthis case we observe two memory leaks though:\n\n - We leak the `struct strvec` itself because we directly exit after\n calling `run_builtin()`, without bothering about any cleanups.\n\n - Even if we free'd that vector we'd end up leaking some of its\n strings because `run_builtin()` will modify the array.\n\nPlug both of these leaks.\n\nSigned-off-by: Patrick Steinhardt \n---\n git.c | 22 +++++++++++++++++++---\n t/t0012-help.sh | 1 +\n 2 files changed, 20 insertions(+), 3 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex 9a618a2740f..3c7fabfda26 100644\n--- a/git.c\n+++ b/git.c\n@@ -711,6 +711,7 @@ static void strip_extension(const char **argv)\n static void handle_builtin(int argc, const char **argv)\n {\n \tstruct strvec args = STRVEC_INIT;\n+\tconst char **argv_copy = NULL;\n \tconst char *cmd;\n \tstruct cmd_struct *builtin;\n \n@@ -731,13 +732,28 @@ static void handle_builtin(int argc, const char **argv)\n \t\t}\n \n \t\targc++;\n-\t\targv = args.v;\n+\n+\t\t/*\n+\t\t * `run_builtin()` will modify the argv array, so we need to\n+\t\t * create a shallow copy such that we can free all of its\n+\t\t * strings.\n+\t\t */\n+\t\tCALLOC_ARRAY(argv_copy, argc + 1);\n+\t\tCOPY_ARRAY(argv_copy, args.v, argc);\n+\n+\t\targv = argv_copy;\n \t}\n \n \tbuiltin = get_builtin(cmd);\n-\tif (builtin)\n-\t\texit(run_builtin(builtin, argc, argv));\n+\tif (builtin) {\n+\t\tint ret = run_builtin(builtin, argc, argv);\n+\t\tstrvec_clear(&args);\n+\t\tfree(argv_copy);\n+\t\texit(ret);\n+\t}\n+\n \tstrvec_clear(&args);\n+\tfree(argv_copy);\n }\n \n static void execv_dashed_external(const char **argv)\ndiff --git a/t/t0012-help.sh b/t/t0012-help.sh\nindex 1d273d91c21..9eae0d83563 100755\n--- a/t/t0012-help.sh\n+++ b/t/t0012-help.sh\n@@ -2,6 +2,7 @@\n \n test_description='help'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n configure_help () {\n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"502849","messageId":"7cdd2691b77d328447855f898540f7efb2a415c3.1726484308.git.ps@pks.im","threadId":"62120","inReplyTo":"cover.1726484308.git.ps@pks.im","subject":"[PATCH 04/23] submodule: fix leaking update strategy","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-16T11:45:32Z","receivedAt":"2024-09-16T11:45:34Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We're not freeing the submodule update strategy command. Provide a\nhelper function that does this for us and call it in\n`update_data_release()`.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/submodule--helper.c | 1 +\n submodule-config.c | 2 +-\n submodule.c | 5 +++++\n submodule.h | 6 ++++--\n t/t7406-submodule-update.sh | 1 +\n 5 files changed, 12 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex bf8f9a40128..ed05dc51347 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2021,6 +2021,7 @@ struct update_data {\n static void update_data_release(struct update_data *ud)\n {\n \tfree(ud->displaypath);\n+\tsubmodule_update_strategy_release(&ud->update_strategy);\n \tmodule_list_release(&ud->list);\n }\n \ndiff --git a/submodule-config.c b/submodule-config.c\nindex c8f2bb2bdd3..471637a725a 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -95,7 +95,7 @@ static void free_one_config(struct submodule_entry *entry)\n \tfree((void *) entry->config->branch);\n \tfree((void *) entry->config->url);\n \tfree((void *) entry->config->ignore);\n-\tfree((void *) entry->config->update_strategy.command);\n+\tsubmodule_update_strategy_release(&entry->config->update_strategy);\n \tfree(entry->config);\n }\n \ndiff --git a/submodule.c b/submodule.c\nindex 97d0d47b561..0e67984d770 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -424,6 +424,11 @@ int parse_submodule_update_strategy(const char *value,\n \treturn 0;\n }\n \n+void submodule_update_strategy_release(struct submodule_update_strategy *strategy)\n+{\n+\tfree((char *) strategy->command);\n+}\n+\n const char *submodule_update_type_to_string(enum submodule_update_type type)\n {\n \tswitch (type) {\ndiff --git a/submodule.h b/submodule.h\nindex b50d29eba4f..4deb1b5f84e 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -41,6 +41,10 @@ struct submodule_update_strategy {\n \t.type = SM_UPDATE_UNSPECIFIED, \\\n }\n \n+int parse_submodule_update_strategy(const char *value,\n+\t\t\t\t struct submodule_update_strategy *dst);\n+void submodule_update_strategy_release(struct submodule_update_strategy *strategy);\n+\n int is_gitmodules_unmerged(struct index_state *istate);\n int is_writing_gitmodules_ok(void);\n int is_staging_gitmodules_ok(struct index_state *istate);\n@@ -70,8 +74,6 @@ void die_in_unpopulated_submodule(struct index_state *istate,\n void die_path_inside_submodule(struct index_state *istate,\n \t\t\t const struct pathspec *ps);\n enum submodule_update_type parse_submodule_update_type(const char *value);\n-int parse_submodule_update_strategy(const char *value,\n-\t\t\t\t struct submodule_update_strategy *dst);\n const char *submodule_update_type_to_string(enum submodule_update_type type);\n void handle_ignore_submodules_arg(struct diff_options *, const char *);\n void show_submodule_diff_summary(struct diff_options *o, const char *path,\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex 297c6c3b5cc..0f0c86f9cb2 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -12,6 +12,7 @@ submodule and \"git submodule update --rebase/--merge\" does not detach the HEAD.\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n \n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"502850","messageId":"0d0964a2bec7c6e6375a65a5ea9dc79a18ad325c.1726484308.git.ps@pks.im","threadId":"62120","inReplyTo":"cover.1726484308.git.ps@pks.im","subject":"[PATCH 05/23] builtin/submodule--helper: clear child process when not running it","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-16T11:45:35Z","receivedAt":"2024-09-16T11:45:40Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In `runcommand_in_submodule_cb()` we may end up not executing the child\ncommand when `argv` is empty. But we still populate the command with\nenvironment variables and other things, which needs cleanup. This leads\nto a memory leak because we do not call `finish_command()`.\n\nFix this by clearing the child process when we don't execute it.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/submodule--helper.c | 10 +++++++---\n t/t7407-submodule-foreach.sh | 1 +\n 2 files changed, 8 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex ed05dc51347..fd1b6794083 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -363,9 +363,13 @@ static void runcommand_in_submodule_cb(const struct cache_entry *list_item,\n \tif (!info->quiet)\n \t\tprintf(_(\"Entering '%s'\\n\"), displaypath);\n \n-\tif (info->argv[0] && run_command(&cp))\n-\t\tdie(_(\"run_command returned non-zero status for %s\\n.\"),\n-\t\t\tdisplaypath);\n+\tif (info->argv[0]) {\n+\t\tif (run_command(&cp))\n+\t\t\tdie(_(\"run_command returned non-zero status for %s\\n.\"),\n+\t\t\t displaypath);\n+\t} else {\n+\t\tchild_process_clear(&cp);\n+\t}\n \n \tif (info->recursive) {\n \t\tstruct child_process cpr = CHILD_PROCESS_INIT;\ndiff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh\nindex 8d7b234beb8..9f688932613 100755\n--- a/t/t7407-submodule-foreach.sh\n+++ b/t/t7407-submodule-foreach.sh\n@@ -12,6 +12,7 @@ that are currently checked out.\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n \n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"502851","messageId":"52d12e034b8104a0283958ada013b028322ce421.1726484308.git.ps@pks.im","threadId":"62120","inReplyTo":"cover.1726484308.git.ps@pks.im","subject":"[PATCH 06/23] builtin/submodule--helper: fix leaking error buffer","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-16T11:45:41Z","receivedAt":"2024-09-16T11:45:43Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Fix leaking error buffer when `compute_alternate_path()` fails.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/submodule--helper.c | 2 ++\n t/t7408-submodule-reference.sh | 2 ++\n 2 files changed, 4 insertions(+)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex fd1b6794083..ff1376f69fc 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1621,6 +1621,8 @@ static int add_possible_reference_from_superproject(\n \t\t\t\t; /* nothing */\n \t\t\t}\n \t\t}\n+\n+\t\tstrbuf_release(&err);\n \t\tstrbuf_release(&sb);\n \t}\n \ndiff --git a/t/t7408-submodule-reference.sh b/t/t7408-submodule-reference.sh\nindex d6040e0a337..7e1afa9ce47 100755\n--- a/t/t7408-submodule-reference.sh\n+++ b/t/t7408-submodule-reference.sh\n@@ -4,6 +4,8 @@\n #\n \n test_description='test clone --reference'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n base_dir=$(pwd)\n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"502852","messageId":"96bd7f01d5d1800a0d0520c0d790cc524e8b4c8c.1726484308.git.ps@pks.im","threadId":"62120","inReplyTo":"cover.1726484308.git.ps@pks.im","subject":"[PATCH 07/23] t/helper: fix leaking subrepo in nested submodule config helper","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-16T11:45:43Z","receivedAt":"2024-09-16T11:45:46Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In the \"submodule-nested-repo-config\" helper we create a submodule\nrepository and print its configuration. We do not clear the repo,\ncausing a memory leak. Plug it.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/helper/test-submodule-nested-repo-config.c | 2 +-\n t/t7411-submodule-config.sh | 1 +\n 2 files changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/t/helper/test-submodule-nested-repo-config.c b/t/helper/test-submodule-nested-repo-config.c\nindex 6ca069ce633..6dce9571531 100644\n--- a/t/helper/test-submodule-nested-repo-config.c\n+++ b/t/helper/test-submodule-nested-repo-config.c\n@@ -29,6 +29,6 @@ int cmd__submodule_nested_repo_config(int argc, const char **argv)\n \tprint_config_from_gitmodules(&subrepo, argv[2]);\n \n \tsubmodule_free(the_repository);\n-\n+\trepo_clear(&subrepo);\n \treturn 0;\n }\ndiff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh\nindex 31271f8e0a6..af0de496e07 100755\n--- a/t/t7411-submodule-config.sh\n+++ b/t/t7411-submodule-config.sh\n@@ -10,6 +10,7 @@ from the database and from the worktree works.\n '\n \n TEST_NO_CREATE_REPO=1\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'setup' '\n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"502853","messageId":"d088703d317a8598e1cc4eb068234c105cdeffe6.1726484308.git.ps@pks.im","threadId":"62120","inReplyTo":"cover.1726484308.git.ps@pks.im","subject":"[PATCH 08/23] builtin/submodule--helper: fix leaking remote ref on errors","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-16T11:45:46Z","receivedAt":"2024-09-16T11:45:49Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When `update_submodule()` fails we return with `die_message()`.\nCuriously enough, this causes a memory leak because we use the\n`run_process_parallel()` interfaces here, which swap out the die\nroutine.\n\nFix the leak by freeing the remote ref.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/submodule--helper.c | 13 +++++++++----\n t/t7420-submodule-set-url.sh | 1 +\n 2 files changed, 10 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex ff1376f69fc..a9bd93a7856 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2648,15 +2648,20 @@ static int update_submodule(struct update_data *update_data)\n \n \t\tif (!update_data->nofetch) {\n \t\t\tif (fetch_in_submodule(update_data->sm_path, update_data->depth,\n-\t\t\t\t\t 0, NULL))\n+\t\t\t\t\t 0, NULL)) {\n+\t\t\t\tfree(remote_ref);\n \t\t\t\treturn die_message(_(\"Unable to fetch in submodule path '%s'\"),\n \t\t\t\t\t\t update_data->sm_path);\n+\t\t\t}\n \t\t}\n \n \t\tif (repo_resolve_gitlink_ref(the_repository, update_data->sm_path,\n-\t\t\t\t\t remote_ref, &update_data->oid))\n-\t\t\treturn die_message(_(\"Unable to find %s revision in submodule path '%s'\"),\n-\t\t\t\t\t remote_ref, update_data->sm_path);\n+\t\t\t\t\t remote_ref, &update_data->oid)) {\n+\t\t\tret = die_message(_(\"Unable to find %s revision in submodule path '%s'\"),\n+\t\t\t\t\t remote_ref, update_data->sm_path);\n+\t\t\tfree(remote_ref);\n+\t\t\treturn ret;\n+\t\t}\n \n \t\tfree(remote_ref);\n \t}\ndiff --git a/t/t7420-submodule-set-url.sh b/t/t7420-submodule-set-url.sh\nindex bf7f15ee797..d7fe910bbe1 100755\n--- a/t/t7420-submodule-set-url.sh\n+++ b/t/t7420-submodule-set-url.sh\n@@ -10,6 +10,7 @@ as expected.\n '\n \n TEST_NO_CREATE_REPO=1\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'setup' '\n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"502854","messageId":"d5c9cccb8291a1941e6ee1bc8023232e8d5384de.1726484308.git.ps@pks.im","threadId":"62120","inReplyTo":"cover.1726484308.git.ps@pks.im","subject":"[PATCH 09/23] dir: fix off by one errors for ignored and untracked entries","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-16T11:45:49Z","receivedAt":"2024-09-16T11:45:51Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In `treat_directory()` we perform some logic to handle ignored and\nuntracked entries. When populating a directory with entries we first\nsave the current number of ignored/untracked entries and then populate\nnew entries at the end of our arrays that keep track of those entries.\nWhen we figure out that all entries have been ignored/are untracked we\nthen remove this tail of entries from those vectors again. But there is\nan off by one error in both paths that causes us to not free the first\nignored and untracked entries, respectively.\n\nFix these off-by-one errors to plug the resulting leak. While at it,\nmassage the code a bit to match our modern code style.\n\nSigned-off-by: Patrick Steinhardt \n---\n dir.c | 6 ++----\n t/t3011-common-prefixes-and-directory-traversal.sh | 1 +\n t/t7061-wtstatus-ignore.sh | 1 +\n t/t7521-ignored-mode.sh | 1 +\n 4 files changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex 5a23376bdae..787bcb7a1a4 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -2135,8 +2135,7 @@ static enum path_treatment treat_directory(struct dir_struct *dir,\n \t\t\t */\n \t\t\tstate = path_none;\n \t\t} else {\n-\t\t\tint i;\n-\t\t\tfor (i = old_ignored_nr + 1; iignored_nr; ++i)\n+\t\t\tfor (int i = old_ignored_nr; i < dir->ignored_nr; i++)\n \t\t\t\tFREE_AND_NULL(dir->ignored[i]);\n \t\t\tdir->ignored_nr = old_ignored_nr;\n \t\t}\n@@ -2148,8 +2147,7 @@ static enum path_treatment treat_directory(struct dir_struct *dir,\n \t */\n \tif ((dir->flags & DIR_SHOW_IGNORED_TOO) &&\n \t !(dir->flags & DIR_KEEP_UNTRACKED_CONTENTS)) {\n-\t\tint i;\n-\t\tfor (i = old_untracked_nr + 1; inr; ++i)\n+\t\tfor (int i = old_untracked_nr; i < dir->nr; i++)\n \t\t\tFREE_AND_NULL(dir->entries[i]);\n \t\tdir->nr = old_untracked_nr;\n \t}\ndiff --git a/t/t3011-common-prefixes-and-directory-traversal.sh b/t/t3011-common-prefixes-and-directory-traversal.sh\nindex 3da5b2b6e79..69e44c387fa 100755\n--- a/t/t3011-common-prefixes-and-directory-traversal.sh\n+++ b/t/t3011-common-prefixes-and-directory-traversal.sh\n@@ -2,6 +2,7 @@\n \n test_description='directory traversal handling, especially with common prefixes'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'setup' '\ndiff --git a/t/t7061-wtstatus-ignore.sh b/t/t7061-wtstatus-ignore.sh\nindex 2f9bea9793c..64145a05b1f 100755\n--- a/t/t7061-wtstatus-ignore.sh\n+++ b/t/t7061-wtstatus-ignore.sh\n@@ -2,6 +2,7 @@\n \n test_description='git-status ignored files'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n cat >expected <<\\EOF\ndiff --git a/t/t7521-ignored-mode.sh b/t/t7521-ignored-mode.sh\nindex a88b02b06ed..edce10f998e 100755\n--- a/t/t7521-ignored-mode.sh\n+++ b/t/t7521-ignored-mode.sh\n@@ -2,6 +2,7 @@\n \n test_description='git status ignored modes'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'setup initial commit and ignore file' '\n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"502855","messageId":"747c9a76a288d6fbfad85203e0bccaaf4b6db282.1726484308.git.ps@pks.im","threadId":"62120","inReplyTo":"cover.1726484308.git.ps@pks.im","subject":"[PATCH 10/23] builtin/pull: fix leaking \"ff\" option","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-16T11:45:52Z","receivedAt":"2024-09-16T11:45:53Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `opt_ff` field gets populated either via `OPT_PASSTHRU` via\n`config_get_ff()` or when `--rebase` is passed. So we sometimes end up\noverriding the value in `opt_ff` with another value, but we do not free\nthe old value, causing a memory leak.\n\nAdapt the type of the variable to be `char *` and consistently assign\nallocated strings to it such that we can easily free it when it is being\noverridden.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/pull.c | 11 +++++++----\n t/t7601-merge-pull-config.sh | 1 +\n 2 files changed, 8 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 4c54d8196fa..5d9d9e467e5 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -84,7 +84,7 @@ static const char *opt_squash;\n static const char *opt_commit;\n static const char *opt_edit;\n static const char *cleanup_arg;\n-static const char *opt_ff;\n+static char *opt_ff;\n static const char *opt_verify_signatures;\n static const char *opt_verify;\n static int opt_autostash = -1;\n@@ -1024,8 +1024,10 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \t\t * \"--rebase\" can override a config setting of\n \t\t * pull.ff=only.\n \t\t */\n-\t\tif (opt_rebase >= 0 && opt_ff && !strcmp(opt_ff, \"--ff-only\"))\n-\t\t\topt_ff = \"--ff\";\n+\t\tif (opt_rebase >= 0 && opt_ff && !strcmp(opt_ff, \"--ff-only\")) {\n+\t\t\tfree(opt_ff);\n+\t\t\topt_ff = xstrdup(\"--ff\");\n+\t\t}\n \t}\n \n \tif (opt_rebase < 0)\n@@ -1135,7 +1137,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \n \t\tif (can_ff) {\n \t\t\t/* we can fast-forward this without invoking rebase */\n-\t\t\topt_ff = \"--ff-only\";\n+\t\t\tfree(opt_ff);\n+\t\t\topt_ff = xstrdup(\"--ff-only\");\n \t\t\tret = run_merge();\n \t\t} else {\n \t\t\tret = run_rebase(&newbase, &upstream);\ndiff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh\nindex a94387a75f2..7fd8c086af3 100755\n--- a/t/t7601-merge-pull-config.sh\n+++ b/t/t7601-merge-pull-config.sh\n@@ -4,6 +4,7 @@ test_description='git merge\n \n Testing pull.* configuration parsing and other things.'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'setup' '\n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"502856","messageId":"85c0f9e5f598cc65d1c03f6622900bb950a31159.1726484308.git.ps@pks.im","threadId":"62120","inReplyTo":"cover.1726484308.git.ps@pks.im","subject":"[PATCH 11/23] diff: fix leaking orderfile option","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-16T11:45:54Z","receivedAt":"2024-09-16T11:45:58Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `orderfile` diff option is being assigned via `OPT_FILENAME()`,\nwhich assigns an allocated string to the variable. We never free it\nthough, causing a memory leak.\n\nChange the type of the string to `char *` and free it to plug the leak.\nThis also requires us to use `xstrdup()` to assign the global config to\nit in case it is set.\n\nThis leak is being hit in t7621, but plugging it alone does not make the\ntest suite pass.\n\nSigned-off-by: Patrick Steinhardt \n---\n combine-diff.c | 3 +--\n diff.c | 7 +++++--\n diff.h | 2 +-\n 3 files changed, 7 insertions(+), 5 deletions(-)\n\ndiff --git a/combine-diff.c b/combine-diff.c\nindex 829a44e4167..f6b624dc288 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -1393,9 +1393,8 @@ static struct combine_diff_path *find_paths_generic(const struct object_id *oid,\n {\n \tstruct combine_diff_path *paths = NULL;\n \tint i, num_parent = parents->nr;\n-\n \tint output_format = opt->output_format;\n-\tconst char *orderfile = opt->orderfile;\n+\tchar *orderfile = opt->orderfile;\n \n \topt->output_format = DIFF_FORMAT_NO_OUTPUT;\n \t/* tell diff_tree to emit paths in sorted (=tree) order */\ndiff --git a/diff.c b/diff.c\nindex 472479eb101..6555b8a32c1 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -441,8 +441,10 @@ int git_diff_ui_config(const char *var, const char *value,\n \t}\n \tif (!strcmp(var, \"diff.wordregex\"))\n \t\treturn git_config_string(&diff_word_regex_cfg, var, value);\n-\tif (!strcmp(var, \"diff.orderfile\"))\n+\tif (!strcmp(var, \"diff.orderfile\")) {\n+\t\tFREE_AND_NULL(diff_order_file_cfg);\n \t\treturn git_config_pathname(&diff_order_file_cfg, var, value);\n+\t}\n \n \tif (!strcmp(var, \"diff.ignoresubmodules\")) {\n \t\tif (!value)\n@@ -4775,7 +4777,7 @@ void repo_diff_setup(struct repository *r, struct diff_options *options)\n \tif (diff_indent_heuristic)\n \t\tDIFF_XDL_SET(options, INDENT_HEURISTIC);\n \n-\toptions->orderfile = diff_order_file_cfg;\n+\toptions->orderfile = xstrdup_or_null(diff_order_file_cfg);\n \n \tif (!options->flags.ignore_submodule_set)\n \t\toptions->flags.ignore_untracked_in_submodules = 1;\n@@ -6727,6 +6729,7 @@ void diff_free(struct diff_options *options)\n \t\tFREE_AND_NULL(options->objfind);\n \t}\n \n+\tFREE_AND_NULL(options->orderfile);\n \tfor (size_t i = 0; i < options->anchors_nr; i++)\n \t\tfree(options->anchors[i]);\n \tFREE_AND_NULL(options->anchors);\ndiff --git a/diff.h b/diff.h\nindex 9901c8ca8c8..b95d3c1e830 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -235,7 +235,7 @@ enum diff_submodule_format {\n * diffcore library with.\n */\n struct diff_options {\n-\tconst char *orderfile;\n+\tchar *orderfile;\n \n \t/*\n \t * \"--rotate-to=\" would start showing at and when\n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"502857","messageId":"330b6c52a0a99ba8fa18378310250e5b617d40c2.1726484308.git.ps@pks.im","threadId":"62120","inReplyTo":"cover.1726484308.git.ps@pks.im","subject":"[PATCH 12/23] parse-options: free previous value of `OPTION_FILENAME`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-16T11:45:59Z","receivedAt":"2024-09-16T11:46:01Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `OPTION_FILENAME` option always assigns either an allocated string\nor `NULL` to the value. In case it is passed multiple times it does not\nknow to free the previous value though, which causes a memory leak.\n\nRefactor the function to always free the previous value. None of the\nsites where this option is used pass a string constant, so this change\nis safe.\n\nWhile at it, fix the argument of `fix_filename()` to be a string\nconstant. The only reason why it's not is because we use it as an\nin-out-parameter, where the input is a constant and the output is not.\nThis is weird and unnecessary, as we can just return the result instead\nof using the parameter for this.\n\nThis leak is being hit in t7621, but plugging it alone does not make the\ntest suite pass.\n\nSigned-off-by: Patrick Steinhardt \n---\n parse-options.c | 22 ++++++++++++++--------\n 1 file changed, 14 insertions(+), 8 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 30b9e68f8ac..33bfba0ed4a 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -60,12 +60,12 @@ static enum parse_opt_result get_arg(struct parse_opt_ctx_t *p,\n \treturn 0;\n }\n \n-static void fix_filename(const char *prefix, char **file)\n+static char *fix_filename(const char *prefix, const char *file)\n {\n \tif (!file || !*file)\n-\t\t; /* leave as NULL */\n+\t\treturn NULL;\n \telse\n-\t\t*file = prefix_filename_except_for_dash(prefix, *file);\n+\t\treturn prefix_filename_except_for_dash(prefix, file);\n }\n \n static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,\n@@ -129,18 +129,24 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,\n \t\treturn 0;\n \n \tcase OPTION_FILENAME:\n+\t{\n+\t\tconst char *value;\n+\n+\t\tFREE_AND_NULL(*(char **)opt->value);\n+\n \t\terr = 0;\n+\n \t\tif (unset)\n-\t\t\t*(const char **)opt->value = NULL;\n+\t\t\tvalue = NULL;\n \t\telse if (opt->flags & PARSE_OPT_OPTARG && !p->opt)\n-\t\t\t*(const char **)opt->value = (const char *)opt->defval;\n+\t\t\tvalue = (const char *) opt->defval;\n \t\telse\n-\t\t\terr = get_arg(p, opt, flags, (const char **)opt->value);\n+\t\t\terr = get_arg(p, opt, flags, &value);\n \n \t\tif (!err)\n-\t\t\tfix_filename(p->prefix, (char **)opt->value);\n+\t\t\t*(char **)opt->value = fix_filename(p->prefix, value);\n \t\treturn err;\n-\n+\t}\n \tcase OPTION_CALLBACK:\n \t{\n \t\tconst char *p_arg = NULL;\n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"502858","messageId":"c975dfe462e321252bd4c83f4ed18bc7757204b7.1726484308.git.ps@pks.im","threadId":"62120","inReplyTo":"cover.1726484308.git.ps@pks.im","subject":"[PATCH 13/23] diffcore-order: fix leaking buffer when parsing orderfiles","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-16T11:46:02Z","receivedAt":"2024-09-16T11:46:04Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In `prepare_order()` we parse an orderfile and assign it to a global\narray. In order to save on some allocations, we replace newlines with\nNUL characters and then assign pointers into the allocated buffer to\nthat array. This can cause the buffer to be completely unreferenced\nthough in some cases, e.g. because the order file is empty or because we\nhad to use `xmemdupz()` to copy the lines instead of NUL-terminating\nthem.\n\nRefactor the code to always `xmemdupz()` the strings. This is a bit\nsimpler, and it is rather unlikely that saving a handful of allocations\nreally matters. This allows us to release the string buffer and thus\nplug the memory leak.\n\nSigned-off-by: Patrick Steinhardt \n---\n diffcore-order.c | 19 +++++++------------\n t/t4056-diff-order.sh | 1 +\n t/t4204-patch-id.sh | 1 +\n 3 files changed, 9 insertions(+), 12 deletions(-)\n\ndiff --git a/diffcore-order.c b/diffcore-order.c\nindex e7d20ebd2d1..912513d3e67 100644\n--- a/diffcore-order.c\n+++ b/diffcore-order.c\n@@ -14,8 +14,7 @@ static void prepare_order(const char *orderfile)\n {\n \tint cnt, pass;\n \tstruct strbuf sb = STRBUF_INIT;\n-\tvoid *map;\n-\tchar *cp, *endp;\n+\tconst char *cp, *endp;\n \tssize_t sz;\n \n \tif (order)\n@@ -24,14 +23,13 @@ static void prepare_order(const char *orderfile)\n \tsz = strbuf_read_file(&sb, orderfile, 0);\n \tif (sz < 0)\n \t\tdie_errno(_(\"failed to read orderfile '%s'\"), orderfile);\n-\tmap = strbuf_detach(&sb, NULL);\n-\tendp = (char *) map + sz;\n+\tendp = sb.buf + sz;\n \n \tfor (pass = 0; pass < 2; pass++) {\n \t\tcnt = 0;\n-\t\tcp = map;\n+\t\tcp = sb.buf;\n \t\twhile (cp < endp) {\n-\t\t\tchar *ep;\n+\t\t\tconst char *ep;\n \t\t\tfor (ep = cp; ep < endp && *ep != '\\n'; ep++)\n \t\t\t\t;\n \t\t\t/* cp to ep has one line */\n@@ -40,12 +38,7 @@ static void prepare_order(const char *orderfile)\n \t\t\telse if (pass == 0)\n \t\t\t\tcnt++;\n \t\t\telse {\n-\t\t\t\tif (*ep == '\\n') {\n-\t\t\t\t\t*ep = 0;\n-\t\t\t\t\torder[cnt] = cp;\n-\t\t\t\t} else {\n-\t\t\t\t\torder[cnt] = xmemdupz(cp, ep - cp);\n-\t\t\t\t}\n+\t\t\t\torder[cnt] = xmemdupz(cp, ep - cp);\n \t\t\t\tcnt++;\n \t\t\t}\n \t\t\tif (ep < endp)\n@@ -57,6 +50,8 @@ static void prepare_order(const char *orderfile)\n \t\t\tALLOC_ARRAY(order, cnt);\n \t\t}\n \t}\n+\n+\tstrbuf_release(&sb);\n }\n \n static int match_order(const char *path)\ndiff --git a/t/t4056-diff-order.sh b/t/t4056-diff-order.sh\nindex aec1d9d1b42..32c5fcb9a27 100755\n--- a/t/t4056-diff-order.sh\n+++ b/t/t4056-diff-order.sh\n@@ -5,6 +5,7 @@ test_description='diff order & rotate'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n create_files () {\ndiff --git a/t/t4204-patch-id.sh b/t/t4204-patch-id.sh\nindex dc8ddb10aff..c0a4a02dcfa 100755\n--- a/t/t4204-patch-id.sh\n+++ b/t/t4204-patch-id.sh\n@@ -5,6 +5,7 @@ test_description='git patch-id'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'setup' '\n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"502859","messageId":"a5f3931eee1e49c60e1d5be214263aeaf3d5c65f.1726484308.git.ps@pks.im","threadId":"62120","inReplyTo":"cover.1726484308.git.ps@pks.im","subject":"[PATCH 14/23] builtin/repack: fix leaking configuration","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-16T11:46:05Z","receivedAt":"2024-09-16T11:46:07Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When repacking, we assemble git-pack-objects(1) arguments both for the\n\"normal\" pack and for the cruft pack. This configuration gets populated\nwith a bunch of `OPT_PASSTHRU` options that we end up passing to the\nchild process. These options are allocated, but never free'd.\n\nCreate a new `pack_objects_args_release()` function that releases the\nmemory for us and call it for both sets of options.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/repack.c | 57 ++++++++++++++++++++++++++---------\n t/t5329-pack-objects-cruft.sh | 2 ++\n t/t7700-repack.sh | 1 +\n 3 files changed, 45 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 3ee8cfa732f..c31d5653f1f 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -85,17 +85,34 @@ static int repack_config(const char *var, const char *value,\n \t\trun_update_server_info = git_config_bool(var, value);\n \t\treturn 0;\n \t}\n-\tif (!strcmp(var, \"repack.cruftwindow\"))\n+\tif (!strcmp(var, \"repack.cruftwindow\")) {\n+\t\tfree(cruft_po_args->window);\n \t\treturn git_config_string(&cruft_po_args->window, var, value);\n-\tif (!strcmp(var, \"repack.cruftwindowmemory\"))\n+\t}\n+\tif (!strcmp(var, \"repack.cruftwindowmemory\")) {\n+\t\tfree(cruft_po_args->window_memory);\n \t\treturn git_config_string(&cruft_po_args->window_memory, var, value);\n-\tif (!strcmp(var, \"repack.cruftdepth\"))\n+\t}\n+\tif (!strcmp(var, \"repack.cruftdepth\")) {\n+\t\tfree(cruft_po_args->depth);\n \t\treturn git_config_string(&cruft_po_args->depth, var, value);\n-\tif (!strcmp(var, \"repack.cruftthreads\"))\n+\t}\n+\tif (!strcmp(var, \"repack.cruftthreads\")) {\n+\t\tfree(cruft_po_args->threads);\n \t\treturn git_config_string(&cruft_po_args->threads, var, value);\n+\t}\n \treturn git_default_config(var, value, ctx, cb);\n }\n \n+static void pack_objects_args_release(struct pack_objects_args *args)\n+{\n+\tfree(args->window);\n+\tfree(args->window_memory);\n+\tfree(args->depth);\n+\tfree(args->threads);\n+\tlist_objects_filter_release(&args->filter_options);\n+}\n+\n struct existing_packs {\n \tstruct string_list kept_packs;\n \tstruct string_list non_kept_packs;\n@@ -1152,12 +1169,16 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tconst char *unpack_unreachable = NULL;\n \tint keep_unreachable = 0;\n \tstruct string_list keep_pack_list = STRING_LIST_INIT_NODUP;\n-\tstruct pack_objects_args po_args = {NULL};\n-\tstruct pack_objects_args cruft_po_args = {NULL};\n+\tstruct pack_objects_args po_args = { 0 };\n+\tstruct pack_objects_args cruft_po_args = { 0 };\n \tint write_midx = 0;\n \tconst char *cruft_expiration = NULL;\n \tconst char *expire_to = NULL;\n \tconst char *filter_to = NULL;\n+\tconst char *opt_window = NULL;\n+\tconst char *opt_window_memory = NULL;\n+\tconst char *opt_depth = NULL;\n+\tconst char *opt_threads = NULL;\n \n \tstruct option builtin_repack_options[] = {\n \t\tOPT_BIT('a', NULL, &pack_everything,\n@@ -1191,13 +1212,13 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\t\tN_(\"with -A, do not loosen objects older than this\")),\n \t\tOPT_BOOL('k', \"keep-unreachable\", &keep_unreachable,\n \t\t\t\tN_(\"with -a, repack unreachable objects\")),\n-\t\tOPT_STRING(0, \"window\", &po_args.window, N_(\"n\"),\n+\t\tOPT_STRING(0, \"window\", &opt_window, N_(\"n\"),\n \t\t\t\tN_(\"size of the window used for delta compression\")),\n-\t\tOPT_STRING(0, \"window-memory\", &po_args.window_memory, N_(\"bytes\"),\n+\t\tOPT_STRING(0, \"window-memory\", &opt_window_memory, N_(\"bytes\"),\n \t\t\t\tN_(\"same as the above, but limit memory size instead of entries count\")),\n-\t\tOPT_STRING(0, \"depth\", &po_args.depth, N_(\"n\"),\n+\t\tOPT_STRING(0, \"depth\", &opt_depth, N_(\"n\"),\n \t\t\t\tN_(\"limits the maximum delta depth\")),\n-\t\tOPT_STRING(0, \"threads\", &po_args.threads, N_(\"n\"),\n+\t\tOPT_STRING(0, \"threads\", &opt_threads, N_(\"n\"),\n \t\t\t\tN_(\"limits the maximum number of threads\")),\n \t\tOPT_MAGNITUDE(0, \"max-pack-size\", &po_args.max_pack_size,\n \t\t\t\tN_(\"maximum size of each packfile\")),\n@@ -1224,6 +1245,11 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, prefix, builtin_repack_options,\n \t\t\t\tgit_repack_usage, 0);\n \n+\tpo_args.window = xstrdup_or_null(opt_window);\n+\tpo_args.window_memory = xstrdup_or_null(opt_window_memory);\n+\tpo_args.depth = xstrdup_or_null(opt_depth);\n+\tpo_args.threads = xstrdup_or_null(opt_threads);\n+\n \tif (delete_redundant && repository_format_precious_objects)\n \t\tdie(_(\"cannot delete packs in a precious-objects repo\"));\n \n@@ -1389,13 +1415,13 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\tconst char *pack_prefix = find_pack_prefix(packdir, packtmp);\n \n \t\tif (!cruft_po_args.window)\n-\t\t\tcruft_po_args.window = po_args.window;\n+\t\t\tcruft_po_args.window = xstrdup_or_null(po_args.window);\n \t\tif (!cruft_po_args.window_memory)\n-\t\t\tcruft_po_args.window_memory = po_args.window_memory;\n+\t\t\tcruft_po_args.window_memory = xstrdup_or_null(po_args.window_memory);\n \t\tif (!cruft_po_args.depth)\n-\t\t\tcruft_po_args.depth = po_args.depth;\n+\t\t\tcruft_po_args.depth = xstrdup_or_null(po_args.depth);\n \t\tif (!cruft_po_args.threads)\n-\t\t\tcruft_po_args.threads = po_args.threads;\n+\t\t\tcruft_po_args.threads = xstrdup_or_null(po_args.threads);\n \t\tif (!cruft_po_args.max_pack_size)\n \t\t\tcruft_po_args.max_pack_size = po_args.max_pack_size;\n \n@@ -1547,7 +1573,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tstring_list_clear(&names, 1);\n \texisting_packs_release(&existing);\n \tfree_pack_geometry(&geometry);\n-\tlist_objects_filter_release(&po_args.filter_options);\n+\tpack_objects_args_release(&po_args);\n+\tpack_objects_args_release(&cruft_po_args);\n \n \treturn ret;\n }\ndiff --git a/t/t5329-pack-objects-cruft.sh b/t/t5329-pack-objects-cruft.sh\nindex fc5fedbe9b0..445739d06c9 100755\n--- a/t/t5329-pack-objects-cruft.sh\n+++ b/t/t5329-pack-objects-cruft.sh\n@@ -1,6 +1,8 @@\n #!/bin/sh\n \n test_description='cruft pack related pack-objects tests'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n objdir=.git/objects\ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex be1188e7365..c4c3d1a15d9 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -2,6 +2,7 @@\n \n test_description='git repack works correctly'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n . \"${TEST_DIRECTORY}/lib-bitmap.sh\"\n . \"${TEST_DIRECTORY}/lib-midx.sh\"\n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"502860","messageId":"c79a5118e42284cd59e1003a2637cd0491cec050.1726484308.git.ps@pks.im","threadId":"62120","inReplyTo":"cover.1726484308.git.ps@pks.im","subject":"[PATCH 15/23] builtin/difftool: plug several trivial memory leaks","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-16T11:46:07Z","receivedAt":"2024-09-16T11:46:09Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"There are several leaking data structures in git-difftool(1). Plug them.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/difftool.c | 6 ++++++\n t/t7800-difftool.sh | 1 +\n 2 files changed, 7 insertions(+)\n\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex dcc68e190c2..1a68ab66993 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -660,6 +660,12 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \tif (fp)\n \t\tfclose(fp);\n \n+\thashmap_clear_and_free(&working_tree_dups, struct working_tree_entry, entry);\n+\thashmap_clear_and_free(&wt_modified, struct path_entry, entry);\n+\thashmap_clear_and_free(&tmp_modified, struct path_entry, entry);\n+\thashmap_clear_and_free(&submodules, struct pair_entry, entry);\n+\thashmap_clear_and_free(&symlinks2, struct pair_entry, entry);\n+\trelease_index(&wtindex);\n \tfree(lbase_dir);\n \tfree(rbase_dir);\n \tstrbuf_release(&info);\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex cc917b257e3..f67b9345b8a 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -11,6 +11,7 @@ Testing basic diff tool invocation\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n difftool_test_setup ()\n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"502861","messageId":"0fb3dc55e58559272cc79c69646be78aefd2f7ac.1726484308.git.ps@pks.im","threadId":"62120","inReplyTo":"cover.1726484308.git.ps@pks.im","subject":"[PATCH 16/23] trace2: destroy context stored in thread-local storage","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-16T11:46:10Z","receivedAt":"2024-09-16T11:46:12Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Each thread may have a specific context in the trace2 subsystem that we\nset up via thread-local storage. We do not set up a destructor for this\ndata though, which means that the context data will leak.\n\nPlug this leak by installing a destructor. This leak is exposed by\nt7814, but plugging it alone does not make the whole test suite pass.\n\nSigned-off-by: Patrick Steinhardt \n---\n trace2/tr2_tls.c | 10 +++++++++-\n 1 file changed, 9 insertions(+), 1 deletion(-)\n\ndiff --git a/trace2/tr2_tls.c b/trace2/tr2_tls.c\nindex 4f75392952b..7b023c1bfc6 100644\n--- a/trace2/tr2_tls.c\n+++ b/trace2/tr2_tls.c\n@@ -152,11 +152,19 @@ uint64_t tr2tls_absolute_elapsed(uint64_t us)\n \treturn us - tr2tls_us_start_process;\n }\n \n+static void tr2tls_key_destructor(void *payload)\n+{\n+\tstruct tr2tls_thread_ctx *ctx = payload;\n+\tfree((char *)ctx->thread_name);\n+\tfree(ctx->array_us_start);\n+\tfree(ctx);\n+}\n+\n void tr2tls_init(void)\n {\n \ttr2tls_start_process_clock();\n \n-\tpthread_key_create(&tr2tls_key, NULL);\n+\tpthread_key_create(&tr2tls_key, tr2tls_key_destructor);\n \tinit_recursive_mutex(&tr2tls_mutex);\n \n \ttr2tls_thread_main =\n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"502862","messageId":"f1cb8122d18746852b31f36d49c6aa760c572545.1726484308.git.ps@pks.im","threadId":"62120","inReplyTo":"cover.1726484308.git.ps@pks.im","subject":"[PATCH 17/23] submodule: fix leaking submodule ODB paths","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-16T11:46:13Z","receivedAt":"2024-09-16T11:46:17Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In `add_submodule_odb_by_path()` we add a path into a global string\nlist. The list is initialized with `NODUP`, which means that we do not\npass ownership of strings to the list. But we use `xstrdup()` when we\ninsert a path, with the consequence that the string will never get\nfree'd.\n\nPlug the leak by marking the list as `DUP`. There is only a single\ncallsite where we insert paths anyway, and as explained above that\ncallsite was mishandling the allocation.\n\nThis leak is exposed by t7814, but plugging it does not make the whole\ntest suite pass.\n\nSigned-off-by: Patrick Steinhardt \n---\n submodule.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 0e67984d770..a07debc227f 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -175,11 +175,11 @@ void stage_updated_gitmodules(struct index_state *istate)\n \t\tdie(_(\"staging updated .gitmodules failed\"));\n }\n \n-static struct string_list added_submodule_odb_paths = STRING_LIST_INIT_NODUP;\n+static struct string_list added_submodule_odb_paths = STRING_LIST_INIT_DUP;\n \n void add_submodule_odb_by_path(const char *path)\n {\n-\tstring_list_insert(&added_submodule_odb_paths, xstrdup(path));\n+\tstring_list_insert(&added_submodule_odb_paths, path);\n }\n \n int register_all_submodule_odb_as_alternates(void)\n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"502863","messageId":"411df7248d2868e906d5ba882f9cd9abf82eaca8.1726484308.git.ps@pks.im","threadId":"62120","inReplyTo":"cover.1726484308.git.ps@pks.im","subject":"[PATCH 18/23] grep: fix leaking grep pattern","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-16T11:46:18Z","receivedAt":"2024-09-16T11:46:20Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When creating a pattern via `create_grep_pat()` we allocate the pattern\nmember of the structure regardless of the token type. But later, when we\ntry to free the structure, we free the pattern member conditionally on\nthe token type and thus leak memory.\n\nPlug this leak. The leak is exposed by t7814, but plugging it alone does\nnot make the whole test suite pass.\n\nSigned-off-by: Patrick Steinhardt \n---\n grep.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/grep.c b/grep.c\nindex e5761426e4f..701e58de04e 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -843,11 +843,11 @@ static void free_grep_pat(struct grep_pat *pattern)\n \t\t\t\tfree_pcre2_pattern(p);\n \t\t\telse\n \t\t\t\tregfree(&p->regexp);\n-\t\t\tfree(p->pattern);\n \t\t\tbreak;\n \t\tdefault:\n \t\t\tbreak;\n \t\t}\n+\t\tfree(p->pattern);\n \t\tfree(p);\n \t}\n }\n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"502864","messageId":"690de28bef8c4c7608aaf5c1f92ee24525dcb987.1726484308.git.ps@pks.im","threadId":"62120","inReplyTo":"cover.1726484308.git.ps@pks.im","subject":"[PATCH 19/23] promisor-remote: fix leaking partial clone filter","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-16T11:46:20Z","receivedAt":"2024-09-16T11:46:22Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The partial clone filter of a promisor remote is never free'd, causing\nmemory leaks. Furthermore, in case multiple partial clone filters are\ndefined for the same remote, we'd overwrite previous values without\nfreeing them.\n\nFix these leaks.\n\nSigned-off-by: Patrick Steinhardt \n---\n promisor-remote.c | 2 ++\n t/t7814-grep-recurse-submodules.sh | 1 +\n 2 files changed, 3 insertions(+)\n\ndiff --git a/promisor-remote.c b/promisor-remote.c\nindex 317e1b127fe..9345ae3db23 100644\n--- a/promisor-remote.c\n+++ b/promisor-remote.c\n@@ -154,6 +154,7 @@ static int promisor_remote_config(const char *var, const char *value,\n \t\tif (!r)\n \t\t\treturn 0;\n \n+\t\tFREE_AND_NULL(r->partial_clone_filter);\n \t\treturn git_config_string(&r->partial_clone_filter, var, value);\n \t}\n \n@@ -189,6 +190,7 @@ void promisor_remote_clear(struct promisor_remote_config *config)\n {\n \twhile (config->promisors) {\n \t\tstruct promisor_remote *r = config->promisors;\n+\t\tfree(r->partial_clone_filter);\n \t\tconfig->promisors = config->promisors->next;\n \t\tfree(r);\n \t}\ndiff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh\nindex 167fe661504..55ed630e77e 100755\n--- a/t/t7814-grep-recurse-submodules.sh\n+++ b/t/t7814-grep-recurse-submodules.sh\n@@ -7,6 +7,7 @@ submodules.\n '\n \n TEST_CREATE_REPO_NO_TEMPLATE=1\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB=1\n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"502865","messageId":"ed4091255cd25e82db01cb05df63b8c6804b6449.1726484308.git.ps@pks.im","threadId":"62120","inReplyTo":"cover.1726484308.git.ps@pks.im","subject":"[PATCH 20/23] builtin/maintenance: fix leaking config string","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-16T11:46:23Z","receivedAt":"2024-09-16T11:46:25Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When parsing the maintenance strategy from config we allocate a config\nstring, but do not free it after parsing it. Plug this leak by instead\nusing `git_config_get_string_tmp()`, which does not allocate any memory.\n\nThis leak is exposed by t7900, but plugging it alone does not make the\ntest suite pass.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/gc.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 7dac9714054..3acfa367ad1 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -1476,9 +1476,9 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts,\n \n static void initialize_maintenance_strategy(void)\n {\n-\tchar *config_str;\n+\tconst char *config_str;\n \n-\tif (git_config_get_string(\"maintenance.strategy\", &config_str))\n+\tif (git_config_get_string_tmp(\"maintenance.strategy\", &config_str))\n \t\treturn;\n \n \tif (!strcasecmp(config_str, \"incremental\")) {\n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"502866","messageId":"46956bd8fbda45a9e2115e2b3b40c1ab20ea3eee.1726484308.git.ps@pks.im","threadId":"62120","inReplyTo":"cover.1726484308.git.ps@pks.im","subject":"[PATCH 21/23] builtin/maintenance: fix leak in `get_schedule_cmd()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-16T11:46:26Z","receivedAt":"2024-09-16T11:46:28Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `get_schedule_cmd()` function allows us to override the schedule\ncommand with a specific test command such that we can verify the\nunderlying logic in a platform-independent way. Its memory management is\nsomewhat wild though, because it basically gives up and assigns an\nallocated string to the string constant output pointer. While this part\nis marked with `UNLEAK()` to mask this, we also leak the local string\nlists.\n\nRework the function such that it has a separate out parameter. If set,\nwe will assign it the final allocated command. Plug the other memory\nleaks and create a common exit path.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/gc.c | 127 ++++++++++++++++++++++++++---------------\n t/t7900-maintenance.sh | 1 +\n 2 files changed, 81 insertions(+), 47 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 3acfa367ad1..b68a0be62c1 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -1780,32 +1780,33 @@ static const char *get_frequency(enum schedule_priority schedule)\n * * If $GIT_TEST_MAINT_SCHEDULER is set, return true.\n * In this case, the *cmd value is read as input.\n *\n- * * if the input value *cmd is the key of one of the comma-separated list\n- * item, then *is_available is set to true and *cmd is modified and becomes\n+ * * if the input value cmd is the key of one of the comma-separated list\n+ * item, then *is_available is set to true and *out is set to\n * the mock command.\n *\n * * if the input value *cmd isn’t the key of any of the comma-separated list\n- * item, then *is_available is set to false.\n+ * item, then *is_available is set to false and *out is set to the original\n+ * command.\n *\n * Ex.:\n * GIT_TEST_MAINT_SCHEDULER not set\n * +-------+-------------------------------------------------+\n * | Input | Output |\n- * | *cmd | return code | *cmd | *is_available |\n+ * | *cmd | return code | *out | *is_available |\n * +-------+-------------+-------------------+---------------+\n- * | \"foo\" | false | \"foo\" (unchanged) | (unchanged) |\n+ * | \"foo\" | false | NULL | (unchanged) |\n * +-------+-------------+-------------------+---------------+\n *\n * GIT_TEST_MAINT_SCHEDULER set to “foo:./mock_foo.sh,bar:./mock_bar.sh”\n * +-------+-------------------------------------------------+\n * | Input | Output |\n- * | *cmd | return code | *cmd | *is_available |\n+ * | *cmd | return code | *out | *is_available |\n * +-------+-------------+-------------------+---------------+\n * | \"foo\" | true | \"./mock.foo.sh\" | true |\n- * | \"qux\" | true | \"qux\" (unchanged) | false |\n+ * | \"qux\" | true | \"qux\" (allocated) | false |\n * +-------+-------------+-------------------+---------------+\n */\n-static int get_schedule_cmd(const char **cmd, int *is_available)\n+static int get_schedule_cmd(const char *cmd, int *is_available, char **out)\n {\n \tchar *testing = xstrdup_or_null(getenv(\"GIT_TEST_MAINT_SCHEDULER\"));\n \tstruct string_list_item *item;\n@@ -1824,16 +1825,22 @@ static int get_schedule_cmd(const char **cmd, int *is_available)\n \t\tif (string_list_split_in_place(&pair, item->string, \":\", 2) != 2)\n \t\t\tcontinue;\n \n-\t\tif (!strcmp(*cmd, pair.items[0].string)) {\n-\t\t\t*cmd = pair.items[1].string;\n+\t\tif (!strcmp(cmd, pair.items[0].string)) {\n+\t\t\tif (out)\n+\t\t\t\t*out = xstrdup(pair.items[1].string);\n \t\t\tif (is_available)\n \t\t\t\t*is_available = 1;\n-\t\t\tstring_list_clear(&list, 0);\n-\t\t\tUNLEAK(testing);\n-\t\t\treturn 1;\n+\t\t\tstring_list_clear(&pair, 0);\n+\t\t\tgoto out;\n \t\t}\n+\n+\t\tstring_list_clear(&pair, 0);\n \t}\n \n+\tif (out)\n+\t\t*out = xstrdup(cmd);\n+\n+out:\n \tstring_list_clear(&list, 0);\n \tfree(testing);\n \treturn 1;\n@@ -1850,9 +1857,8 @@ static int get_random_minute(void)\n \n static int is_launchctl_available(void)\n {\n-\tconst char *cmd = \"launchctl\";\n \tint is_available;\n-\tif (get_schedule_cmd(&cmd, &is_available))\n+\tif (get_schedule_cmd(\"launchctl\", &is_available, NULL))\n \t\treturn is_available;\n \n #ifdef __APPLE__\n@@ -1890,12 +1896,12 @@ static char *launchctl_get_uid(void)\n \n static int launchctl_boot_plist(int enable, const char *filename)\n {\n-\tconst char *cmd = \"launchctl\";\n+\tchar *cmd;\n \tint result;\n \tstruct child_process child = CHILD_PROCESS_INIT;\n \tchar *uid = launchctl_get_uid();\n \n-\tget_schedule_cmd(&cmd, NULL);\n+\tget_schedule_cmd(\"launchctl\", NULL, &cmd);\n \tstrvec_split(&child.args, cmd);\n \tstrvec_pushl(&child.args, enable ? \"bootstrap\" : \"bootout\", uid,\n \t\t filename, NULL);\n@@ -1908,6 +1914,7 @@ static int launchctl_boot_plist(int enable, const char *filename)\n \n \tresult = finish_command(&child);\n \n+\tfree(cmd);\n \tfree(uid);\n \treturn result;\n }\n@@ -1959,10 +1966,10 @@ static int launchctl_schedule_plist(const char *exec_path, enum schedule_priorit\n \tstatic unsigned long lock_file_timeout_ms = ULONG_MAX;\n \tstruct strbuf plist = STRBUF_INIT, plist2 = STRBUF_INIT;\n \tstruct stat st;\n-\tconst char *cmd = \"launchctl\";\n+\tchar *cmd;\n \tint minute = get_random_minute();\n \n-\tget_schedule_cmd(&cmd, NULL);\n+\tget_schedule_cmd(\"launchctl\", NULL, &cmd);\n \tpreamble = \"\\n\"\n \t\t \"\\n\"\n \t\t \"\"\n@@ -2052,6 +2059,7 @@ static int launchctl_schedule_plist(const char *exec_path, enum schedule_priorit\n \n \tfree(filename);\n \tfree(name);\n+\tfree(cmd);\n \tstrbuf_release(&plist);\n \tstrbuf_release(&plist2);\n \treturn 0;\n@@ -2076,9 +2084,8 @@ static int launchctl_update_schedule(int run_maintenance, int fd UNUSED)\n \n static int is_schtasks_available(void)\n {\n-\tconst char *cmd = \"schtasks\";\n \tint is_available;\n-\tif (get_schedule_cmd(&cmd, &is_available))\n+\tif (get_schedule_cmd(\"schtasks\", &is_available, NULL))\n \t\treturn is_available;\n \n #ifdef GIT_WINDOWS_NATIVE\n@@ -2097,15 +2104,16 @@ static char *schtasks_task_name(const char *frequency)\n \n static int schtasks_remove_task(enum schedule_priority schedule)\n {\n-\tconst char *cmd = \"schtasks\";\n+\tchar *cmd;\n \tstruct child_process child = CHILD_PROCESS_INIT;\n \tconst char *frequency = get_frequency(schedule);\n \tchar *name = schtasks_task_name(frequency);\n \n-\tget_schedule_cmd(&cmd, NULL);\n+\tget_schedule_cmd(\"schtasks\", NULL, &cmd);\n \tstrvec_split(&child.args, cmd);\n \tstrvec_pushl(&child.args, \"/delete\", \"/tn\", name, \"/f\", NULL);\n \tfree(name);\n+\tfree(cmd);\n \n \treturn run_command(&child);\n }\n@@ -2119,7 +2127,7 @@ static int schtasks_remove_tasks(void)\n \n static int schtasks_schedule_task(const char *exec_path, enum schedule_priority schedule)\n {\n-\tconst char *cmd = \"schtasks\";\n+\tchar *cmd;\n \tint result;\n \tstruct child_process child = CHILD_PROCESS_INIT;\n \tconst char *xml;\n@@ -2129,7 +2137,7 @@ static int schtasks_schedule_task(const char *exec_path, enum schedule_priority\n \tstruct strbuf tfilename = STRBUF_INIT;\n \tint minute = get_random_minute();\n \n-\tget_schedule_cmd(&cmd, NULL);\n+\tget_schedule_cmd(\"schtasks\", NULL, &cmd);\n \n \tstrbuf_addf(&tfilename, \"%s/schedule_%s_XXXXXX\",\n \t\t get_git_common_dir(), frequency);\n@@ -2235,6 +2243,7 @@ static int schtasks_schedule_task(const char *exec_path, enum schedule_priority\n \n \tdelete_tempfile(&tfile);\n \tfree(name);\n+\tfree(cmd);\n \treturn result;\n }\n \n@@ -2276,21 +2285,28 @@ static int check_crontab_process(const char *cmd)\n \n static int is_crontab_available(void)\n {\n-\tconst char *cmd = \"crontab\";\n+\tchar *cmd;\n \tint is_available;\n+\tint ret;\n \n-\tif (get_schedule_cmd(&cmd, &is_available))\n-\t\treturn is_available;\n+\tif (get_schedule_cmd(\"crontab\", &is_available, &cmd)) {\n+\t\tret = is_available;\n+\t\tgoto out;\n+\t}\n \n #ifdef __APPLE__\n \t/*\n \t * macOS has cron, but it requires special permissions and will\n \t * create a UI alert when attempting to run this command.\n \t */\n-\treturn 0;\n+\tret = 0;\n #else\n-\treturn check_crontab_process(cmd);\n+\tret = check_crontab_process(cmd);\n #endif\n+\n+out:\n+\tfree(cmd);\n+\treturn ret;\n }\n \n #define BEGIN_LINE \"# BEGIN GIT MAINTENANCE SCHEDULE\"\n@@ -2298,7 +2314,7 @@ static int is_crontab_available(void)\n \n static int crontab_update_schedule(int run_maintenance, int fd)\n {\n-\tconst char *cmd = \"crontab\";\n+\tchar *cmd;\n \tint result = 0;\n \tint in_old_region = 0;\n \tstruct child_process crontab_list = CHILD_PROCESS_INIT;\n@@ -2308,15 +2324,17 @@ static int crontab_update_schedule(int run_maintenance, int fd)\n \tstruct tempfile *tmpedit = NULL;\n \tint minute = get_random_minute();\n \n-\tget_schedule_cmd(&cmd, NULL);\n+\tget_schedule_cmd(\"crontab\", NULL, &cmd);\n \tstrvec_split(&crontab_list.args, cmd);\n \tstrvec_push(&crontab_list.args, \"-l\");\n \tcrontab_list.in = -1;\n \tcrontab_list.out = dup(fd);\n \tcrontab_list.git_cmd = 0;\n \n-\tif (start_command(&crontab_list))\n-\t\treturn error(_(\"failed to run 'crontab -l'; your system might not support 'cron'\"));\n+\tif (start_command(&crontab_list)) {\n+\t\tresult = error(_(\"failed to run 'crontab -l'; your system might not support 'cron'\"));\n+\t\tgoto out;\n+\t}\n \n \t/* Ignore exit code, as an empty crontab will return error. */\n \tfinish_command(&crontab_list);\n@@ -2386,8 +2404,10 @@ static int crontab_update_schedule(int run_maintenance, int fd)\n \t\tresult = error(_(\"'crontab' died\"));\n \telse\n \t\tfclose(cron_list);\n+\n out:\n \tdelete_tempfile(&tmpedit);\n+\tfree(cmd);\n \treturn result;\n }\n \n@@ -2410,10 +2430,9 @@ static int real_is_systemd_timer_available(void)\n \n static int is_systemd_timer_available(void)\n {\n-\tconst char *cmd = \"systemctl\";\n \tint is_available;\n \n-\tif (get_schedule_cmd(&cmd, &is_available))\n+\tif (get_schedule_cmd(\"systemctl\", &is_available, NULL))\n \t\treturn is_available;\n \n \treturn real_is_systemd_timer_available();\n@@ -2594,9 +2613,10 @@ static int systemd_timer_enable_unit(int enable,\n \t\t\t\t enum schedule_priority schedule,\n \t\t\t\t int minute)\n {\n-\tconst char *cmd = \"systemctl\";\n+\tchar *cmd = NULL;\n \tstruct child_process child = CHILD_PROCESS_INIT;\n \tconst char *frequency = get_frequency(schedule);\n+\tint ret;\n \n \t/*\n \t * Disabling the systemd unit while it is already disabled makes\n@@ -2607,20 +2627,25 @@ static int systemd_timer_enable_unit(int enable,\n \t * On the other hand, enabling a systemd unit which is already enabled\n \t * produces no error.\n \t */\n-\tif (!enable)\n+\tif (!enable) {\n \t\tchild.no_stderr = 1;\n-\telse if (systemd_timer_write_timer_file(schedule, minute))\n-\t\treturn -1;\n+\t} else if (systemd_timer_write_timer_file(schedule, minute)) {\n+\t\tret = -1;\n+\t\tgoto out;\n+\t}\n \n-\tget_schedule_cmd(&cmd, NULL);\n+\tget_schedule_cmd(\"systemctl\", NULL, &cmd);\n \tstrvec_split(&child.args, cmd);\n \tstrvec_pushl(&child.args, \"--user\", enable ? \"enable\" : \"disable\",\n \t\t \"--now\", NULL);\n \tstrvec_pushf(&child.args, SYSTEMD_UNIT_FORMAT, frequency, \"timer\");\n \n-\tif (start_command(&child))\n-\t\treturn error(_(\"failed to start systemctl\"));\n-\tif (finish_command(&child))\n+\tif (start_command(&child)) {\n+\t\tret = error(_(\"failed to start systemctl\"));\n+\t\tgoto out;\n+\t}\n+\n+\tif (finish_command(&child)) {\n \t\t/*\n \t\t * Disabling an already disabled systemd unit makes\n \t\t * systemctl fail.\n@@ -2628,9 +2653,17 @@ static int systemd_timer_enable_unit(int enable,\n \t\t *\n \t\t * Enabling an enabled systemd unit doesn't fail.\n \t\t */\n-\t\tif (enable)\n-\t\t\treturn error(_(\"failed to run systemctl\"));\n-\treturn 0;\n+\t\tif (enable) {\n+\t\t\tret = error(_(\"failed to run systemctl\"));\n+\t\t\tgoto out;\n+\t\t}\n+\t}\n+\n+\tret = 0;\n+\n+out:\n+\tfree(cmd);\n+\treturn ret;\n }\n \n /*\ndiff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\nindex abae7a97546..c62c42848a2 100755\n--- a/t/t7900-maintenance.sh\n+++ b/t/t7900-maintenance.sh\n@@ -2,6 +2,7 @@\n \n test_description='git maintenance builtin'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n GIT_TEST_COMMIT_GRAPH=0\n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"502867","messageId":"2a23df9a6869f58a231cfe4947b322690d48cb1a.1726484308.git.ps@pks.im","threadId":"62120","inReplyTo":"cover.1726484308.git.ps@pks.im","subject":"[PATCH 22/23] revision: fix leaking parents when simplifying commits","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-16T11:46:28Z","receivedAt":"2024-09-16T11:46:30Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When simplifying commits, e.g. because they are treesame with their\nparents, we unset the commit's parent pointers but never free them. Plug\nthe resulting memory leaks.\n\nSigned-off-by: Patrick Steinhardt \n---\n revision.c | 5 +++++\n t/t1414-reflog-walk.sh | 1 +\n t/t5310-pack-bitmaps.sh | 1 +\n t/t5326-multi-pack-bitmaps.sh | 2 ++\n t/t6004-rev-list-path-optim.sh | 1 +\n t/t6019-rev-list-ancestry-path.sh | 1 +\n t/t6111-rev-list-treesame.sh | 1 +\n 7 files changed, 12 insertions(+)\n\ndiff --git a/revision.c b/revision.c\nindex 2d7ad2bddff..e79f39e5555 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1071,7 +1071,11 @@ static void try_to_simplify_commit(struct rev_info *revs, struct commit *commit)\n \t\t\t\t\tts->treesame[nth_parent] = 1;\n \t\t\t\tcontinue;\n \t\t\t}\n+\n+\t\t\tfree_commit_list(parent->next);\n \t\t\tparent->next = NULL;\n+\t\t\twhile (commit->parents != parent)\n+\t\t\t\tpop_commit(&commit->parents);\n \t\t\tcommit->parents = parent;\n \n \t\t\t/*\n@@ -1103,6 +1107,7 @@ static void try_to_simplify_commit(struct rev_info *revs, struct commit *commit)\n \t\t\t\t\tdie(\"cannot simplify commit %s (invalid %s)\",\n \t\t\t\t\t oid_to_hex(&commit->object.oid),\n \t\t\t\t\t oid_to_hex(&p->object.oid));\n+\t\t\t\tfree_commit_list(p->parents);\n \t\t\t\tp->parents = NULL;\n \t\t\t}\n \t\t/* fallthrough */\ndiff --git a/t/t1414-reflog-walk.sh b/t/t1414-reflog-walk.sh\nindex be6c3f472c1..49d28166da0 100755\n--- a/t/t1414-reflog-walk.sh\n+++ b/t/t1414-reflog-walk.sh\n@@ -4,6 +4,7 @@ test_description='various tests of reflog walk (log -g) behavior'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'set up some reflog entries' '\ndiff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh\nindex a6de7c57643..7044c7d7c6d 100755\n--- a/t/t5310-pack-bitmaps.sh\n+++ b/t/t5310-pack-bitmaps.sh\n@@ -2,6 +2,7 @@\n \n test_description='exercise basic bitmap functionality'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-bitmap.sh\n \ndiff --git a/t/t5326-multi-pack-bitmaps.sh b/t/t5326-multi-pack-bitmaps.sh\nindex 832b92619c0..6eaa692f33b 100755\n--- a/t/t5326-multi-pack-bitmaps.sh\n+++ b/t/t5326-multi-pack-bitmaps.sh\n@@ -1,6 +1,8 @@\n #!/bin/sh\n \n test_description='exercise basic multi-pack bitmap functionality'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n . \"${TEST_DIRECTORY}/lib-bitmap.sh\"\n \ndiff --git a/t/t6004-rev-list-path-optim.sh b/t/t6004-rev-list-path-optim.sh\nindex cd4f420e2a1..5416241edea 100755\n--- a/t/t6004-rev-list-path-optim.sh\n+++ b/t/t6004-rev-list-path-optim.sh\n@@ -16,6 +16,7 @@ test_description='git rev-list trivial path optimization test\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success setup '\ndiff --git a/t/t6019-rev-list-ancestry-path.sh b/t/t6019-rev-list-ancestry-path.sh\nindex 738da23628b..1aabab69568 100755\n--- a/t/t6019-rev-list-ancestry-path.sh\n+++ b/t/t6019-rev-list-ancestry-path.sh\n@@ -29,6 +29,7 @@ test_description='--ancestry-path'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_merge () {\ndiff --git a/t/t6111-rev-list-treesame.sh b/t/t6111-rev-list-treesame.sh\nindex 90ff1416400..f63bc8d3da6 100755\n--- a/t/t6111-rev-list-treesame.sh\n+++ b/t/t6111-rev-list-treesame.sh\n@@ -16,6 +16,7 @@ test_description='TREESAME and limiting'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n note () {\n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"502868","messageId":"57a3a9e9f8d400d3a3f4de1fa39f41ba80e2eda6.1726484308.git.ps@pks.im","threadId":"62120","inReplyTo":"cover.1726484308.git.ps@pks.im","subject":"[PATCH 23/23] diffcore-break: fix leaking filespecs when merging broken pairs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-16T11:46:32Z","receivedAt":"2024-09-16T11:46:37Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When merging file pairs after they have been broken up we queue a new\nfile pair and discard the broken-up ones. The newly-queued file pair\nreuses one filespec of the broken up pairs each, where the respective\nother filespec gets discarded. But we only end up freeing the filespec's\ndata, not the filespec itself, and thus leak memory.\n\nFix these leaks by using `free_filespec()` instead.\n\nSigned-off-by: Patrick Steinhardt \n---\n diffcore-break.c | 4 ++--\n t/t4008-diff-break-rewrite.sh | 2 ++\n t/t4022-diff-rewrite.sh | 1 +\n t/t4023-diff-rename-typechange.sh | 1 +\n t/t4031-diff-rewrite-binary.sh | 1 +\n t/t7524-commit-summary.sh | 2 ++\n 6 files changed, 9 insertions(+), 2 deletions(-)\n\ndiff --git a/diffcore-break.c b/diffcore-break.c\nindex 831b66b5c3e..02735f80c65 100644\n--- a/diffcore-break.c\n+++ b/diffcore-break.c\n@@ -266,8 +266,8 @@ static void merge_broken(struct diff_filepair *p,\n \t * in the resulting tree.\n \t */\n \td->one->rename_used++;\n-\tdiff_free_filespec_data(d->two);\n-\tdiff_free_filespec_data(c->one);\n+\tfree_filespec(d->two);\n+\tfree_filespec(c->one);\n \tfree(d);\n \tfree(c);\n }\ndiff --git a/t/t4008-diff-break-rewrite.sh b/t/t4008-diff-break-rewrite.sh\nindex 562aaf3a2a2..b0ef0026e08 100755\n--- a/t/t4008-diff-break-rewrite.sh\n+++ b/t/t4008-diff-break-rewrite.sh\n@@ -21,6 +21,8 @@ With -B, this should be detected as two complete rewrites.\n \n Further, with -B and -M together, these should turn into two renames.\n '\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-diff.sh ;# test-lib chdir's into trash\n \ndiff --git a/t/t4022-diff-rewrite.sh b/t/t4022-diff-rewrite.sh\nindex 6fed993ea0b..77bc36d5d8f 100755\n--- a/t/t4022-diff-rewrite.sh\n+++ b/t/t4022-diff-rewrite.sh\n@@ -2,6 +2,7 @@\n \n test_description='rewrite diff'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-diff-data.sh\n \ndiff --git a/t/t4023-diff-rename-typechange.sh b/t/t4023-diff-rename-typechange.sh\nindex 787605ce3fd..e6f4fe441e1 100755\n--- a/t/t4023-diff-rename-typechange.sh\n+++ b/t/t4023-diff-rename-typechange.sh\n@@ -2,6 +2,7 @@\n \n test_description='typechange rename detection'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-diff.sh\n \ndiff --git a/t/t4031-diff-rewrite-binary.sh b/t/t4031-diff-rewrite-binary.sh\nindex c4394a27b56..1b8cd3e4c97 100755\n--- a/t/t4031-diff-rewrite-binary.sh\n+++ b/t/t4031-diff-rewrite-binary.sh\n@@ -2,6 +2,7 @@\n \n test_description='rewrite diff on binary file'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n # We must be large enough to meet the MINIMUM_BREAK_SIZE\ndiff --git a/t/t7524-commit-summary.sh b/t/t7524-commit-summary.sh\nindex 47b2f1dc22a..a8fceb6a47c 100755\n--- a/t/t7524-commit-summary.sh\n+++ b/t/t7524-commit-summary.sh\n@@ -1,6 +1,8 @@\n #!/bin/sh\n \n test_description='git commit summary'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'setup' '\n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"502896","messageId":"guzjej2ghio2bxxt6epofcnl57nqaoagm65byox4iwhvkim2oy@qcuyzzsw5que","threadId":"62120","inReplyTo":"e3bed973afacaec801cff1e77aeea6050cb34f57.1726484308.git.ps@pks.im","subject":"Re: [PATCH 01/23] builtin/help: fix dangling reference to `html_path`","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-09-16T16:24:15Z","receivedAt":"2024-09-16T16:25:31Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 24/09/16 01:45PM, Patrick Steinhardt wrote:\n> In `get_html_page_path()` we may end up assigning the return value of\n> `system_path()` to the global `html_path` variable. But as we also\n> assign the returned value to `to_free`, we will deallocate its memory\n> upon returning from the function. Consequently, `html_path` will now\n> point to deallocated memory.\n> \n> Fix this issue by instead assigning the value to a separate local\n> variable.\n> \n> Signed-off-by: Patrick Steinhardt \n> ---\n> builtin/help.c | 13 +++++++------\n> 1 file changed, 7 insertions(+), 6 deletions(-)\n> \n> diff --git a/builtin/help.c b/builtin/help.c\n> index dc1fbe2b986..282ea5721fa 100644\n> --- a/builtin/help.c\n> +++ b/builtin/help.c\n> @@ -513,23 +513,24 @@ static void show_info_page(const char *page)\n> static void get_html_page_path(struct strbuf *page_path, const char *page)\n> {\n> \tstruct stat st;\n> +\tconst char *path = html_path;\n> \tchar *to_free = NULL;\n> \n> -\tif (!html_path)\n> -\t\thtml_path = to_free = system_path(GIT_HTML_PATH);\n> +\tif (!path)\n> +\t\tpath = to_free = system_path(GIT_HTML_PATH);\n\nPreviously, `html_path` was being assigned the return value of\n`system_path()` and consequently escaping this scope even though the\nvalue was being freed. There is no reason to modify the global value to\nbegin with so we instead assign the value to a local variable. Makes\nsense.\n\n> \n> \t/*\n> \t * Check that the page we're looking for exists.\n> \t */\n> -\tif (!strstr(html_path, \"://\")) {\n> -\t\tif (stat(mkpath(\"%s/%s.html\", html_path, page), &st)\n> +\tif (!strstr(path, \"://\")) {\n> +\t\tif (stat(mkpath(\"%s/%s.html\", path, page), &st)\n> \t\t || !S_ISREG(st.st_mode))\n> \t\t\tdie(\"'%s/%s.html': documentation file not found.\",\n> -\t\t\t\thtml_path, page);\n> +\t\t\t\tpath, page);\n> \t}\n> \n> \tstrbuf_init(page_path, 0);\n> -\tstrbuf_addf(page_path, \"%s/%s.html\", html_path, page);\n> +\tstrbuf_addf(page_path, \"%s/%s.html\", path, page);\n> \tfree(to_free);\n> }\n> \n> -- \n> 2.46.0.551.gc5ee8f2d1c.dirty\n> \n> \n"},{"id":"502901","messageId":"l5aljv4zlvkfpjsizofsypgfaxdzkihwghd3voxin5oxibuixz@fesroo5tihzi","threadId":"62120","inReplyTo":"d088703d317a8598e1cc4eb068234c105cdeffe6.1726484308.git.ps@pks.im","subject":"Re: [PATCH 08/23] builtin/submodule--helper: fix leaking remote ref on errors","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-09-16T18:51:21Z","receivedAt":"2024-09-16T18:52:39Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 24/09/16 01:45PM, Patrick Steinhardt wrote:\n> When `update_submodule()` fails we return with `die_message()`.\n> Curiously enough, this causes a memory leak because we use the\n> `run_process_parallel()` interfaces here, which swap out the die\n> routine.\n\nNaive question, is `update_submodule()` itself being run in parallel\nhere? Is that why the die routine gets swapped out so a child process\ndying is handled differently? Also is it correct to say leaks are not\nconsidered when we \"die\" normally? \n\n> Fix the leak by freeing the remote ref.\n> \n> Signed-off-by: Patrick Steinhardt \n"},{"id":"502948","messageId":"ZulXjXSozNrXgMUM@pks.im","threadId":"62120","inReplyTo":"l5aljv4zlvkfpjsizofsypgfaxdzkihwghd3voxin5oxibuixz@fesroo5tihzi","subject":"Re: [PATCH 08/23] builtin/submodule--helper: fix leaking remote ref on errors","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-17T10:19:05Z","receivedAt":"2024-09-17T10:19:09Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Sep 16, 2024 at 01:51:21PM -0500, Justin Tobler wrote:\n> On 24/09/16 01:45PM, Patrick Steinhardt wrote:\n> > When `update_submodule()` fails we return with `die_message()`.\n> > Curiously enough, this causes a memory leak because we use the\n> > `run_process_parallel()` interfaces here, which swap out the die\n> > routine.\n> \n> Naive question, is `update_submodule()` itself being run in parallel\n> here? Is that why the die routine gets swapped out so a child process\n> dying is handled differently? Also is it correct to say leaks are not\n> considered when we \"die\" normally? \n\nHm. Revisiting this patch: my analysis was wrong. It's not the parallel\nsubsystem that swaps out `die()`, but it's the fact that we call\n`die_message()`, which actually doesn't die. It really only prints the\nmessage you would see when we call `die()`, nothing more.\n\nI'll amend the commit message and send out the amended version once\nthere is more feedback to address.\n\nPatrick\n"},{"id":"503064","messageId":"xmqq34lv8sw5.fsf@gitster.g","threadId":"62120","inReplyTo":"2a23df9a6869f58a231cfe4947b322690d48cb1a.1726484308.git.ps@pks.im","subject":"Re: [PATCH 22/23] revision: fix leaking parents when simplifying commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-19T17:17:46Z","receivedAt":"2024-09-19T17:17:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> When simplifying commits, e.g. because they are treesame with their\n> parents, we unset the commit's parent pointers but never free them. Plug\n> the resulting memory leaks.\n>\n> Signed-off-by: Patrick Steinhardt \n> ---\n> revision.c | 5 +++++\n> t/t1414-reflog-walk.sh | 1 +\n> t/t5310-pack-bitmaps.sh | 1 +\n> t/t5326-multi-pack-bitmaps.sh | 2 ++\n> t/t6004-rev-list-path-optim.sh | 1 +\n> t/t6019-rev-list-ancestry-path.sh | 1 +\n> t/t6111-rev-list-treesame.sh | 1 +\n> 7 files changed, 12 insertions(+)\n>\n> diff --git a/revision.c b/revision.c\n> index 2d7ad2bddff..e79f39e5555 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -1071,7 +1071,11 @@ static void try_to_simplify_commit(struct rev_info *revs, struct commit *commit)\n> \t\t\t\t\tts->treesame[nth_parent] = 1;\n> \t\t\t\tcontinue;\n> \t\t\t}\n> +\n> +\t\t\tfree_commit_list(parent->next);\n> \t\t\tparent->next = NULL;\n\nWe have been walking commit->parents linked list, \"parent\" is the\ncurrent parent we are looking at. We are simplifying the history\nand decided that later parents are not needed, hence we discard the\nremaining commit_list entries starting from parents->next. But we\ndidn't discard; we just updated parent->next pointer and made the\npointed data unreachable, leaking. So we free_commit_list().\n\n> +\t\t\twhile (commit->parents != parent)\n> +\t\t\t\tpop_commit(&commit->parents);\n> \t\t\tcommit->parents = parent;\n\nAnd this is the other direction, discarding the parents before the\ncurrent one we are looking at.\n\nOf course it makes me wonder if we can just free_commit_list() the\nwhole thing and then add the current parent commit (in \"p\", which\nwas taken with \"p = parent->item\" before) as the sole parent, with\nsomething like\n\n\tfree_commit_list(commit->parents);\n commit->parents = NULL;\n /* parent = */ commit_list_insert(p, &commit->parents);\n\nto replace the 5-line (2 to discard later parents, 3 to discard\nearlier ones) code, but I do not think it becomes particularly\neasier to read, so let's drop that.\n\n> @@ -1103,6 +1107,7 @@ static void try_to_simplify_commit(struct rev_info *revs, struct commit *commit)\n> \t\t\t\t\tdie(\"cannot simplify commit %s (invalid %s)\",\n> \t\t\t\t\t oid_to_hex(&commit->object.oid),\n> \t\t\t\t\t oid_to_hex(&p->object.oid));\n> +\t\t\t\tfree_commit_list(p->parents);\n> \t\t\t\tp->parents = NULL;\n> \t\t\t}\n> \t\t/* fallthrough */\n\nLooking good.\n\nThanks.\n"},{"id":"503086","messageId":"xmqqwmj779u9.fsf@gitster.g","threadId":"62120","inReplyTo":"cover.1726484308.git.ps@pks.im","subject":"Re: [PATCH 00/23] Memory leak fixes (pt.7)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-19T18:54:38Z","receivedAt":"2024-09-19T18:54:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> The patch series is built on top of ed155187b4 (Sync with Git 2.46.1,\n> 2024-09-13) with ps/leakfixes-part-6 at 46f6ca2a68 (builtin/repack: fix\n> leaking keep-pack list, 2024-09-05) merged into it.\n\nI haven't said this, but I really appreciate contributors (like you)\npacing their series submission perfectly with the rate of related\ntopics in flight solidifying. If people hold off sending new and\ndependent topics for too long, that would waste available review and\ntesting bandwidth, but if people send them while the other topics\nthat they depend on are not yet in a good shape, it would require\nconflict resolutions that may get wasted when the depended topics\nhave to be updated.\n\nI've been trying to be more explicit in the \"What's cooking\" report\nwhat my evaluation of each topic's \"doneness\" is, to help contributors\nwho need to send new topics that may conflict with the topics that\nare already cooking, and I hope it helped in your (and other\ncontributors') pacing their submissions to help the process run more\nsmoothly.\n\nTHanks.\n"},{"id":"503142","messageId":"xmqqfrpu46oy.fsf@gitster.g","threadId":"62120","inReplyTo":"d5c9cccb8291a1941e6ee1bc8023232e8d5384de.1726484308.git.ps@pks.im","subject":"Re: [PATCH 09/23] dir: fix off by one errors for ignored and untracked entries","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-20T16:43:09Z","receivedAt":"2024-09-20T16:43:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> In `treat_directory()` we perform some logic to handle ignored and\n> untracked entries. When populating a directory with entries we first\n> save the current number of ignored/untracked entries and then populate\n> new entries at the end of our arrays that keep track of those entries.\n> When we figure out that all entries have been ignored/are untracked we\n> then remove this tail of entries from those vectors again. But there is\n> an off by one error in both paths that causes us to not free the first\n> ignored and untracked entries, respectively.\n>\n> Fix these off-by-one errors to plug the resulting leak. While at it,\n> massage the code a bit to match our modern code style.\n>\n> Signed-off-by: Patrick Steinhardt \n> ---\n> dir.c | 6 ++----\n> t/t3011-common-prefixes-and-directory-traversal.sh | 1 +\n> t/t7061-wtstatus-ignore.sh | 1 +\n> t/t7521-ignored-mode.sh | 1 +\n> 4 files changed, 5 insertions(+), 4 deletions(-)\n>\n> diff --git a/dir.c b/dir.c\n> index 5a23376bdae..787bcb7a1a4 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -2135,8 +2135,7 @@ static enum path_treatment treat_directory(struct dir_struct *dir,\n> \t\t\t */\n> \t\t\tstate = path_none;\n> \t\t} else {\n> -\t\t\tint i;\n> -\t\t\tfor (i = old_ignored_nr + 1; iignored_nr; ++i)\n> +\t\t\tfor (int i = old_ignored_nr; i < dir->ignored_nr; i++)\n> \t\t\t\tFREE_AND_NULL(dir->ignored[i]);\n> \t\t\tdir->ignored_nr = old_ignored_nr;\n\nSo the only effect this bug had was that the entry in the array at\nthe old_ignored_nr did not get freed, even though the code correctly\nupdated that the length of the dir->ignored[] after truncation. I\nspeculated, before looking at the code change but only from what was\nin the proposed log message, that the resulting array ended up to\nhave an extra entry, or to lose an entry, due to miscounting, but\nthat is not what is going on. If the bug's nature was that we\nscrewed up the length of the updated array, somebody would certainly\nhave noticed a bug in the end-user observable behaviour that a path\nthat should be ignored is not getting ignored or the other way\naround. Interesting.\n\nI would imagine this is something hard to spot with code inspection\nalone, yet fairly easy for the leak checker to notice. I am happy\nto see that it does really help to have good code coverage in the\ntest suite.\n\nThanks.\n"},{"id":"503144","messageId":"xmqq4j6a45wr.fsf@gitster.g","threadId":"62120","inReplyTo":"747c9a76a288d6fbfad85203e0bccaaf4b6db282.1726484308.git.ps@pks.im","subject":"Re: [PATCH 10/23] builtin/pull: fix leaking \"ff\" option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-20T17:00:04Z","receivedAt":"2024-09-20T17:00:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> The `opt_ff` field gets populated either via `OPT_PASSTHRU` via\n> `config_get_ff()` or when `--rebase` is passed. So we sometimes end up\n> overriding the value in `opt_ff` with another value, but we do not free\n> the old value, causing a memory leak.\n\nGood eyes.\n\nI have to wonder if it will come back and bite us again that\nOPT_PASSTHRU does not pass through but creates a new copy, while\nOPT_STRING borrows from the argv[]. I guess that we could check in\nparse_options_check() if the address of the same variable is passed\nto both OPT_PASSTHRU (giving it a piece of memory we are responsible\nfor freeing) and OPT_STRING (giving it a piece of memory borrowed\nfrom argv[], and would trigger a failure if given to free()).\n\nIn an extremely longer run, I wonder if it becomes necessary for us\nto also make OPT_STRING to make a copy (which means strings that\ncome from the configuration and from parsing the command line\narguments are all allocated ones that we are responsible to free).\n\nThanks.\n"},{"id":"503145","messageId":"xmqqy13m2qd2.fsf@gitster.g","threadId":"62120","inReplyTo":"330b6c52a0a99ba8fa18378310250e5b617d40c2.1726484308.git.ps@pks.im","subject":"Re: [PATCH 12/23] parse-options: free previous value of `OPTION_FILENAME`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-20T17:21:13Z","receivedAt":"2024-09-20T17:21:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> The `OPTION_FILENAME` option always assigns either an allocated string\n> or `NULL` to the value. In case it is passed multiple times it does not\n> know to free the previous value though, which causes a memory leak.\n\nOur earlier attitude was \"the user gave --filename twice from the\ncommand line, fully knowing that the last one wins rule will make\nthe earlier one have no effect; not worth worrying about\".\n\nBut defining the memory ownership rules and make sure the\nlibrary-ish part of the codebase honors these rules is absolutely\nthe right thing to do.\n\nWe saw earlier that OPT_PASSTHRU correctly frees the original value\nbefore assigning the new value, and this makes OPT_FILENAME do the\nsame thing. Nice.\n"},{"id":"503146","messageId":"xmqqr09e2q0m.fsf@gitster.g","threadId":"62120","inReplyTo":"a5f3931eee1e49c60e1d5be214263aeaf3d5c65f.1726484308.git.ps@pks.im","subject":"Re: [PATCH 14/23] builtin/repack: fix leaking configuration","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-20T17:28:41Z","receivedAt":"2024-09-20T17:28:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> When repacking, we assemble git-pack-objects(1) arguments both for the\n> \"normal\" pack and for the cruft pack. This configuration gets populated\n> with a bunch of `OPT_PASSTHRU` options that we end up passing to the\n> child process. These options are allocated, but never free'd.\n>\n> Create a new `pack_objects_args_release()` function that releases the\n> memory for us and call it for both sets of options.\n>\n> Signed-off-by: Patrick Steinhardt \n> ---\n> builtin/repack.c | 57 ++++++++++++++++++++++++++---------\n> t/t5329-pack-objects-cruft.sh | 2 ++\n> t/t7700-repack.sh | 1 +\n> 3 files changed, 45 insertions(+), 15 deletions(-)\n>\n> diff --git a/builtin/repack.c b/builtin/repack.c\n> index 3ee8cfa732f..c31d5653f1f 100644\n> --- a/builtin/repack.c\n> +++ b/builtin/repack.c\n> @@ -85,17 +85,34 @@ static int repack_config(const char *var, const char *value,\n> \t\trun_update_server_info = git_config_bool(var, value);\n> \t\treturn 0;\n> \t}\n> -\tif (!strcmp(var, \"repack.cruftwindow\"))\n> +\tif (!strcmp(var, \"repack.cruftwindow\")) {\n> +\t\tfree(cruft_po_args->window);\n> \t\treturn git_config_string(&cruft_po_args->window, var, value);\n\nSurely, as git_config_string() gives an allocated string, the\nownership rules should be that the receiving variable/struct member\nis responsible for properly freeing it, which means that the current\nvalue must be discarded before the new value is taken. Looking\ngood.\n\nI wonder if any of these targets (like the cruft_po_args->window we\nsee here) can be updated by parsing the command line arguments (with\nOPT_STRING)? It turns out that this worry is not unfounded but you\nalready covered it ...\n\n> \t\tif (!cruft_po_args.window)\n> -\t\t\tcruft_po_args.window = po_args.window;\n> +\t\t\tcruft_po_args.window = xstrdup_or_null(po_args.window);\n\n... here, so we are safe.\n\nLooking good.\n\nThanks.\n"},{"id":"503151","messageId":"xmqqed5ei4u7.fsf@gitster.g","threadId":"62120","inReplyTo":"ed4091255cd25e82db01cb05df63b8c6804b6449.1726484308.git.ps@pks.im","subject":"Re: [PATCH 20/23] builtin/maintenance: fix leaking config string","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-20T17:59:28Z","receivedAt":"2024-09-20T17:59:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> When parsing the maintenance strategy from config we allocate a config\n> string, but do not free it after parsing it. Plug this leak by instead\n> using `git_config_get_string_tmp()`, which does not allocate any memory.\n>\n> This leak is exposed by t7900, but plugging it alone does not make the\n> test suite pass.\n>\n> Signed-off-by: Patrick Steinhardt \n> ---\n> builtin/gc.c | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index 7dac9714054..3acfa367ad1 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -1476,9 +1476,9 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts,\n> \n> static void initialize_maintenance_strategy(void)\n> {\n> -\tchar *config_str;\n> +\tconst char *config_str;\n> \n> -\tif (git_config_get_string(\"maintenance.strategy\", &config_str))\n> +\tif (git_config_get_string_tmp(\"maintenance.strategy\", &config_str))\n> \t\treturn;\n\nI am not a huge fan of the name of this function, because many\ncallers use the result as a fairly long-lived string (the primary\nreason why they call them is to avoid having to free() the result at\nthe end), but this new caller's use is very much in line with the\n\"tmp\" in its name. We peek into the configset to learn the value of\nthis string, and check its value against \"incremental\".\n\nLooking good.\n\nThanks.\n"},{"id":"503337","messageId":"ZvJoJ7dbqmhoVdS9@pks.im","threadId":"62120","inReplyTo":"xmqqwmj779u9.fsf@gitster.g","subject":"Re: [PATCH 00/23] Memory leak fixes (pt.7)","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-24T07:20:13Z","receivedAt":"2024-09-24T07:20:20Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Sep 19, 2024 at 11:54:38AM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> \n> > The patch series is built on top of ed155187b4 (Sync with Git 2.46.1,\n> > 2024-09-13) with ps/leakfixes-part-6 at 46f6ca2a68 (builtin/repack: fix\n> > leaking keep-pack list, 2024-09-05) merged into it.\n> \n> I haven't said this, but I really appreciate contributors (like you)\n> pacing their series submission perfectly with the rate of related\n> topics in flight solidifying. If people hold off sending new and\n> dependent topics for too long, that would waste available review and\n> testing bandwidth, but if people send them while the other topics\n> that they depend on are not yet in a good shape, it would require\n> conflict resolutions that may get wasted when the depended topics\n> have to be updated.\n> \n> I've been trying to be more explicit in the \"What's cooking\" report\n> what my evaluation of each topic's \"doneness\" is, to help contributors\n> who need to send new topics that may conflict with the topics that\n> are already cooking, and I hope it helped in your (and other\n> contributors') pacing their submissions to help the process run more\n> smoothly.\n\nThis series of patch series was something where I wasn't immediately\nsure for how well-received it will be. In order to make progress and not\nbe busy with memory leak fixes for the next couple years I had to keep a\nsteady pace that wasn't too slow. But sending out a 20-patch series\nevery other week could feel overwhelming to some folks.\n\nSo I'm happy to hear that this is well-received overall. Thanks, and\nalso thanks for your reviews!\n\nPatrick\n"},{"id":"503338","messageId":"ZvJoM_JbJKPrgHnC@pks.im","threadId":"62120","inReplyTo":"xmqq4j6a45wr.fsf@gitster.g","subject":"Re: [PATCH 10/23] builtin/pull: fix leaking \"ff\" option","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-24T07:20:19Z","receivedAt":"2024-09-24T07:20:24Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Sep 20, 2024 at 10:00:04AM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> \n> > The `opt_ff` field gets populated either via `OPT_PASSTHRU` via\n> > `config_get_ff()` or when `--rebase` is passed. So we sometimes end up\n> > overriding the value in `opt_ff` with another value, but we do not free\n> > the old value, causing a memory leak.\n> \n> Good eyes.\n> \n> I have to wonder if it will come back and bite us again that\n> OPT_PASSTHRU does not pass through but creates a new copy, while\n> OPT_STRING borrows from the argv[]. I guess that we could check in\n> parse_options_check() if the address of the same variable is passed\n> to both OPT_PASSTHRU (giving it a piece of memory we are responsible\n> for freeing) and OPT_STRING (giving it a piece of memory borrowed\n> from argv[], and would trigger a failure if given to free()).\n> \n> In an extremely longer run, I wonder if it becomes necessary for us\n> to also make OPT_STRING to make a copy (which means strings that\n> come from the configuration and from parsing the command line\n> arguments are all allocated ones that we are responsible to free).\n\nFor now it seems like we are fine with `OPT_STRING` not copying the\nvalue, and I haven't yet found a situation where we cannot work around\nthe alloc/don't alloc split. But it certainly does lead to an awkward\ncalling convention in many places that makes the resulting code way\nharder to read than necessary if it did allocate.\n\nI also doubt that avoiding an allocation for these gives us a noticeable\nspeedup. After all we're bounded by the number of arguments passed by\nthe user, so it's not like we would end up dupliciating thousands of\nstrings in the general case.\n\nSo yes, I think that it would simplify things quite a bit when all three\nof them (OPT_PASSTHRU, OPT_STRING, config parsing) were duplicating the\nvalues.\n\nPatrick\n"},{"id":"503494","messageId":"xmqqbk0bo4ye.fsf@gitster.g","threadId":"62120","inReplyTo":"ZulXjXSozNrXgMUM@pks.im","subject":"Re: [PATCH 08/23] builtin/submodule--helper: fix leaking remote ref on errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-25T20:26:17Z","receivedAt":"2024-09-25T20:26:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> On Mon, Sep 16, 2024 at 01:51:21PM -0500, Justin Tobler wrote:\n>> On 24/09/16 01:45PM, Patrick Steinhardt wrote:\n>> > When `update_submodule()` fails we return with `die_message()`.\n>> > Curiously enough, this causes a memory leak because we use the\n>> > `run_process_parallel()` interfaces here, which swap out the die\n>> > routine.\n>> \n>> Naive question, is `update_submodule()` itself being run in parallel\n>> here? Is that why the die routine gets swapped out so a child process\n>> dying is handled differently? Also is it correct to say leaks are not\n>> considered when we \"die\" normally? \n>\n> Hm. Revisiting this patch: my analysis was wrong. It's not the parallel\n> subsystem that swaps out `die()`, but it's the fact that we call\n> `die_message()`, which actually doesn't die. It really only prints the\n> message you would see when we call `die()`, nothing more.\n>\n> I'll amend the commit message and send out the amended version once\n> there is more feedback to address.\n\nSo it has been a week and half since the series was posted and it\nseems that this is the only thing you might want to touch up.\n\nWhat's next? Just have an updated patch [08/23] and nothing else\nand be done with it? A v2 round of 23-patch series hopefully will\nsee somebody other than Justin and I lend an extra set of eyes to\ndouble check before we merge it to 'next'?\n\nThanks.\n\n\n"},{"id":"503512","messageId":"cover.1727351062.git.ps@pks.im","threadId":"62120","inReplyTo":"cover.1726484308.git.ps@pks.im","subject":"[PATCH v2 00/23] Memory leak fixes (pt.7)","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-26T11:45:56Z","receivedAt":"2024-09-26T11:46:06Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis is the second version of another round of memory leak fixes.\nThere's only a single change compared to v1, namely a revised commit\nmessage.\n\nThanks!\n\nPatrick\n\nPatrick Steinhardt (23):\n builtin/help: fix dangling reference to `html_path`\n builtin/help: fix leaking `html_path` when reading config multiple\n times\n git: fix leaking argv when handling builtins\n submodule: fix leaking update strategy\n builtin/submodule--helper: clear child process when not running it\n builtin/submodule--helper: fix leaking error buffer\n t/helper: fix leaking subrepo in nested submodule config helper\n builtin/submodule--helper: fix leaking remote ref on errors\n dir: fix off by one errors for ignored and untracked entries\n builtin/pull: fix leaking \"ff\" option\n diff: fix leaking orderfile option\n parse-options: free previous value of `OPTION_FILENAME`\n diffcore-order: fix leaking buffer when parsing orderfiles\n builtin/repack: fix leaking configuration\n builtin/difftool: plug several trivial memory leaks\n trace2: destroy context stored in thread-local storage\n submodule: fix leaking submodule ODB paths\n grep: fix leaking grep pattern\n promisor-remote: fix leaking partial clone filter\n builtin/maintenance: fix leaking config string\n builtin/maintenance: fix leak in `get_schedule_cmd()`\n revision: fix leaking parents when simplifying commits\n diffcore-break: fix leaking filespecs when merging broken pairs\n\n builtin/difftool.c | 6 +\n builtin/gc.c | 131 +++++++++++-------\n builtin/help.c | 16 ++-\n builtin/pull.c | 11 +-\n builtin/repack.c | 57 ++++++--\n builtin/submodule--helper.c | 26 +++-\n combine-diff.c | 3 +-\n diff.c | 7 +-\n diff.h | 2 +-\n diffcore-break.c | 4 +-\n diffcore-order.c | 19 +--\n dir.c | 6 +-\n git.c | 22 ++-\n grep.c | 2 +-\n parse-options.c | 22 +--\n promisor-remote.c | 2 +\n revision.c | 5 +\n submodule-config.c | 2 +-\n submodule.c | 9 +-\n submodule.h | 6 +-\n t/helper/test-submodule-nested-repo-config.c | 2 +-\n t/t0012-help.sh | 1 +\n t/t1414-reflog-walk.sh | 1 +\n ...common-prefixes-and-directory-traversal.sh | 1 +\n t/t4008-diff-break-rewrite.sh | 2 +\n t/t4022-diff-rewrite.sh | 1 +\n t/t4023-diff-rename-typechange.sh | 1 +\n t/t4031-diff-rewrite-binary.sh | 1 +\n t/t4056-diff-order.sh | 1 +\n t/t4204-patch-id.sh | 1 +\n t/t5310-pack-bitmaps.sh | 1 +\n t/t5326-multi-pack-bitmaps.sh | 2 +\n t/t5329-pack-objects-cruft.sh | 2 +\n t/t6004-rev-list-path-optim.sh | 1 +\n t/t6019-rev-list-ancestry-path.sh | 1 +\n t/t6111-rev-list-treesame.sh | 1 +\n t/t7061-wtstatus-ignore.sh | 1 +\n t/t7406-submodule-update.sh | 1 +\n t/t7407-submodule-foreach.sh | 1 +\n t/t7408-submodule-reference.sh | 2 +\n t/t7411-submodule-config.sh | 1 +\n t/t7420-submodule-set-url.sh | 1 +\n t/t7521-ignored-mode.sh | 1 +\n t/t7524-commit-summary.sh | 2 +\n t/t7601-merge-pull-config.sh | 1 +\n t/t7700-repack.sh | 1 +\n t/t7800-difftool.sh | 1 +\n t/t7814-grep-recurse-submodules.sh | 1 +\n t/t7900-maintenance.sh | 1 +\n trace2/tr2_tls.c | 10 +-\n 50 files changed, 279 insertions(+), 124 deletions(-)\n\nRange-diff against v1:\n 1: e3bed973af = 1: e3bed973af builtin/help: fix dangling reference to `html_path`\n 2: 4a59fe15ae = 2: 4a59fe15ae builtin/help: fix leaking `html_path` when reading config multiple times\n 3: ea3dd851ad = 3: ea3dd851ad git: fix leaking argv when handling builtins\n 4: 7cdd2691b7 = 4: 7cdd2691b7 submodule: fix leaking update strategy\n 5: 0d0964a2be = 5: 0d0964a2be builtin/submodule--helper: clear child process when not running it\n 6: 52d12e034b = 6: 52d12e034b builtin/submodule--helper: fix leaking error buffer\n 7: 96bd7f01d5 = 7: 96bd7f01d5 t/helper: fix leaking subrepo in nested submodule config helper\n 8: d088703d31 ! 8: d5e7a24aac builtin/submodule--helper: fix leaking remote ref on errors\n @@ Metadata\n ## Commit message ##\n builtin/submodule--helper: fix leaking remote ref on errors\n \n - When `update_submodule()` fails we return with `die_message()`.\n - Curiously enough, this causes a memory leak because we use the\n - `run_process_parallel()` interfaces here, which swap out the die\n - routine.\n + When `update_submodule()` fails we return with `die_message()`, which\n + only causes us to print the same message as `die()` would without\n + actually causing the process to die. We don't free memory in that case\n + and thus leak memory.\n \n Fix the leak by freeing the remote ref.\n \n 9: d5c9cccb82 = 9: fca161d389 dir: fix off by one errors for ignored and untracked entries\n10: 747c9a76a2 = 10: 2338b5e2a8 builtin/pull: fix leaking \"ff\" option\n11: 85c0f9e5f5 = 11: cb08db4d37 diff: fix leaking orderfile option\n12: 330b6c52a0 = 12: 650b89bcca parse-options: free previous value of `OPTION_FILENAME`\n13: c975dfe462 = 13: cd79422087 diffcore-order: fix leaking buffer when parsing orderfiles\n14: a5f3931eee = 14: e015d1704b builtin/repack: fix leaking configuration\n15: c79a5118e4 = 15: 7bb07ec2f0 builtin/difftool: plug several trivial memory leaks\n16: 0fb3dc55e5 = 16: 30928eb8f9 trace2: destroy context stored in thread-local storage\n17: f1cb8122d1 = 17: 35f5de5467 submodule: fix leaking submodule ODB paths\n18: 411df7248d = 18: 3d1cece660 grep: fix leaking grep pattern\n19: 690de28bef = 19: da7768bad5 promisor-remote: fix leaking partial clone filter\n20: ed4091255c = 20: 0a72fc83f7 builtin/maintenance: fix leaking config string\n21: 46956bd8fb = 21: 95200b8a76 builtin/maintenance: fix leak in `get_schedule_cmd()`\n22: 2a23df9a68 = 22: 8e7ea54863 revision: fix leaking parents when simplifying commits\n23: 57a3a9e9f8 = 23: 8cbc41425f diffcore-break: fix leaking filespecs when merging broken pairs\n-- \n2.46.2.852.g229c0bf0e5.dirty\n\n"},{"id":"503513","messageId":"e3bed973afacaec801cff1e77aeea6050cb34f57.1727351062.git.ps@pks.im","threadId":"62120","inReplyTo":"cover.1727351062.git.ps@pks.im","subject":"[PATCH v2 01/23] builtin/help: fix dangling reference to `html_path`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-26T11:46:00Z","receivedAt":"2024-09-26T11:46:07Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In `get_html_page_path()` we may end up assigning the return value of\n`system_path()` to the global `html_path` variable. But as we also\nassign the returned value to `to_free`, we will deallocate its memory\nupon returning from the function. Consequently, `html_path` will now\npoint to deallocated memory.\n\nFix this issue by instead assigning the value to a separate local\nvariable.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/help.c | 13 +++++++------\n 1 file changed, 7 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/help.c b/builtin/help.c\nindex dc1fbe2b98..282ea5721f 100644\n--- a/builtin/help.c\n+++ b/builtin/help.c\n@@ -513,23 +513,24 @@ static void show_info_page(const char *page)\n static void get_html_page_path(struct strbuf *page_path, const char *page)\n {\n \tstruct stat st;\n+\tconst char *path = html_path;\n \tchar *to_free = NULL;\n \n-\tif (!html_path)\n-\t\thtml_path = to_free = system_path(GIT_HTML_PATH);\n+\tif (!path)\n+\t\tpath = to_free = system_path(GIT_HTML_PATH);\n \n \t/*\n \t * Check that the page we're looking for exists.\n \t */\n-\tif (!strstr(html_path, \"://\")) {\n-\t\tif (stat(mkpath(\"%s/%s.html\", html_path, page), &st)\n+\tif (!strstr(path, \"://\")) {\n+\t\tif (stat(mkpath(\"%s/%s.html\", path, page), &st)\n \t\t || !S_ISREG(st.st_mode))\n \t\t\tdie(\"'%s/%s.html': documentation file not found.\",\n-\t\t\t\thtml_path, page);\n+\t\t\t\tpath, page);\n \t}\n \n \tstrbuf_init(page_path, 0);\n-\tstrbuf_addf(page_path, \"%s/%s.html\", html_path, page);\n+\tstrbuf_addf(page_path, \"%s/%s.html\", path, page);\n \tfree(to_free);\n }\n \n-- \n2.46.2.852.g229c0bf0e5.dirty\n\n"},{"id":"503514","messageId":"4a59fe15ae30548164d1433d1d96fb2726512cb2.1727351062.git.ps@pks.im","threadId":"62120","inReplyTo":"cover.1727351062.git.ps@pks.im","subject":"[PATCH v2 02/23] builtin/help: fix leaking `html_path` when reading config multiple times","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-26T11:46:03Z","receivedAt":"2024-09-26T11:46:08Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `html_path` variable gets populated via `git_help_config()`, which\nputs an allocated string into it if its value has been configured. We do\nnot clear the old value though, which causes a memory leak in case the\nconfig exists multiple times.\n\nPlug this leak. The leak is exposed by t0012, but plugging it alone is\nnot sufficient to make the test suite pass.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/help.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/help.c b/builtin/help.c\nindex 282ea5721f..2c249cbca4 100644\n--- a/builtin/help.c\n+++ b/builtin/help.c\n@@ -52,7 +52,7 @@ static enum help_action {\n \tHELP_ACTION_CONFIG_SECTIONS_FOR_COMPLETION,\n } cmd_mode;\n \n-static const char *html_path;\n+static char *html_path;\n static int verbose = 1;\n static enum help_format help_format = HELP_FORMAT_NONE;\n static int exclude_guides;\n@@ -409,6 +409,7 @@ static int git_help_config(const char *var, const char *value,\n \tif (!strcmp(var, \"help.htmlpath\")) {\n \t\tif (!value)\n \t\t\treturn config_error_nonbool(var);\n+\t\tfree(html_path);\n \t\thtml_path = xstrdup(value);\n \t\treturn 0;\n \t}\n-- \n2.46.2.852.g229c0bf0e5.dirty\n\n"},{"id":"503515","messageId":"ea3dd851add1eeb5d53063847b73480b5f6245ed.1727351062.git.ps@pks.im","threadId":"62120","inReplyTo":"cover.1727351062.git.ps@pks.im","subject":"[PATCH v2 03/23] git: fix leaking argv when handling builtins","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-26T11:46:06Z","receivedAt":"2024-09-26T11:46:10Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In `handle_builtin()` we may end up creating an ad-hoc argv array in\ncase we see that the command line contains the \"--help\" parameter. In\nthis case we observe two memory leaks though:\n\n - We leak the `struct strvec` itself because we directly exit after\n calling `run_builtin()`, without bothering about any cleanups.\n\n - Even if we free'd that vector we'd end up leaking some of its\n strings because `run_builtin()` will modify the array.\n\nPlug both of these leaks.\n\nSigned-off-by: Patrick Steinhardt \n---\n git.c | 22 +++++++++++++++++++---\n t/t0012-help.sh | 1 +\n 2 files changed, 20 insertions(+), 3 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex 9a618a2740..3c7fabfda2 100644\n--- a/git.c\n+++ b/git.c\n@@ -711,6 +711,7 @@ static void strip_extension(const char **argv)\n static void handle_builtin(int argc, const char **argv)\n {\n \tstruct strvec args = STRVEC_INIT;\n+\tconst char **argv_copy = NULL;\n \tconst char *cmd;\n \tstruct cmd_struct *builtin;\n \n@@ -731,13 +732,28 @@ static void handle_builtin(int argc, const char **argv)\n \t\t}\n \n \t\targc++;\n-\t\targv = args.v;\n+\n+\t\t/*\n+\t\t * `run_builtin()` will modify the argv array, so we need to\n+\t\t * create a shallow copy such that we can free all of its\n+\t\t * strings.\n+\t\t */\n+\t\tCALLOC_ARRAY(argv_copy, argc + 1);\n+\t\tCOPY_ARRAY(argv_copy, args.v, argc);\n+\n+\t\targv = argv_copy;\n \t}\n \n \tbuiltin = get_builtin(cmd);\n-\tif (builtin)\n-\t\texit(run_builtin(builtin, argc, argv));\n+\tif (builtin) {\n+\t\tint ret = run_builtin(builtin, argc, argv);\n+\t\tstrvec_clear(&args);\n+\t\tfree(argv_copy);\n+\t\texit(ret);\n+\t}\n+\n \tstrvec_clear(&args);\n+\tfree(argv_copy);\n }\n \n static void execv_dashed_external(const char **argv)\ndiff --git a/t/t0012-help.sh b/t/t0012-help.sh\nindex 1d273d91c2..9eae0d8356 100755\n--- a/t/t0012-help.sh\n+++ b/t/t0012-help.sh\n@@ -2,6 +2,7 @@\n \n test_description='help'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n configure_help () {\n-- \n2.46.2.852.g229c0bf0e5.dirty\n\n"},{"id":"503516","messageId":"7cdd2691b77d328447855f898540f7efb2a415c3.1727351062.git.ps@pks.im","threadId":"62120","inReplyTo":"cover.1727351062.git.ps@pks.im","subject":"[PATCH v2 04/23] submodule: fix leaking update strategy","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-26T11:46:08Z","receivedAt":"2024-09-26T11:46:13Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We're not freeing the submodule update strategy command. Provide a\nhelper function that does this for us and call it in\n`update_data_release()`.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/submodule--helper.c | 1 +\n submodule-config.c | 2 +-\n submodule.c | 5 +++++\n submodule.h | 6 ++++--\n t/t7406-submodule-update.sh | 1 +\n 5 files changed, 12 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex bf8f9a4012..ed05dc5134 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2021,6 +2021,7 @@ struct update_data {\n static void update_data_release(struct update_data *ud)\n {\n \tfree(ud->displaypath);\n+\tsubmodule_update_strategy_release(&ud->update_strategy);\n \tmodule_list_release(&ud->list);\n }\n \ndiff --git a/submodule-config.c b/submodule-config.c\nindex c8f2bb2bdd..471637a725 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -95,7 +95,7 @@ static void free_one_config(struct submodule_entry *entry)\n \tfree((void *) entry->config->branch);\n \tfree((void *) entry->config->url);\n \tfree((void *) entry->config->ignore);\n-\tfree((void *) entry->config->update_strategy.command);\n+\tsubmodule_update_strategy_release(&entry->config->update_strategy);\n \tfree(entry->config);\n }\n \ndiff --git a/submodule.c b/submodule.c\nindex 97d0d47b56..0e67984d77 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -424,6 +424,11 @@ int parse_submodule_update_strategy(const char *value,\n \treturn 0;\n }\n \n+void submodule_update_strategy_release(struct submodule_update_strategy *strategy)\n+{\n+\tfree((char *) strategy->command);\n+}\n+\n const char *submodule_update_type_to_string(enum submodule_update_type type)\n {\n \tswitch (type) {\ndiff --git a/submodule.h b/submodule.h\nindex b50d29eba4..4deb1b5f84 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -41,6 +41,10 @@ struct submodule_update_strategy {\n \t.type = SM_UPDATE_UNSPECIFIED, \\\n }\n \n+int parse_submodule_update_strategy(const char *value,\n+\t\t\t\t struct submodule_update_strategy *dst);\n+void submodule_update_strategy_release(struct submodule_update_strategy *strategy);\n+\n int is_gitmodules_unmerged(struct index_state *istate);\n int is_writing_gitmodules_ok(void);\n int is_staging_gitmodules_ok(struct index_state *istate);\n@@ -70,8 +74,6 @@ void die_in_unpopulated_submodule(struct index_state *istate,\n void die_path_inside_submodule(struct index_state *istate,\n \t\t\t const struct pathspec *ps);\n enum submodule_update_type parse_submodule_update_type(const char *value);\n-int parse_submodule_update_strategy(const char *value,\n-\t\t\t\t struct submodule_update_strategy *dst);\n const char *submodule_update_type_to_string(enum submodule_update_type type);\n void handle_ignore_submodules_arg(struct diff_options *, const char *);\n void show_submodule_diff_summary(struct diff_options *o, const char *path,\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex 297c6c3b5c..0f0c86f9cb 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -12,6 +12,7 @@ submodule and \"git submodule update --rebase/--merge\" does not detach the HEAD.\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n \n-- \n2.46.2.852.g229c0bf0e5.dirty\n\n"},{"id":"503517","messageId":"0d0964a2bec7c6e6375a65a5ea9dc79a18ad325c.1727351062.git.ps@pks.im","threadId":"62120","inReplyTo":"cover.1727351062.git.ps@pks.im","subject":"[PATCH v2 05/23] builtin/submodule--helper: clear child process when not running it","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-26T11:46:11Z","receivedAt":"2024-09-26T11:46:16Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In `runcommand_in_submodule_cb()` we may end up not executing the child\ncommand when `argv` is empty. But we still populate the command with\nenvironment variables and other things, which needs cleanup. This leads\nto a memory leak because we do not call `finish_command()`.\n\nFix this by clearing the child process when we don't execute it.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/submodule--helper.c | 10 +++++++---\n t/t7407-submodule-foreach.sh | 1 +\n 2 files changed, 8 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex ed05dc5134..fd1b679408 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -363,9 +363,13 @@ static void runcommand_in_submodule_cb(const struct cache_entry *list_item,\n \tif (!info->quiet)\n \t\tprintf(_(\"Entering '%s'\\n\"), displaypath);\n \n-\tif (info->argv[0] && run_command(&cp))\n-\t\tdie(_(\"run_command returned non-zero status for %s\\n.\"),\n-\t\t\tdisplaypath);\n+\tif (info->argv[0]) {\n+\t\tif (run_command(&cp))\n+\t\t\tdie(_(\"run_command returned non-zero status for %s\\n.\"),\n+\t\t\t displaypath);\n+\t} else {\n+\t\tchild_process_clear(&cp);\n+\t}\n \n \tif (info->recursive) {\n \t\tstruct child_process cpr = CHILD_PROCESS_INIT;\ndiff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh\nindex 8d7b234beb..9f68893261 100755\n--- a/t/t7407-submodule-foreach.sh\n+++ b/t/t7407-submodule-foreach.sh\n@@ -12,6 +12,7 @@ that are currently checked out.\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n \n-- \n2.46.2.852.g229c0bf0e5.dirty\n\n"},{"id":"503518","messageId":"52d12e034b8104a0283958ada013b028322ce421.1727351062.git.ps@pks.im","threadId":"62120","inReplyTo":"cover.1727351062.git.ps@pks.im","subject":"[PATCH v2 06/23] builtin/submodule--helper: fix leaking error buffer","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-26T11:46:14Z","receivedAt":"2024-09-26T11:46:21Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Fix leaking error buffer when `compute_alternate_path()` fails.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/submodule--helper.c | 2 ++\n t/t7408-submodule-reference.sh | 2 ++\n 2 files changed, 4 insertions(+)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex fd1b679408..ff1376f69f 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1621,6 +1621,8 @@ static int add_possible_reference_from_superproject(\n \t\t\t\t; /* nothing */\n \t\t\t}\n \t\t}\n+\n+\t\tstrbuf_release(&err);\n \t\tstrbuf_release(&sb);\n \t}\n \ndiff --git a/t/t7408-submodule-reference.sh b/t/t7408-submodule-reference.sh\nindex d6040e0a33..7e1afa9ce4 100755\n--- a/t/t7408-submodule-reference.sh\n+++ b/t/t7408-submodule-reference.sh\n@@ -4,6 +4,8 @@\n #\n \n test_description='test clone --reference'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n base_dir=$(pwd)\n-- \n2.46.2.852.g229c0bf0e5.dirty\n\n"},{"id":"503519","messageId":"96bd7f01d5d1800a0d0520c0d790cc524e8b4c8c.1727351062.git.ps@pks.im","threadId":"62120","inReplyTo":"cover.1727351062.git.ps@pks.im","subject":"[PATCH v2 07/23] t/helper: fix leaking subrepo in nested submodule config helper","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-26T11:46:18Z","receivedAt":"2024-09-26T11:46:23Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In the \"submodule-nested-repo-config\" helper we create a submodule\nrepository and print its configuration. We do not clear the repo,\ncausing a memory leak. Plug it.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/helper/test-submodule-nested-repo-config.c | 2 +-\n t/t7411-submodule-config.sh | 1 +\n 2 files changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/t/helper/test-submodule-nested-repo-config.c b/t/helper/test-submodule-nested-repo-config.c\nindex 6ca069ce63..6dce957153 100644\n--- a/t/helper/test-submodule-nested-repo-config.c\n+++ b/t/helper/test-submodule-nested-repo-config.c\n@@ -29,6 +29,6 @@ int cmd__submodule_nested_repo_config(int argc, const char **argv)\n \tprint_config_from_gitmodules(&subrepo, argv[2]);\n \n \tsubmodule_free(the_repository);\n-\n+\trepo_clear(&subrepo);\n \treturn 0;\n }\ndiff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh\nindex 31271f8e0a..af0de496e0 100755\n--- a/t/t7411-submodule-config.sh\n+++ b/t/t7411-submodule-config.sh\n@@ -10,6 +10,7 @@ from the database and from the worktree works.\n '\n \n TEST_NO_CREATE_REPO=1\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'setup' '\n-- \n2.46.2.852.g229c0bf0e5.dirty\n\n"},{"id":"503520","messageId":"d5e7a24aac145f1018f0dbc34e37204fc300ffc2.1727351062.git.ps@pks.im","threadId":"62120","inReplyTo":"cover.1727351062.git.ps@pks.im","subject":"[PATCH v2 08/23] builtin/submodule--helper: fix leaking remote ref on errors","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-26T11:46:21Z","receivedAt":"2024-09-26T11:46:26Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When `update_submodule()` fails we return with `die_message()`, which\nonly causes us to print the same message as `die()` would without\nactually causing the process to die. We don't free memory in that case\nand thus leak memory.\n\nFix the leak by freeing the remote ref.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/submodule--helper.c | 13 +++++++++----\n t/t7420-submodule-set-url.sh | 1 +\n 2 files changed, 10 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex ff1376f69f..a9bd93a785 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2648,15 +2648,20 @@ static int update_submodule(struct update_data *update_data)\n \n \t\tif (!update_data->nofetch) {\n \t\t\tif (fetch_in_submodule(update_data->sm_path, update_data->depth,\n-\t\t\t\t\t 0, NULL))\n+\t\t\t\t\t 0, NULL)) {\n+\t\t\t\tfree(remote_ref);\n \t\t\t\treturn die_message(_(\"Unable to fetch in submodule path '%s'\"),\n \t\t\t\t\t\t update_data->sm_path);\n+\t\t\t}\n \t\t}\n \n \t\tif (repo_resolve_gitlink_ref(the_repository, update_data->sm_path,\n-\t\t\t\t\t remote_ref, &update_data->oid))\n-\t\t\treturn die_message(_(\"Unable to find %s revision in submodule path '%s'\"),\n-\t\t\t\t\t remote_ref, update_data->sm_path);\n+\t\t\t\t\t remote_ref, &update_data->oid)) {\n+\t\t\tret = die_message(_(\"Unable to find %s revision in submodule path '%s'\"),\n+\t\t\t\t\t remote_ref, update_data->sm_path);\n+\t\t\tfree(remote_ref);\n+\t\t\treturn ret;\n+\t\t}\n \n \t\tfree(remote_ref);\n \t}\ndiff --git a/t/t7420-submodule-set-url.sh b/t/t7420-submodule-set-url.sh\nindex bf7f15ee79..d7fe910bbe 100755\n--- a/t/t7420-submodule-set-url.sh\n+++ b/t/t7420-submodule-set-url.sh\n@@ -10,6 +10,7 @@ as expected.\n '\n \n TEST_NO_CREATE_REPO=1\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'setup' '\n-- \n2.46.2.852.g229c0bf0e5.dirty\n\n"},{"id":"503521","messageId":"fca161d389125692a9a8f0f5acb218f94f2b0062.1727351062.git.ps@pks.im","threadId":"62120","inReplyTo":"cover.1727351062.git.ps@pks.im","subject":"[PATCH v2 09/23] dir: fix off by one errors for ignored and untracked entries","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-26T11:46:24Z","receivedAt":"2024-09-26T11:46:28Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In `treat_directory()` we perform some logic to handle ignored and\nuntracked entries. When populating a directory with entries we first\nsave the current number of ignored/untracked entries and then populate\nnew entries at the end of our arrays that keep track of those entries.\nWhen we figure out that all entries have been ignored/are untracked we\nthen remove this tail of entries from those vectors again. But there is\nan off by one error in both paths that causes us to not free the first\nignored and untracked entries, respectively.\n\nFix these off-by-one errors to plug the resulting leak. While at it,\nmassage the code a bit to match our modern code style.\n\nSigned-off-by: Patrick Steinhardt \n---\n dir.c | 6 ++----\n t/t3011-common-prefixes-and-directory-traversal.sh | 1 +\n t/t7061-wtstatus-ignore.sh | 1 +\n t/t7521-ignored-mode.sh | 1 +\n 4 files changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex 5a23376bda..787bcb7a1a 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -2135,8 +2135,7 @@ static enum path_treatment treat_directory(struct dir_struct *dir,\n \t\t\t */\n \t\t\tstate = path_none;\n \t\t} else {\n-\t\t\tint i;\n-\t\t\tfor (i = old_ignored_nr + 1; iignored_nr; ++i)\n+\t\t\tfor (int i = old_ignored_nr; i < dir->ignored_nr; i++)\n \t\t\t\tFREE_AND_NULL(dir->ignored[i]);\n \t\t\tdir->ignored_nr = old_ignored_nr;\n \t\t}\n@@ -2148,8 +2147,7 @@ static enum path_treatment treat_directory(struct dir_struct *dir,\n \t */\n \tif ((dir->flags & DIR_SHOW_IGNORED_TOO) &&\n \t !(dir->flags & DIR_KEEP_UNTRACKED_CONTENTS)) {\n-\t\tint i;\n-\t\tfor (i = old_untracked_nr + 1; inr; ++i)\n+\t\tfor (int i = old_untracked_nr; i < dir->nr; i++)\n \t\t\tFREE_AND_NULL(dir->entries[i]);\n \t\tdir->nr = old_untracked_nr;\n \t}\ndiff --git a/t/t3011-common-prefixes-and-directory-traversal.sh b/t/t3011-common-prefixes-and-directory-traversal.sh\nindex 3da5b2b6e7..69e44c387f 100755\n--- a/t/t3011-common-prefixes-and-directory-traversal.sh\n+++ b/t/t3011-common-prefixes-and-directory-traversal.sh\n@@ -2,6 +2,7 @@\n \n test_description='directory traversal handling, especially with common prefixes'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'setup' '\ndiff --git a/t/t7061-wtstatus-ignore.sh b/t/t7061-wtstatus-ignore.sh\nindex 2f9bea9793..64145a05b1 100755\n--- a/t/t7061-wtstatus-ignore.sh\n+++ b/t/t7061-wtstatus-ignore.sh\n@@ -2,6 +2,7 @@\n \n test_description='git-status ignored files'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n cat >expected <<\\EOF\ndiff --git a/t/t7521-ignored-mode.sh b/t/t7521-ignored-mode.sh\nindex a88b02b06e..edce10f998 100755\n--- a/t/t7521-ignored-mode.sh\n+++ b/t/t7521-ignored-mode.sh\n@@ -2,6 +2,7 @@\n \n test_description='git status ignored modes'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'setup initial commit and ignore file' '\n-- \n2.46.2.852.g229c0bf0e5.dirty\n\n"},{"id":"503522","messageId":"2338b5e2a81b4036bf3740265126afddd2fbe6a1.1727351062.git.ps@pks.im","threadId":"62120","inReplyTo":"cover.1727351062.git.ps@pks.im","subject":"[PATCH v2 10/23] builtin/pull: fix leaking \"ff\" option","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-26T11:46:26Z","receivedAt":"2024-09-26T11:46:31Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `opt_ff` field gets populated either via `OPT_PASSTHRU` via\n`config_get_ff()` or when `--rebase` is passed. So we sometimes end up\noverriding the value in `opt_ff` with another value, but we do not free\nthe old value, causing a memory leak.\n\nAdapt the type of the variable to be `char *` and consistently assign\nallocated strings to it such that we can easily free it when it is being\noverridden.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/pull.c | 11 +++++++----\n t/t7601-merge-pull-config.sh | 1 +\n 2 files changed, 8 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 4c54d8196f..5d9d9e467e 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -84,7 +84,7 @@ static const char *opt_squash;\n static const char *opt_commit;\n static const char *opt_edit;\n static const char *cleanup_arg;\n-static const char *opt_ff;\n+static char *opt_ff;\n static const char *opt_verify_signatures;\n static const char *opt_verify;\n static int opt_autostash = -1;\n@@ -1024,8 +1024,10 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \t\t * \"--rebase\" can override a config setting of\n \t\t * pull.ff=only.\n \t\t */\n-\t\tif (opt_rebase >= 0 && opt_ff && !strcmp(opt_ff, \"--ff-only\"))\n-\t\t\topt_ff = \"--ff\";\n+\t\tif (opt_rebase >= 0 && opt_ff && !strcmp(opt_ff, \"--ff-only\")) {\n+\t\t\tfree(opt_ff);\n+\t\t\topt_ff = xstrdup(\"--ff\");\n+\t\t}\n \t}\n \n \tif (opt_rebase < 0)\n@@ -1135,7 +1137,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \n \t\tif (can_ff) {\n \t\t\t/* we can fast-forward this without invoking rebase */\n-\t\t\topt_ff = \"--ff-only\";\n+\t\t\tfree(opt_ff);\n+\t\t\topt_ff = xstrdup(\"--ff-only\");\n \t\t\tret = run_merge();\n \t\t} else {\n \t\t\tret = run_rebase(&newbase, &upstream);\ndiff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh\nindex a94387a75f..7fd8c086af 100755\n--- a/t/t7601-merge-pull-config.sh\n+++ b/t/t7601-merge-pull-config.sh\n@@ -4,6 +4,7 @@ test_description='git merge\n \n Testing pull.* configuration parsing and other things.'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'setup' '\n-- \n2.46.2.852.g229c0bf0e5.dirty\n\n"},{"id":"503523","messageId":"cb08db4d3766e1bbe96d10798be6792f8f9ee19c.1727351062.git.ps@pks.im","threadId":"62120","inReplyTo":"cover.1727351062.git.ps@pks.im","subject":"[PATCH v2 11/23] diff: fix leaking orderfile option","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-26T11:46:29Z","receivedAt":"2024-09-26T11:46:34Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `orderfile` diff option is being assigned via `OPT_FILENAME()`,\nwhich assigns an allocated string to the variable. We never free it\nthough, causing a memory leak.\n\nChange the type of the string to `char *` and free it to plug the leak.\nThis also requires us to use `xstrdup()` to assign the global config to\nit in case it is set.\n\nThis leak is being hit in t7621, but plugging it alone does not make the\ntest suite pass.\n\nSigned-off-by: Patrick Steinhardt \n---\n combine-diff.c | 3 +--\n diff.c | 7 +++++--\n diff.h | 2 +-\n 3 files changed, 7 insertions(+), 5 deletions(-)\n\ndiff --git a/combine-diff.c b/combine-diff.c\nindex 829a44e416..f6b624dc28 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -1393,9 +1393,8 @@ static struct combine_diff_path *find_paths_generic(const struct object_id *oid,\n {\n \tstruct combine_diff_path *paths = NULL;\n \tint i, num_parent = parents->nr;\n-\n \tint output_format = opt->output_format;\n-\tconst char *orderfile = opt->orderfile;\n+\tchar *orderfile = opt->orderfile;\n \n \topt->output_format = DIFF_FORMAT_NO_OUTPUT;\n \t/* tell diff_tree to emit paths in sorted (=tree) order */\ndiff --git a/diff.c b/diff.c\nindex 472479eb10..6555b8a32c 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -441,8 +441,10 @@ int git_diff_ui_config(const char *var, const char *value,\n \t}\n \tif (!strcmp(var, \"diff.wordregex\"))\n \t\treturn git_config_string(&diff_word_regex_cfg, var, value);\n-\tif (!strcmp(var, \"diff.orderfile\"))\n+\tif (!strcmp(var, \"diff.orderfile\")) {\n+\t\tFREE_AND_NULL(diff_order_file_cfg);\n \t\treturn git_config_pathname(&diff_order_file_cfg, var, value);\n+\t}\n \n \tif (!strcmp(var, \"diff.ignoresubmodules\")) {\n \t\tif (!value)\n@@ -4775,7 +4777,7 @@ void repo_diff_setup(struct repository *r, struct diff_options *options)\n \tif (diff_indent_heuristic)\n \t\tDIFF_XDL_SET(options, INDENT_HEURISTIC);\n \n-\toptions->orderfile = diff_order_file_cfg;\n+\toptions->orderfile = xstrdup_or_null(diff_order_file_cfg);\n \n \tif (!options->flags.ignore_submodule_set)\n \t\toptions->flags.ignore_untracked_in_submodules = 1;\n@@ -6727,6 +6729,7 @@ void diff_free(struct diff_options *options)\n \t\tFREE_AND_NULL(options->objfind);\n \t}\n \n+\tFREE_AND_NULL(options->orderfile);\n \tfor (size_t i = 0; i < options->anchors_nr; i++)\n \t\tfree(options->anchors[i]);\n \tFREE_AND_NULL(options->anchors);\ndiff --git a/diff.h b/diff.h\nindex 9901c8ca8c..b95d3c1e83 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -235,7 +235,7 @@ enum diff_submodule_format {\n * diffcore library with.\n */\n struct diff_options {\n-\tconst char *orderfile;\n+\tchar *orderfile;\n \n \t/*\n \t * \"--rotate-to=\" would start showing at and when\n-- \n2.46.2.852.g229c0bf0e5.dirty\n\n"},{"id":"503524","messageId":"650b89bcca8bec87718e791302adcd4aa35b2a68.1727351062.git.ps@pks.im","threadId":"62120","inReplyTo":"cover.1727351062.git.ps@pks.im","subject":"[PATCH v2 12/23] parse-options: free previous value of `OPTION_FILENAME`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-26T11:46:32Z","receivedAt":"2024-09-26T11:46:39Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `OPTION_FILENAME` option always assigns either an allocated string\nor `NULL` to the value. In case it is passed multiple times it does not\nknow to free the previous value though, which causes a memory leak.\n\nRefactor the function to always free the previous value. None of the\nsites where this option is used pass a string constant, so this change\nis safe.\n\nWhile at it, fix the argument of `fix_filename()` to be a string\nconstant. The only reason why it's not is because we use it as an\nin-out-parameter, where the input is a constant and the output is not.\nThis is weird and unnecessary, as we can just return the result instead\nof using the parameter for this.\n\nThis leak is being hit in t7621, but plugging it alone does not make the\ntest suite pass.\n\nSigned-off-by: Patrick Steinhardt \n---\n parse-options.c | 22 ++++++++++++++--------\n 1 file changed, 14 insertions(+), 8 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 30b9e68f8a..33bfba0ed4 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -60,12 +60,12 @@ static enum parse_opt_result get_arg(struct parse_opt_ctx_t *p,\n \treturn 0;\n }\n \n-static void fix_filename(const char *prefix, char **file)\n+static char *fix_filename(const char *prefix, const char *file)\n {\n \tif (!file || !*file)\n-\t\t; /* leave as NULL */\n+\t\treturn NULL;\n \telse\n-\t\t*file = prefix_filename_except_for_dash(prefix, *file);\n+\t\treturn prefix_filename_except_for_dash(prefix, file);\n }\n \n static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,\n@@ -129,18 +129,24 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,\n \t\treturn 0;\n \n \tcase OPTION_FILENAME:\n+\t{\n+\t\tconst char *value;\n+\n+\t\tFREE_AND_NULL(*(char **)opt->value);\n+\n \t\terr = 0;\n+\n \t\tif (unset)\n-\t\t\t*(const char **)opt->value = NULL;\n+\t\t\tvalue = NULL;\n \t\telse if (opt->flags & PARSE_OPT_OPTARG && !p->opt)\n-\t\t\t*(const char **)opt->value = (const char *)opt->defval;\n+\t\t\tvalue = (const char *) opt->defval;\n \t\telse\n-\t\t\terr = get_arg(p, opt, flags, (const char **)opt->value);\n+\t\t\terr = get_arg(p, opt, flags, &value);\n \n \t\tif (!err)\n-\t\t\tfix_filename(p->prefix, (char **)opt->value);\n+\t\t\t*(char **)opt->value = fix_filename(p->prefix, value);\n \t\treturn err;\n-\n+\t}\n \tcase OPTION_CALLBACK:\n \t{\n \t\tconst char *p_arg = NULL;\n-- \n2.46.2.852.g229c0bf0e5.dirty\n\n"},{"id":"503525","messageId":"cd794220878a5bd77e864e293e011d2e300cfd9b.1727351062.git.ps@pks.im","threadId":"62120","inReplyTo":"cover.1727351062.git.ps@pks.im","subject":"[PATCH v2 13/23] diffcore-order: fix leaking buffer when parsing orderfiles","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-26T11:46:37Z","receivedAt":"2024-09-26T11:46:42Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In `prepare_order()` we parse an orderfile and assign it to a global\narray. In order to save on some allocations, we replace newlines with\nNUL characters and then assign pointers into the allocated buffer to\nthat array. This can cause the buffer to be completely unreferenced\nthough in some cases, e.g. because the order file is empty or because we\nhad to use `xmemdupz()` to copy the lines instead of NUL-terminating\nthem.\n\nRefactor the code to always `xmemdupz()` the strings. This is a bit\nsimpler, and it is rather unlikely that saving a handful of allocations\nreally matters. This allows us to release the string buffer and thus\nplug the memory leak.\n\nSigned-off-by: Patrick Steinhardt \n---\n diffcore-order.c | 19 +++++++------------\n t/t4056-diff-order.sh | 1 +\n t/t4204-patch-id.sh | 1 +\n 3 files changed, 9 insertions(+), 12 deletions(-)\n\ndiff --git a/diffcore-order.c b/diffcore-order.c\nindex e7d20ebd2d..912513d3e6 100644\n--- a/diffcore-order.c\n+++ b/diffcore-order.c\n@@ -14,8 +14,7 @@ static void prepare_order(const char *orderfile)\n {\n \tint cnt, pass;\n \tstruct strbuf sb = STRBUF_INIT;\n-\tvoid *map;\n-\tchar *cp, *endp;\n+\tconst char *cp, *endp;\n \tssize_t sz;\n \n \tif (order)\n@@ -24,14 +23,13 @@ static void prepare_order(const char *orderfile)\n \tsz = strbuf_read_file(&sb, orderfile, 0);\n \tif (sz < 0)\n \t\tdie_errno(_(\"failed to read orderfile '%s'\"), orderfile);\n-\tmap = strbuf_detach(&sb, NULL);\n-\tendp = (char *) map + sz;\n+\tendp = sb.buf + sz;\n \n \tfor (pass = 0; pass < 2; pass++) {\n \t\tcnt = 0;\n-\t\tcp = map;\n+\t\tcp = sb.buf;\n \t\twhile (cp < endp) {\n-\t\t\tchar *ep;\n+\t\t\tconst char *ep;\n \t\t\tfor (ep = cp; ep < endp && *ep != '\\n'; ep++)\n \t\t\t\t;\n \t\t\t/* cp to ep has one line */\n@@ -40,12 +38,7 @@ static void prepare_order(const char *orderfile)\n \t\t\telse if (pass == 0)\n \t\t\t\tcnt++;\n \t\t\telse {\n-\t\t\t\tif (*ep == '\\n') {\n-\t\t\t\t\t*ep = 0;\n-\t\t\t\t\torder[cnt] = cp;\n-\t\t\t\t} else {\n-\t\t\t\t\torder[cnt] = xmemdupz(cp, ep - cp);\n-\t\t\t\t}\n+\t\t\t\torder[cnt] = xmemdupz(cp, ep - cp);\n \t\t\t\tcnt++;\n \t\t\t}\n \t\t\tif (ep < endp)\n@@ -57,6 +50,8 @@ static void prepare_order(const char *orderfile)\n \t\t\tALLOC_ARRAY(order, cnt);\n \t\t}\n \t}\n+\n+\tstrbuf_release(&sb);\n }\n \n static int match_order(const char *path)\ndiff --git a/t/t4056-diff-order.sh b/t/t4056-diff-order.sh\nindex aec1d9d1b4..32c5fcb9a2 100755\n--- a/t/t4056-diff-order.sh\n+++ b/t/t4056-diff-order.sh\n@@ -5,6 +5,7 @@ test_description='diff order & rotate'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n create_files () {\ndiff --git a/t/t4204-patch-id.sh b/t/t4204-patch-id.sh\nindex dc8ddb10af..c0a4a02dcf 100755\n--- a/t/t4204-patch-id.sh\n+++ b/t/t4204-patch-id.sh\n@@ -5,6 +5,7 @@ test_description='git patch-id'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'setup' '\n-- \n2.46.2.852.g229c0bf0e5.dirty\n\n"},{"id":"503526","messageId":"e015d1704b0da60fbf3663fb31c19780528bc7e6.1727351062.git.ps@pks.im","threadId":"62120","inReplyTo":"cover.1727351062.git.ps@pks.im","subject":"[PATCH v2 14/23] builtin/repack: fix leaking configuration","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-26T11:46:40Z","receivedAt":"2024-09-26T11:46:44Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When repacking, we assemble git-pack-objects(1) arguments both for the\n\"normal\" pack and for the cruft pack. This configuration gets populated\nwith a bunch of `OPT_PASSTHRU` options that we end up passing to the\nchild process. These options are allocated, but never free'd.\n\nCreate a new `pack_objects_args_release()` function that releases the\nmemory for us and call it for both sets of options.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/repack.c | 57 ++++++++++++++++++++++++++---------\n t/t5329-pack-objects-cruft.sh | 2 ++\n t/t7700-repack.sh | 1 +\n 3 files changed, 45 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 3ee8cfa732..c31d5653f1 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -85,17 +85,34 @@ static int repack_config(const char *var, const char *value,\n \t\trun_update_server_info = git_config_bool(var, value);\n \t\treturn 0;\n \t}\n-\tif (!strcmp(var, \"repack.cruftwindow\"))\n+\tif (!strcmp(var, \"repack.cruftwindow\")) {\n+\t\tfree(cruft_po_args->window);\n \t\treturn git_config_string(&cruft_po_args->window, var, value);\n-\tif (!strcmp(var, \"repack.cruftwindowmemory\"))\n+\t}\n+\tif (!strcmp(var, \"repack.cruftwindowmemory\")) {\n+\t\tfree(cruft_po_args->window_memory);\n \t\treturn git_config_string(&cruft_po_args->window_memory, var, value);\n-\tif (!strcmp(var, \"repack.cruftdepth\"))\n+\t}\n+\tif (!strcmp(var, \"repack.cruftdepth\")) {\n+\t\tfree(cruft_po_args->depth);\n \t\treturn git_config_string(&cruft_po_args->depth, var, value);\n-\tif (!strcmp(var, \"repack.cruftthreads\"))\n+\t}\n+\tif (!strcmp(var, \"repack.cruftthreads\")) {\n+\t\tfree(cruft_po_args->threads);\n \t\treturn git_config_string(&cruft_po_args->threads, var, value);\n+\t}\n \treturn git_default_config(var, value, ctx, cb);\n }\n \n+static void pack_objects_args_release(struct pack_objects_args *args)\n+{\n+\tfree(args->window);\n+\tfree(args->window_memory);\n+\tfree(args->depth);\n+\tfree(args->threads);\n+\tlist_objects_filter_release(&args->filter_options);\n+}\n+\n struct existing_packs {\n \tstruct string_list kept_packs;\n \tstruct string_list non_kept_packs;\n@@ -1152,12 +1169,16 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tconst char *unpack_unreachable = NULL;\n \tint keep_unreachable = 0;\n \tstruct string_list keep_pack_list = STRING_LIST_INIT_NODUP;\n-\tstruct pack_objects_args po_args = {NULL};\n-\tstruct pack_objects_args cruft_po_args = {NULL};\n+\tstruct pack_objects_args po_args = { 0 };\n+\tstruct pack_objects_args cruft_po_args = { 0 };\n \tint write_midx = 0;\n \tconst char *cruft_expiration = NULL;\n \tconst char *expire_to = NULL;\n \tconst char *filter_to = NULL;\n+\tconst char *opt_window = NULL;\n+\tconst char *opt_window_memory = NULL;\n+\tconst char *opt_depth = NULL;\n+\tconst char *opt_threads = NULL;\n \n \tstruct option builtin_repack_options[] = {\n \t\tOPT_BIT('a', NULL, &pack_everything,\n@@ -1191,13 +1212,13 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\t\tN_(\"with -A, do not loosen objects older than this\")),\n \t\tOPT_BOOL('k', \"keep-unreachable\", &keep_unreachable,\n \t\t\t\tN_(\"with -a, repack unreachable objects\")),\n-\t\tOPT_STRING(0, \"window\", &po_args.window, N_(\"n\"),\n+\t\tOPT_STRING(0, \"window\", &opt_window, N_(\"n\"),\n \t\t\t\tN_(\"size of the window used for delta compression\")),\n-\t\tOPT_STRING(0, \"window-memory\", &po_args.window_memory, N_(\"bytes\"),\n+\t\tOPT_STRING(0, \"window-memory\", &opt_window_memory, N_(\"bytes\"),\n \t\t\t\tN_(\"same as the above, but limit memory size instead of entries count\")),\n-\t\tOPT_STRING(0, \"depth\", &po_args.depth, N_(\"n\"),\n+\t\tOPT_STRING(0, \"depth\", &opt_depth, N_(\"n\"),\n \t\t\t\tN_(\"limits the maximum delta depth\")),\n-\t\tOPT_STRING(0, \"threads\", &po_args.threads, N_(\"n\"),\n+\t\tOPT_STRING(0, \"threads\", &opt_threads, N_(\"n\"),\n \t\t\t\tN_(\"limits the maximum number of threads\")),\n \t\tOPT_MAGNITUDE(0, \"max-pack-size\", &po_args.max_pack_size,\n \t\t\t\tN_(\"maximum size of each packfile\")),\n@@ -1224,6 +1245,11 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, prefix, builtin_repack_options,\n \t\t\t\tgit_repack_usage, 0);\n \n+\tpo_args.window = xstrdup_or_null(opt_window);\n+\tpo_args.window_memory = xstrdup_or_null(opt_window_memory);\n+\tpo_args.depth = xstrdup_or_null(opt_depth);\n+\tpo_args.threads = xstrdup_or_null(opt_threads);\n+\n \tif (delete_redundant && repository_format_precious_objects)\n \t\tdie(_(\"cannot delete packs in a precious-objects repo\"));\n \n@@ -1389,13 +1415,13 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\tconst char *pack_prefix = find_pack_prefix(packdir, packtmp);\n \n \t\tif (!cruft_po_args.window)\n-\t\t\tcruft_po_args.window = po_args.window;\n+\t\t\tcruft_po_args.window = xstrdup_or_null(po_args.window);\n \t\tif (!cruft_po_args.window_memory)\n-\t\t\tcruft_po_args.window_memory = po_args.window_memory;\n+\t\t\tcruft_po_args.window_memory = xstrdup_or_null(po_args.window_memory);\n \t\tif (!cruft_po_args.depth)\n-\t\t\tcruft_po_args.depth = po_args.depth;\n+\t\t\tcruft_po_args.depth = xstrdup_or_null(po_args.depth);\n \t\tif (!cruft_po_args.threads)\n-\t\t\tcruft_po_args.threads = po_args.threads;\n+\t\t\tcruft_po_args.threads = xstrdup_or_null(po_args.threads);\n \t\tif (!cruft_po_args.max_pack_size)\n \t\t\tcruft_po_args.max_pack_size = po_args.max_pack_size;\n \n@@ -1547,7 +1573,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tstring_list_clear(&names, 1);\n \texisting_packs_release(&existing);\n \tfree_pack_geometry(&geometry);\n-\tlist_objects_filter_release(&po_args.filter_options);\n+\tpack_objects_args_release(&po_args);\n+\tpack_objects_args_release(&cruft_po_args);\n \n \treturn ret;\n }\ndiff --git a/t/t5329-pack-objects-cruft.sh b/t/t5329-pack-objects-cruft.sh\nindex fc5fedbe9b..445739d06c 100755\n--- a/t/t5329-pack-objects-cruft.sh\n+++ b/t/t5329-pack-objects-cruft.sh\n@@ -1,6 +1,8 @@\n #!/bin/sh\n \n test_description='cruft pack related pack-objects tests'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n objdir=.git/objects\ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex be1188e736..c4c3d1a15d 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -2,6 +2,7 @@\n \n test_description='git repack works correctly'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n . \"${TEST_DIRECTORY}/lib-bitmap.sh\"\n . \"${TEST_DIRECTORY}/lib-midx.sh\"\n-- \n2.46.2.852.g229c0bf0e5.dirty\n\n"},{"id":"503527","messageId":"7bb07ec2f07dbbfac06af039d1f192558150f4f9.1727351062.git.ps@pks.im","threadId":"62120","inReplyTo":"cover.1727351062.git.ps@pks.im","subject":"[PATCH v2 15/23] builtin/difftool: plug several trivial memory leaks","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-26T11:46:43Z","receivedAt":"2024-09-26T11:46:48Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"There are several leaking data structures in git-difftool(1). Plug them.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/difftool.c | 6 ++++++\n t/t7800-difftool.sh | 1 +\n 2 files changed, 7 insertions(+)\n\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex dcc68e190c..1a68ab6699 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -660,6 +660,12 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \tif (fp)\n \t\tfclose(fp);\n \n+\thashmap_clear_and_free(&working_tree_dups, struct working_tree_entry, entry);\n+\thashmap_clear_and_free(&wt_modified, struct path_entry, entry);\n+\thashmap_clear_and_free(&tmp_modified, struct path_entry, entry);\n+\thashmap_clear_and_free(&submodules, struct pair_entry, entry);\n+\thashmap_clear_and_free(&symlinks2, struct pair_entry, entry);\n+\trelease_index(&wtindex);\n \tfree(lbase_dir);\n \tfree(rbase_dir);\n \tstrbuf_release(&info);\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex cc917b257e..f67b9345b8 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -11,6 +11,7 @@ Testing basic diff tool invocation\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n difftool_test_setup ()\n-- \n2.46.2.852.g229c0bf0e5.dirty\n\n"},{"id":"503528","messageId":"30928eb8f9ce4c2e6a82f566ff740c31604b0d5e.1727351062.git.ps@pks.im","threadId":"62120","inReplyTo":"cover.1727351062.git.ps@pks.im","subject":"[PATCH v2 16/23] trace2: destroy context stored in thread-local storage","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-26T11:46:45Z","receivedAt":"2024-09-26T11:46:50Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Each thread may have a specific context in the trace2 subsystem that we\nset up via thread-local storage. We do not set up a destructor for this\ndata though, which means that the context data will leak.\n\nPlug this leak by installing a destructor. This leak is exposed by\nt7814, but plugging it alone does not make the whole test suite pass.\n\nSigned-off-by: Patrick Steinhardt \n---\n trace2/tr2_tls.c | 10 +++++++++-\n 1 file changed, 9 insertions(+), 1 deletion(-)\n\ndiff --git a/trace2/tr2_tls.c b/trace2/tr2_tls.c\nindex 4f75392952..7b023c1bfc 100644\n--- a/trace2/tr2_tls.c\n+++ b/trace2/tr2_tls.c\n@@ -152,11 +152,19 @@ uint64_t tr2tls_absolute_elapsed(uint64_t us)\n \treturn us - tr2tls_us_start_process;\n }\n \n+static void tr2tls_key_destructor(void *payload)\n+{\n+\tstruct tr2tls_thread_ctx *ctx = payload;\n+\tfree((char *)ctx->thread_name);\n+\tfree(ctx->array_us_start);\n+\tfree(ctx);\n+}\n+\n void tr2tls_init(void)\n {\n \ttr2tls_start_process_clock();\n \n-\tpthread_key_create(&tr2tls_key, NULL);\n+\tpthread_key_create(&tr2tls_key, tr2tls_key_destructor);\n \tinit_recursive_mutex(&tr2tls_mutex);\n \n \ttr2tls_thread_main =\n-- \n2.46.2.852.g229c0bf0e5.dirty\n\n"},{"id":"503529","messageId":"35f5de5467f97dad04ae53e6de08b5efc6e2cf41.1727351062.git.ps@pks.im","threadId":"62120","inReplyTo":"cover.1727351062.git.ps@pks.im","subject":"[PATCH v2 17/23] submodule: fix leaking submodule ODB paths","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-26T11:46:48Z","receivedAt":"2024-09-26T11:46:56Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In `add_submodule_odb_by_path()` we add a path into a global string\nlist. The list is initialized with `NODUP`, which means that we do not\npass ownership of strings to the list. But we use `xstrdup()` when we\ninsert a path, with the consequence that the string will never get\nfree'd.\n\nPlug the leak by marking the list as `DUP`. There is only a single\ncallsite where we insert paths anyway, and as explained above that\ncallsite was mishandling the allocation.\n\nThis leak is exposed by t7814, but plugging it does not make the whole\ntest suite pass.\n\nSigned-off-by: Patrick Steinhardt \n---\n submodule.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 0e67984d77..a07debc227 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -175,11 +175,11 @@ void stage_updated_gitmodules(struct index_state *istate)\n \t\tdie(_(\"staging updated .gitmodules failed\"));\n }\n \n-static struct string_list added_submodule_odb_paths = STRING_LIST_INIT_NODUP;\n+static struct string_list added_submodule_odb_paths = STRING_LIST_INIT_DUP;\n \n void add_submodule_odb_by_path(const char *path)\n {\n-\tstring_list_insert(&added_submodule_odb_paths, xstrdup(path));\n+\tstring_list_insert(&added_submodule_odb_paths, path);\n }\n \n int register_all_submodule_odb_as_alternates(void)\n-- \n2.46.2.852.g229c0bf0e5.dirty\n\n"},{"id":"503530","messageId":"3d1cece660a622a2b4f7a068cdb4eeb5ac6f8a61.1727351062.git.ps@pks.im","threadId":"62120","inReplyTo":"cover.1727351062.git.ps@pks.im","subject":"[PATCH v2 18/23] grep: fix leaking grep pattern","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-26T11:46:54Z","receivedAt":"2024-09-26T11:46:59Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When creating a pattern via `create_grep_pat()` we allocate the pattern\nmember of the structure regardless of the token type. But later, when we\ntry to free the structure, we free the pattern member conditionally on\nthe token type and thus leak memory.\n\nPlug this leak. The leak is exposed by t7814, but plugging it alone does\nnot make the whole test suite pass.\n\nSigned-off-by: Patrick Steinhardt \n---\n grep.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/grep.c b/grep.c\nindex e5761426e4..701e58de04 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -843,11 +843,11 @@ static void free_grep_pat(struct grep_pat *pattern)\n \t\t\t\tfree_pcre2_pattern(p);\n \t\t\telse\n \t\t\t\tregfree(&p->regexp);\n-\t\t\tfree(p->pattern);\n \t\t\tbreak;\n \t\tdefault:\n \t\t\tbreak;\n \t\t}\n+\t\tfree(p->pattern);\n \t\tfree(p);\n \t}\n }\n-- \n2.46.2.852.g229c0bf0e5.dirty\n\n"},{"id":"503531","messageId":"da7768bad58ea6cd600b8517f943ad973d3d34cc.1727351062.git.ps@pks.im","threadId":"62120","inReplyTo":"cover.1727351062.git.ps@pks.im","subject":"[PATCH v2 19/23] promisor-remote: fix leaking partial clone filter","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-26T11:46:57Z","receivedAt":"2024-09-26T11:47:01Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The partial clone filter of a promisor remote is never free'd, causing\nmemory leaks. Furthermore, in case multiple partial clone filters are\ndefined for the same remote, we'd overwrite previous values without\nfreeing them.\n\nFix these leaks.\n\nSigned-off-by: Patrick Steinhardt \n---\n promisor-remote.c | 2 ++\n t/t7814-grep-recurse-submodules.sh | 1 +\n 2 files changed, 3 insertions(+)\n\ndiff --git a/promisor-remote.c b/promisor-remote.c\nindex 317e1b127f..9345ae3db2 100644\n--- a/promisor-remote.c\n+++ b/promisor-remote.c\n@@ -154,6 +154,7 @@ static int promisor_remote_config(const char *var, const char *value,\n \t\tif (!r)\n \t\t\treturn 0;\n \n+\t\tFREE_AND_NULL(r->partial_clone_filter);\n \t\treturn git_config_string(&r->partial_clone_filter, var, value);\n \t}\n \n@@ -189,6 +190,7 @@ void promisor_remote_clear(struct promisor_remote_config *config)\n {\n \twhile (config->promisors) {\n \t\tstruct promisor_remote *r = config->promisors;\n+\t\tfree(r->partial_clone_filter);\n \t\tconfig->promisors = config->promisors->next;\n \t\tfree(r);\n \t}\ndiff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh\nindex 167fe66150..55ed630e77 100755\n--- a/t/t7814-grep-recurse-submodules.sh\n+++ b/t/t7814-grep-recurse-submodules.sh\n@@ -7,6 +7,7 @@ submodules.\n '\n \n TEST_CREATE_REPO_NO_TEMPLATE=1\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB=1\n-- \n2.46.2.852.g229c0bf0e5.dirty\n\n"},{"id":"503532","messageId":"0a72fc83f797d0f0f753e54b8473740a041702ae.1727351062.git.ps@pks.im","threadId":"62120","inReplyTo":"cover.1727351062.git.ps@pks.im","subject":"[PATCH v2 20/23] builtin/maintenance: fix leaking config string","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-26T11:46:59Z","receivedAt":"2024-09-26T11:47:04Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When parsing the maintenance strategy from config we allocate a config\nstring, but do not free it after parsing it. Plug this leak by instead\nusing `git_config_get_string_tmp()`, which does not allocate any memory.\n\nThis leak is exposed by t7900, but plugging it alone does not make the\ntest suite pass.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/gc.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 7dac971405..3acfa367ad 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -1476,9 +1476,9 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts,\n \n static void initialize_maintenance_strategy(void)\n {\n-\tchar *config_str;\n+\tconst char *config_str;\n \n-\tif (git_config_get_string(\"maintenance.strategy\", &config_str))\n+\tif (git_config_get_string_tmp(\"maintenance.strategy\", &config_str))\n \t\treturn;\n \n \tif (!strcasecmp(config_str, \"incremental\")) {\n-- \n2.46.2.852.g229c0bf0e5.dirty\n\n"},{"id":"503533","messageId":"95200b8a766e2ba7a971ab79b6d13f03bfa3b8ac.1727351062.git.ps@pks.im","threadId":"62120","inReplyTo":"cover.1727351062.git.ps@pks.im","subject":"[PATCH v2 21/23] builtin/maintenance: fix leak in `get_schedule_cmd()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-26T11:47:02Z","receivedAt":"2024-09-26T11:47:08Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `get_schedule_cmd()` function allows us to override the schedule\ncommand with a specific test command such that we can verify the\nunderlying logic in a platform-independent way. Its memory management is\nsomewhat wild though, because it basically gives up and assigns an\nallocated string to the string constant output pointer. While this part\nis marked with `UNLEAK()` to mask this, we also leak the local string\nlists.\n\nRework the function such that it has a separate out parameter. If set,\nwe will assign it the final allocated command. Plug the other memory\nleaks and create a common exit path.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/gc.c | 127 ++++++++++++++++++++++++++---------------\n t/t7900-maintenance.sh | 1 +\n 2 files changed, 81 insertions(+), 47 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 3acfa367ad..b68a0be62c 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -1780,32 +1780,33 @@ static const char *get_frequency(enum schedule_priority schedule)\n * * If $GIT_TEST_MAINT_SCHEDULER is set, return true.\n * In this case, the *cmd value is read as input.\n *\n- * * if the input value *cmd is the key of one of the comma-separated list\n- * item, then *is_available is set to true and *cmd is modified and becomes\n+ * * if the input value cmd is the key of one of the comma-separated list\n+ * item, then *is_available is set to true and *out is set to\n * the mock command.\n *\n * * if the input value *cmd isn’t the key of any of the comma-separated list\n- * item, then *is_available is set to false.\n+ * item, then *is_available is set to false and *out is set to the original\n+ * command.\n *\n * Ex.:\n * GIT_TEST_MAINT_SCHEDULER not set\n * +-------+-------------------------------------------------+\n * | Input | Output |\n- * | *cmd | return code | *cmd | *is_available |\n+ * | *cmd | return code | *out | *is_available |\n * +-------+-------------+-------------------+---------------+\n- * | \"foo\" | false | \"foo\" (unchanged) | (unchanged) |\n+ * | \"foo\" | false | NULL | (unchanged) |\n * +-------+-------------+-------------------+---------------+\n *\n * GIT_TEST_MAINT_SCHEDULER set to “foo:./mock_foo.sh,bar:./mock_bar.sh”\n * +-------+-------------------------------------------------+\n * | Input | Output |\n- * | *cmd | return code | *cmd | *is_available |\n+ * | *cmd | return code | *out | *is_available |\n * +-------+-------------+-------------------+---------------+\n * | \"foo\" | true | \"./mock.foo.sh\" | true |\n- * | \"qux\" | true | \"qux\" (unchanged) | false |\n+ * | \"qux\" | true | \"qux\" (allocated) | false |\n * +-------+-------------+-------------------+---------------+\n */\n-static int get_schedule_cmd(const char **cmd, int *is_available)\n+static int get_schedule_cmd(const char *cmd, int *is_available, char **out)\n {\n \tchar *testing = xstrdup_or_null(getenv(\"GIT_TEST_MAINT_SCHEDULER\"));\n \tstruct string_list_item *item;\n@@ -1824,16 +1825,22 @@ static int get_schedule_cmd(const char **cmd, int *is_available)\n \t\tif (string_list_split_in_place(&pair, item->string, \":\", 2) != 2)\n \t\t\tcontinue;\n \n-\t\tif (!strcmp(*cmd, pair.items[0].string)) {\n-\t\t\t*cmd = pair.items[1].string;\n+\t\tif (!strcmp(cmd, pair.items[0].string)) {\n+\t\t\tif (out)\n+\t\t\t\t*out = xstrdup(pair.items[1].string);\n \t\t\tif (is_available)\n \t\t\t\t*is_available = 1;\n-\t\t\tstring_list_clear(&list, 0);\n-\t\t\tUNLEAK(testing);\n-\t\t\treturn 1;\n+\t\t\tstring_list_clear(&pair, 0);\n+\t\t\tgoto out;\n \t\t}\n+\n+\t\tstring_list_clear(&pair, 0);\n \t}\n \n+\tif (out)\n+\t\t*out = xstrdup(cmd);\n+\n+out:\n \tstring_list_clear(&list, 0);\n \tfree(testing);\n \treturn 1;\n@@ -1850,9 +1857,8 @@ static int get_random_minute(void)\n \n static int is_launchctl_available(void)\n {\n-\tconst char *cmd = \"launchctl\";\n \tint is_available;\n-\tif (get_schedule_cmd(&cmd, &is_available))\n+\tif (get_schedule_cmd(\"launchctl\", &is_available, NULL))\n \t\treturn is_available;\n \n #ifdef __APPLE__\n@@ -1890,12 +1896,12 @@ static char *launchctl_get_uid(void)\n \n static int launchctl_boot_plist(int enable, const char *filename)\n {\n-\tconst char *cmd = \"launchctl\";\n+\tchar *cmd;\n \tint result;\n \tstruct child_process child = CHILD_PROCESS_INIT;\n \tchar *uid = launchctl_get_uid();\n \n-\tget_schedule_cmd(&cmd, NULL);\n+\tget_schedule_cmd(\"launchctl\", NULL, &cmd);\n \tstrvec_split(&child.args, cmd);\n \tstrvec_pushl(&child.args, enable ? \"bootstrap\" : \"bootout\", uid,\n \t\t filename, NULL);\n@@ -1908,6 +1914,7 @@ static int launchctl_boot_plist(int enable, const char *filename)\n \n \tresult = finish_command(&child);\n \n+\tfree(cmd);\n \tfree(uid);\n \treturn result;\n }\n@@ -1959,10 +1966,10 @@ static int launchctl_schedule_plist(const char *exec_path, enum schedule_priorit\n \tstatic unsigned long lock_file_timeout_ms = ULONG_MAX;\n \tstruct strbuf plist = STRBUF_INIT, plist2 = STRBUF_INIT;\n \tstruct stat st;\n-\tconst char *cmd = \"launchctl\";\n+\tchar *cmd;\n \tint minute = get_random_minute();\n \n-\tget_schedule_cmd(&cmd, NULL);\n+\tget_schedule_cmd(\"launchctl\", NULL, &cmd);\n \tpreamble = \"\\n\"\n \t\t \"\\n\"\n \t\t \"\"\n@@ -2052,6 +2059,7 @@ static int launchctl_schedule_plist(const char *exec_path, enum schedule_priorit\n \n \tfree(filename);\n \tfree(name);\n+\tfree(cmd);\n \tstrbuf_release(&plist);\n \tstrbuf_release(&plist2);\n \treturn 0;\n@@ -2076,9 +2084,8 @@ static int launchctl_update_schedule(int run_maintenance, int fd UNUSED)\n \n static int is_schtasks_available(void)\n {\n-\tconst char *cmd = \"schtasks\";\n \tint is_available;\n-\tif (get_schedule_cmd(&cmd, &is_available))\n+\tif (get_schedule_cmd(\"schtasks\", &is_available, NULL))\n \t\treturn is_available;\n \n #ifdef GIT_WINDOWS_NATIVE\n@@ -2097,15 +2104,16 @@ static char *schtasks_task_name(const char *frequency)\n \n static int schtasks_remove_task(enum schedule_priority schedule)\n {\n-\tconst char *cmd = \"schtasks\";\n+\tchar *cmd;\n \tstruct child_process child = CHILD_PROCESS_INIT;\n \tconst char *frequency = get_frequency(schedule);\n \tchar *name = schtasks_task_name(frequency);\n \n-\tget_schedule_cmd(&cmd, NULL);\n+\tget_schedule_cmd(\"schtasks\", NULL, &cmd);\n \tstrvec_split(&child.args, cmd);\n \tstrvec_pushl(&child.args, \"/delete\", \"/tn\", name, \"/f\", NULL);\n \tfree(name);\n+\tfree(cmd);\n \n \treturn run_command(&child);\n }\n@@ -2119,7 +2127,7 @@ static int schtasks_remove_tasks(void)\n \n static int schtasks_schedule_task(const char *exec_path, enum schedule_priority schedule)\n {\n-\tconst char *cmd = \"schtasks\";\n+\tchar *cmd;\n \tint result;\n \tstruct child_process child = CHILD_PROCESS_INIT;\n \tconst char *xml;\n@@ -2129,7 +2137,7 @@ static int schtasks_schedule_task(const char *exec_path, enum schedule_priority\n \tstruct strbuf tfilename = STRBUF_INIT;\n \tint minute = get_random_minute();\n \n-\tget_schedule_cmd(&cmd, NULL);\n+\tget_schedule_cmd(\"schtasks\", NULL, &cmd);\n \n \tstrbuf_addf(&tfilename, \"%s/schedule_%s_XXXXXX\",\n \t\t get_git_common_dir(), frequency);\n@@ -2235,6 +2243,7 @@ static int schtasks_schedule_task(const char *exec_path, enum schedule_priority\n \n \tdelete_tempfile(&tfile);\n \tfree(name);\n+\tfree(cmd);\n \treturn result;\n }\n \n@@ -2276,21 +2285,28 @@ static int check_crontab_process(const char *cmd)\n \n static int is_crontab_available(void)\n {\n-\tconst char *cmd = \"crontab\";\n+\tchar *cmd;\n \tint is_available;\n+\tint ret;\n \n-\tif (get_schedule_cmd(&cmd, &is_available))\n-\t\treturn is_available;\n+\tif (get_schedule_cmd(\"crontab\", &is_available, &cmd)) {\n+\t\tret = is_available;\n+\t\tgoto out;\n+\t}\n \n #ifdef __APPLE__\n \t/*\n \t * macOS has cron, but it requires special permissions and will\n \t * create a UI alert when attempting to run this command.\n \t */\n-\treturn 0;\n+\tret = 0;\n #else\n-\treturn check_crontab_process(cmd);\n+\tret = check_crontab_process(cmd);\n #endif\n+\n+out:\n+\tfree(cmd);\n+\treturn ret;\n }\n \n #define BEGIN_LINE \"# BEGIN GIT MAINTENANCE SCHEDULE\"\n@@ -2298,7 +2314,7 @@ static int is_crontab_available(void)\n \n static int crontab_update_schedule(int run_maintenance, int fd)\n {\n-\tconst char *cmd = \"crontab\";\n+\tchar *cmd;\n \tint result = 0;\n \tint in_old_region = 0;\n \tstruct child_process crontab_list = CHILD_PROCESS_INIT;\n@@ -2308,15 +2324,17 @@ static int crontab_update_schedule(int run_maintenance, int fd)\n \tstruct tempfile *tmpedit = NULL;\n \tint minute = get_random_minute();\n \n-\tget_schedule_cmd(&cmd, NULL);\n+\tget_schedule_cmd(\"crontab\", NULL, &cmd);\n \tstrvec_split(&crontab_list.args, cmd);\n \tstrvec_push(&crontab_list.args, \"-l\");\n \tcrontab_list.in = -1;\n \tcrontab_list.out = dup(fd);\n \tcrontab_list.git_cmd = 0;\n \n-\tif (start_command(&crontab_list))\n-\t\treturn error(_(\"failed to run 'crontab -l'; your system might not support 'cron'\"));\n+\tif (start_command(&crontab_list)) {\n+\t\tresult = error(_(\"failed to run 'crontab -l'; your system might not support 'cron'\"));\n+\t\tgoto out;\n+\t}\n \n \t/* Ignore exit code, as an empty crontab will return error. */\n \tfinish_command(&crontab_list);\n@@ -2386,8 +2404,10 @@ static int crontab_update_schedule(int run_maintenance, int fd)\n \t\tresult = error(_(\"'crontab' died\"));\n \telse\n \t\tfclose(cron_list);\n+\n out:\n \tdelete_tempfile(&tmpedit);\n+\tfree(cmd);\n \treturn result;\n }\n \n@@ -2410,10 +2430,9 @@ static int real_is_systemd_timer_available(void)\n \n static int is_systemd_timer_available(void)\n {\n-\tconst char *cmd = \"systemctl\";\n \tint is_available;\n \n-\tif (get_schedule_cmd(&cmd, &is_available))\n+\tif (get_schedule_cmd(\"systemctl\", &is_available, NULL))\n \t\treturn is_available;\n \n \treturn real_is_systemd_timer_available();\n@@ -2594,9 +2613,10 @@ static int systemd_timer_enable_unit(int enable,\n \t\t\t\t enum schedule_priority schedule,\n \t\t\t\t int minute)\n {\n-\tconst char *cmd = \"systemctl\";\n+\tchar *cmd = NULL;\n \tstruct child_process child = CHILD_PROCESS_INIT;\n \tconst char *frequency = get_frequency(schedule);\n+\tint ret;\n \n \t/*\n \t * Disabling the systemd unit while it is already disabled makes\n@@ -2607,20 +2627,25 @@ static int systemd_timer_enable_unit(int enable,\n \t * On the other hand, enabling a systemd unit which is already enabled\n \t * produces no error.\n \t */\n-\tif (!enable)\n+\tif (!enable) {\n \t\tchild.no_stderr = 1;\n-\telse if (systemd_timer_write_timer_file(schedule, minute))\n-\t\treturn -1;\n+\t} else if (systemd_timer_write_timer_file(schedule, minute)) {\n+\t\tret = -1;\n+\t\tgoto out;\n+\t}\n \n-\tget_schedule_cmd(&cmd, NULL);\n+\tget_schedule_cmd(\"systemctl\", NULL, &cmd);\n \tstrvec_split(&child.args, cmd);\n \tstrvec_pushl(&child.args, \"--user\", enable ? \"enable\" : \"disable\",\n \t\t \"--now\", NULL);\n \tstrvec_pushf(&child.args, SYSTEMD_UNIT_FORMAT, frequency, \"timer\");\n \n-\tif (start_command(&child))\n-\t\treturn error(_(\"failed to start systemctl\"));\n-\tif (finish_command(&child))\n+\tif (start_command(&child)) {\n+\t\tret = error(_(\"failed to start systemctl\"));\n+\t\tgoto out;\n+\t}\n+\n+\tif (finish_command(&child)) {\n \t\t/*\n \t\t * Disabling an already disabled systemd unit makes\n \t\t * systemctl fail.\n@@ -2628,9 +2653,17 @@ static int systemd_timer_enable_unit(int enable,\n \t\t *\n \t\t * Enabling an enabled systemd unit doesn't fail.\n \t\t */\n-\t\tif (enable)\n-\t\t\treturn error(_(\"failed to run systemctl\"));\n-\treturn 0;\n+\t\tif (enable) {\n+\t\t\tret = error(_(\"failed to run systemctl\"));\n+\t\t\tgoto out;\n+\t\t}\n+\t}\n+\n+\tret = 0;\n+\n+out:\n+\tfree(cmd);\n+\treturn ret;\n }\n \n /*\ndiff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\nindex abae7a9754..c62c42848a 100755\n--- a/t/t7900-maintenance.sh\n+++ b/t/t7900-maintenance.sh\n@@ -2,6 +2,7 @@\n \n test_description='git maintenance builtin'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n GIT_TEST_COMMIT_GRAPH=0\n-- \n2.46.2.852.g229c0bf0e5.dirty\n\n"},{"id":"503534","messageId":"8e7ea54863f9fde80bc99d654140146cbb1247f7.1727351062.git.ps@pks.im","threadId":"62120","inReplyTo":"cover.1727351062.git.ps@pks.im","subject":"[PATCH v2 22/23] revision: fix leaking parents when simplifying commits","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-26T11:47:05Z","receivedAt":"2024-09-26T11:47:10Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When simplifying commits, e.g. because they are treesame with their\nparents, we unset the commit's parent pointers but never free them. Plug\nthe resulting memory leaks.\n\nSigned-off-by: Patrick Steinhardt \n---\n revision.c | 5 +++++\n t/t1414-reflog-walk.sh | 1 +\n t/t5310-pack-bitmaps.sh | 1 +\n t/t5326-multi-pack-bitmaps.sh | 2 ++\n t/t6004-rev-list-path-optim.sh | 1 +\n t/t6019-rev-list-ancestry-path.sh | 1 +\n t/t6111-rev-list-treesame.sh | 1 +\n 7 files changed, 12 insertions(+)\n\ndiff --git a/revision.c b/revision.c\nindex 2d7ad2bddf..e79f39e555 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1071,7 +1071,11 @@ static void try_to_simplify_commit(struct rev_info *revs, struct commit *commit)\n \t\t\t\t\tts->treesame[nth_parent] = 1;\n \t\t\t\tcontinue;\n \t\t\t}\n+\n+\t\t\tfree_commit_list(parent->next);\n \t\t\tparent->next = NULL;\n+\t\t\twhile (commit->parents != parent)\n+\t\t\t\tpop_commit(&commit->parents);\n \t\t\tcommit->parents = parent;\n \n \t\t\t/*\n@@ -1103,6 +1107,7 @@ static void try_to_simplify_commit(struct rev_info *revs, struct commit *commit)\n \t\t\t\t\tdie(\"cannot simplify commit %s (invalid %s)\",\n \t\t\t\t\t oid_to_hex(&commit->object.oid),\n \t\t\t\t\t oid_to_hex(&p->object.oid));\n+\t\t\t\tfree_commit_list(p->parents);\n \t\t\t\tp->parents = NULL;\n \t\t\t}\n \t\t/* fallthrough */\ndiff --git a/t/t1414-reflog-walk.sh b/t/t1414-reflog-walk.sh\nindex be6c3f472c..49d28166da 100755\n--- a/t/t1414-reflog-walk.sh\n+++ b/t/t1414-reflog-walk.sh\n@@ -4,6 +4,7 @@ test_description='various tests of reflog walk (log -g) behavior'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'set up some reflog entries' '\ndiff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh\nindex a6de7c5764..7044c7d7c6 100755\n--- a/t/t5310-pack-bitmaps.sh\n+++ b/t/t5310-pack-bitmaps.sh\n@@ -2,6 +2,7 @@\n \n test_description='exercise basic bitmap functionality'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-bitmap.sh\n \ndiff --git a/t/t5326-multi-pack-bitmaps.sh b/t/t5326-multi-pack-bitmaps.sh\nindex 832b92619c..6eaa692f33 100755\n--- a/t/t5326-multi-pack-bitmaps.sh\n+++ b/t/t5326-multi-pack-bitmaps.sh\n@@ -1,6 +1,8 @@\n #!/bin/sh\n \n test_description='exercise basic multi-pack bitmap functionality'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n . \"${TEST_DIRECTORY}/lib-bitmap.sh\"\n \ndiff --git a/t/t6004-rev-list-path-optim.sh b/t/t6004-rev-list-path-optim.sh\nindex cd4f420e2a..5416241ede 100755\n--- a/t/t6004-rev-list-path-optim.sh\n+++ b/t/t6004-rev-list-path-optim.sh\n@@ -16,6 +16,7 @@ test_description='git rev-list trivial path optimization test\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success setup '\ndiff --git a/t/t6019-rev-list-ancestry-path.sh b/t/t6019-rev-list-ancestry-path.sh\nindex 738da23628..1aabab6956 100755\n--- a/t/t6019-rev-list-ancestry-path.sh\n+++ b/t/t6019-rev-list-ancestry-path.sh\n@@ -29,6 +29,7 @@ test_description='--ancestry-path'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_merge () {\ndiff --git a/t/t6111-rev-list-treesame.sh b/t/t6111-rev-list-treesame.sh\nindex 90ff141640..f63bc8d3da 100755\n--- a/t/t6111-rev-list-treesame.sh\n+++ b/t/t6111-rev-list-treesame.sh\n@@ -16,6 +16,7 @@ test_description='TREESAME and limiting'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n note () {\n-- \n2.46.2.852.g229c0bf0e5.dirty\n\n"},{"id":"503535","messageId":"8cbc41425f1100928c64c02602e62e1070998ec6.1727351062.git.ps@pks.im","threadId":"62120","inReplyTo":"cover.1727351062.git.ps@pks.im","subject":"[PATCH v2 23/23] diffcore-break: fix leaking filespecs when merging broken pairs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-26T11:47:08Z","receivedAt":"2024-09-26T11:47:14Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When merging file pairs after they have been broken up we queue a new\nfile pair and discard the broken-up ones. The newly-queued file pair\nreuses one filespec of the broken up pairs each, where the respective\nother filespec gets discarded. But we only end up freeing the filespec's\ndata, not the filespec itself, and thus leak memory.\n\nFix these leaks by using `free_filespec()` instead.\n\nSigned-off-by: Patrick Steinhardt \n---\n diffcore-break.c | 4 ++--\n t/t4008-diff-break-rewrite.sh | 2 ++\n t/t4022-diff-rewrite.sh | 1 +\n t/t4023-diff-rename-typechange.sh | 1 +\n t/t4031-diff-rewrite-binary.sh | 1 +\n t/t7524-commit-summary.sh | 2 ++\n 6 files changed, 9 insertions(+), 2 deletions(-)\n\ndiff --git a/diffcore-break.c b/diffcore-break.c\nindex 831b66b5c3..02735f80c6 100644\n--- a/diffcore-break.c\n+++ b/diffcore-break.c\n@@ -266,8 +266,8 @@ static void merge_broken(struct diff_filepair *p,\n \t * in the resulting tree.\n \t */\n \td->one->rename_used++;\n-\tdiff_free_filespec_data(d->two);\n-\tdiff_free_filespec_data(c->one);\n+\tfree_filespec(d->two);\n+\tfree_filespec(c->one);\n \tfree(d);\n \tfree(c);\n }\ndiff --git a/t/t4008-diff-break-rewrite.sh b/t/t4008-diff-break-rewrite.sh\nindex 562aaf3a2a..b0ef0026e0 100755\n--- a/t/t4008-diff-break-rewrite.sh\n+++ b/t/t4008-diff-break-rewrite.sh\n@@ -21,6 +21,8 @@ With -B, this should be detected as two complete rewrites.\n \n Further, with -B and -M together, these should turn into two renames.\n '\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-diff.sh ;# test-lib chdir's into trash\n \ndiff --git a/t/t4022-diff-rewrite.sh b/t/t4022-diff-rewrite.sh\nindex 6fed993ea0..77bc36d5d8 100755\n--- a/t/t4022-diff-rewrite.sh\n+++ b/t/t4022-diff-rewrite.sh\n@@ -2,6 +2,7 @@\n \n test_description='rewrite diff'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-diff-data.sh\n \ndiff --git a/t/t4023-diff-rename-typechange.sh b/t/t4023-diff-rename-typechange.sh\nindex 787605ce3f..e6f4fe441e 100755\n--- a/t/t4023-diff-rename-typechange.sh\n+++ b/t/t4023-diff-rename-typechange.sh\n@@ -2,6 +2,7 @@\n \n test_description='typechange rename detection'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-diff.sh\n \ndiff --git a/t/t4031-diff-rewrite-binary.sh b/t/t4031-diff-rewrite-binary.sh\nindex c4394a27b5..1b8cd3e4c9 100755\n--- a/t/t4031-diff-rewrite-binary.sh\n+++ b/t/t4031-diff-rewrite-binary.sh\n@@ -2,6 +2,7 @@\n \n test_description='rewrite diff on binary file'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n # We must be large enough to meet the MINIMUM_BREAK_SIZE\ndiff --git a/t/t7524-commit-summary.sh b/t/t7524-commit-summary.sh\nindex 47b2f1dc22..a8fceb6a47 100755\n--- a/t/t7524-commit-summary.sh\n+++ b/t/t7524-commit-summary.sh\n@@ -1,6 +1,8 @@\n #!/bin/sh\n \n test_description='git commit summary'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'setup' '\n-- \n2.46.2.852.g229c0bf0e5.dirty\n\n"},{"id":"503539","messageId":"ZvVMg2kJR97JuVyG@pks.im","threadId":"62120","inReplyTo":"xmqqbk0bo4ye.fsf@gitster.g","subject":"Re: [PATCH 08/23] builtin/submodule--helper: fix leaking remote ref on errors","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-26T11:58:59Z","receivedAt":"2024-09-26T11:59:06Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Sep 25, 2024 at 01:26:17PM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> \n> > On Mon, Sep 16, 2024 at 01:51:21PM -0500, Justin Tobler wrote:\n> >> On 24/09/16 01:45PM, Patrick Steinhardt wrote:\n> >> > When `update_submodule()` fails we return with `die_message()`.\n> >> > Curiously enough, this causes a memory leak because we use the\n> >> > `run_process_parallel()` interfaces here, which swap out the die\n> >> > routine.\n> >> \n> >> Naive question, is `update_submodule()` itself being run in parallel\n> >> here? Is that why the die routine gets swapped out so a child process\n> >> dying is handled differently? Also is it correct to say leaks are not\n> >> considered when we \"die\" normally? \n> >\n> > Hm. Revisiting this patch: my analysis was wrong. It's not the parallel\n> > subsystem that swaps out `die()`, but it's the fact that we call\n> > `die_message()`, which actually doesn't die. It really only prints the\n> > message you would see when we call `die()`, nothing more.\n> >\n> > I'll amend the commit message and send out the amended version once\n> > there is more feedback to address.\n> \n> So it has been a week and half since the series was posted and it\n> seems that this is the only thing you might want to touch up.\n> \n> What's next? Just have an updated patch [08/23] and nothing else\n> and be done with it? A v2 round of 23-patch series hopefully will\n> see somebody other than Justin and I lend an extra set of eyes to\n> double check before we merge it to 'next'?\n\nMakes sense, let's do it this way! I've sent a v2 a couple minutes ago.\n\nPatrick\n"}]}