This series fixes all of the memory leaks in builtin/submodule--helper.c that our test suite spots, except for those where the leak is downstream of cmd_submodule__helper() and caused by code that's not in builtin/submodule--helper.c (e.g. leaks in the object or config APIs, and in submodule.c). This re-roll is on top of the just-now re-rolled v2 of the base topic[1]. The only changes are to rebase the changes here on top of those changes. Changes: * The base topic renamed another "res" variable (one was left behind before). * The "goto cleanup" pattern in update_submodule() is gone now, which makes 16/17 much easier. For the CI run & pushed branch see [2]. 1. https://lore.kernel.org/git/cover-v2-00.28-00000000000-20220802T154036Z-avarab@xxxxxxxxx/ 2. https://github.com/avar/git/tree/avar/submodule--helper-memory-leaks-5 Ævar Arnfjörð Bjarmason (17): submodule--helper: fix a leak in "clone_submodule" submodule--helper: fix trivial get_default_remote_submodule() leak submodule--helper: fix most "struct pathspec" memory leaks submodule--helper: "struct pathspec" memory leak in module_update() submodule--helper: don't leak {run,capture}_command() cp.dir argument submodule--helper: add and use *_release() functions submodule--helper: fix "errmsg_str" memory leak submodule--helper: fix "sm_path" and other "module_cb_list" leaks submodule--helper: fix a leak with repo_clear() submodule--helper: fix a memory leak in get_default_remote_submodule() submodule--helper: fix "reference" leak submodule--helper: fix obscure leak in module_add() submodule--helper: fix a leak in module_add() submodule--helper: fix a memory leak in print_status() submodule--helper: free some "displaypath" in "struct update_data" submodule--helper: free rest of "displaypath" in "struct update_data" submodule--helper: fix a configure_added_submodule() leak builtin/submodule--helper.c | 239 +++++++++++++++++++++-------- t/t1500-rev-parse.sh | 1 + t/t2403-worktree-move.sh | 1 + t/t6008-rev-list-submodule.sh | 1 + t/t6134-pathspec-in-submodule.sh | 1 + t/t7412-submodule-absorbgitdirs.sh | 1 + t/t7413-submodule-is-active.sh | 1 + t/t7414-submodule-mistakes.sh | 2 + t/t7506-status-submodule.sh | 1 + t/t7507-commit-verbose.sh | 2 + 10 files changed, 185 insertions(+), 65 deletions(-) Range-diff against v4: 1: aac987a414a = 1: 118e965d401 submodule--helper: fix a leak in "clone_submodule" 2: 390c5174e17 ! 2: d885e1dd59a submodule--helper: fix trivial get_default_remote_submodule() leak @@ Commit message Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> ## builtin/submodule--helper.c ## -@@ builtin/submodule--helper.c: static int update_submodule(struct update_data *update_data, +@@ builtin/submodule--helper.c: static int update_submodule(struct update_data *update_data) const char *branch = remote_submodule_branch(update_data->sm_path); char *remote_ref = xstrfmt("refs/remotes/%s/%s", remote_name, branch); 3: 529e10233d5 = 3: 1f90348d61f submodule--helper: fix most "struct pathspec" memory leaks 4: 683d327752f = 4: f768ff2e34d submodule--helper: "struct pathspec" memory leak in module_update() 5: 4e8e9197539 ! 5: 509133c37fa submodule--helper: don't leak {run,capture}_command() cp.dir argument @@ builtin/submodule--helper.c: static int fetch_in_submodule(const char *module_pa strvec_push(&cp.args, "fetch"); if (quiet) -@@ builtin/submodule--helper.c: static int run_update_command(struct update_data *ud, int subforce, +@@ builtin/submodule--helper.c: static int run_update_command(struct update_data *ud, int subforce) } strvec_push(&cp.args, oid); @@ builtin/submodule--helper.c: static int run_update_command(struct update_data *u + cp.dir = ud->sm_path; prepare_submodule_repo_env(&cp.env); if (run_command(&cp)) { - switch (ud->update_strategy.type) { + int ret; 6: 575d3e8d2e2 ! 6: 25377f1d06c submodule--helper: add and use *_release() functions @@ builtin/submodule--helper.c: static int update_submodules(struct update_data *up cleanup: + submodule_update_clone_release(&suc); string_list_clear(&update_data->references, 0); - return res; + return ret; } @@ builtin/submodule--helper.c: static int module_update(int argc, const char **argv, const char *prefix) 7: d4c81e04254 = 7: 1a85057cd0c submodule--helper: fix "errmsg_str" memory leak 8: ef9e29d5bfe = 8: 3c4f734e958 submodule--helper: fix "sm_path" and other "module_cb_list" leaks 9: 0798a00c9ef ! 9: 3aebff9f8e3 submodule--helper: fix a leak with repo_clear() @@ Commit message Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> ## builtin/submodule--helper.c ## -@@ builtin/submodule--helper.c: static void ensure_core_worktree(const char *path) - free(abs_path); +@@ builtin/submodule--helper.c: static int ensure_core_worktree(const char *path) strbuf_release(&sb); } + + repo_clear(&subrepo); + return 0; } - static const char *submodule_update_type_to_label(enum submodule_update_type type) 10: dae2a6f8e07 = 10: 38345ec76bc submodule--helper: fix a memory leak in get_default_remote_submodule() 11: e7352bb8cfa = 11: 4b1e5ced969 submodule--helper: fix "reference" leak 12: 1adb7b66656 = 12: e74c396073b submodule--helper: fix obscure leak in module_add() 13: b27b665d287 = 13: 71a56c59864 submodule--helper: fix a leak in module_add() 14: 53ba1705eb6 = 14: 2429db2f1c7 submodule--helper: fix a memory leak in print_status() 15: 230e5f8ad14 = 15: 613d077c4ec submodule--helper: free some "displaypath" in "struct update_data" 16: c0fba2f1c56 ! 16: 8f150a81507 submodule--helper: free rest of "displaypath" in "struct update_data" @@ Commit message update_submodule() function itself can use it, and for the run_update_procedure() called within this function. + To make managing that clobbering easier let's wrap the + update_submodule() in a new update_submodule_outer() function, which + will do the clobbering and free(to_free) dance for us. + Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> ## builtin/submodule--helper.c ## -@@ builtin/submodule--helper.c: static int update_submodule(struct update_data *update_data, - int *must_die_on_failure) +@@ builtin/submodule--helper.c: static int update_submodule(struct update_data *update_data) { - int ret = 1; -+ char *to_free, *restore = update_data->displaypath; - - ensure_core_worktree(update_data->sm_path); + int ret; +- ret = ensure_core_worktree(update_data->sm_path); +- if (ret) +- return ret; +- - update_data->displaypath = get_submodule_displaypath( -+ update_data->displaypath = to_free = get_submodule_displaypath( - update_data->sm_path, update_data->prefix); - - determine_submodule_update_strategy(the_repository, update_data->just_cloned, -@@ builtin/submodule--helper.c: static int update_submodule(struct update_data *update_data, +- update_data->sm_path, update_data->prefix); +- + ret = determine_submodule_update_strategy(the_repository, + update_data->just_cloned, + update_data->sm_path, +@@ builtin/submodule--helper.c: static int update_submodule(struct update_data *update_data) + return 0; + } - ret = 0; - cleanup: ++static int update_submodule_outer(struct update_data *update_data) ++{ ++ char *to_free, *restore = update_data->displaypath; ++ int ret; ++ ++ ensure_core_worktree(update_data->sm_path); ++ ++ update_data->displaypath = to_free = get_submodule_displaypath( ++ update_data->sm_path, update_data->prefix); ++ ++ ret = update_submodule(update_data); ++ + free(to_free); + update_data->displaypath = restore; + - return ret; - } ++ return ret; ++} ++ + static int update_submodules(struct update_data *update_data) + { + int i, ret = 0; +@@ builtin/submodule--helper.c: static int update_submodules(struct update_data *update_data) + update_data->just_cloned = ucd.just_cloned; + update_data->sm_path = ucd.sub->path; +- code = update_submodule(update_data); ++ code = update_submodule_outer(update_data); + if (code) + ret = code; + if (code == 128) 17: 95f8b68bd41 ! 17: 17c77ceba01 submodule--helper: fix a configure_added_submodule() leak @@ builtin/submodule--helper.c: static void configure_added_submodule(struct add_da * current configured pathspec, set the submodule's active flag ## t/t7413-submodule-is-active.sh ## -@@ t/t7413-submodule-is-active.sh: This test verifies that `test-tool submodule is-active` correctly identifies - submodules which are "active" and interesting to the user. +@@ t/t7413-submodule-is-active.sh: This is a unit test of the submodule.c is_submodule_active() function, + which is also indirectly tested elsewhere. ' +TEST_PASSES_SANITIZE_LEAK=true -- 2.37.1.1233.ge8b09efaedc
