| Message ID | patch-v2-08.24-b7582391c91-20220719T204458Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | submodule--helper: fix memory leaks | expand |
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > Add release functions for "struct module_list", "struct > submodule_update_clone" and "struct update_data". OK, this does a lot of things, but in short, nobody bothered to free module_list (i.e. list of cache entries borrowed from the index to represent the paths we are interested in that are gitlinks), update_data (which has module_list among other things that do not need to be freed), and submodule_update_clone (which has update_data, update_clone_data and failed_clones list) in the original (in other words, these release helpers had to be invented by this patch, not factored out from some codepaths that free them). I think the earlier part of the patch that deals with module_list is correct. I also think the last one that clears update_data in module_update() is correct. I am not sure about the helper that releases suc, especially how suc->update_data is left alone by update_submodules(). Presumably the caller is responsible for releasing the resources held by update_data member alone, but the interaction makes me feel dirty. Luckily there is only one caller of suc_release(), so we can design however dirty interface for convenience, but still it feels wrong to design a release() helper that pretends to be usable by general public, yet they have to be aware of the fact that some members in the struct are still their responsibility to release. I dunno.
On Wed, Jul 20 2022, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> Add release functions for "struct module_list", "struct >> submodule_update_clone" and "struct update_data". > > OK, this does a lot of things, but in short, nobody bothered to free > module_list (i.e. list of cache entries borrowed from the index to > represent the paths we are interested in that are gitlinks), > update_data (which has module_list among other things that do not > need to be freed), and submodule_update_clone (which has > update_data, update_clone_data and failed_clones list) in the > original (in other words, these release helpers had to be invented > by this patch, not factored out from some codepaths that free them). > > I think the earlier part of the patch that deals with module_list is > correct. I also think the last one that clears update_data in > module_update() is correct. ... > I am not sure about the helper that releases suc, especially how > suc->update_data is left alone by update_submodules(). Presumably > the caller is responsible for releasing the resources held by > update_data member alone, but the interaction makes me feel dirty. > Luckily there is only one caller of suc_release(), so we can design > however dirty interface for convenience, but still it feels wrong to > design a release() helper that pretends to be usable by general > public, yet they have to be aware of the fact that some members in > the struct are still their responsibility to release. I dunno. But isn't this fine because that "update_data" member has a "const" on it: /* configuration parameters which are passed on to the children */ const struct update_data *update_data; I fix other similar ownership issues you pointed out for a re-roll I'm preparing, but I don't see why this one is a problem as long as that "const" is there, it's a slot we use for passing things in externally & to ferry it along.
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 7e9af9999bb..5d26e188560 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -182,6 +182,11 @@ struct module_list { }; #define MODULE_LIST_INIT { 0 } +static void module_list_release(struct module_list *ml) +{ + free(ml->entries); +} + static int module_list_compute(int argc, const char **argv, const char *prefix, struct pathspec *pathspec, @@ -243,7 +248,7 @@ static void module_list_active(struct module_list *list) active_modules.entries[active_modules.nr++] = ce; } - free(list->entries); + module_list_release(list); *list = active_modules; } @@ -307,6 +312,7 @@ static int module_list(int argc, const char **argv, const char *prefix) } ret = 0; cleanup: + module_list_release(&list); clear_pathspec(&pathspec); return ret; } @@ -465,6 +471,7 @@ static int module_foreach(int argc, const char **argv, const char *prefix) ret = 0; cleanup: + module_list_release(&list); clear_pathspec(&pathspec); return ret; } @@ -612,6 +619,7 @@ static int module_init(int argc, const char **argv, const char *prefix) ret = 0; cleanup: + module_list_release(&list); clear_pathspec(&pathspec); return ret; } @@ -791,6 +799,7 @@ static int module_status(int argc, const char **argv, const char *prefix) ret = 0; cleanup: + module_list_release(&list); clear_pathspec(&pathspec); return ret; } @@ -1387,6 +1396,7 @@ static int module_sync(int argc, const char **argv, const char *prefix) ret = 0; cleanup: + module_list_release(&list); clear_pathspec(&pathspec); return ret; } @@ -1541,6 +1551,7 @@ static int module_deinit(int argc, const char **argv, const char *prefix) ret = 0; cleanup: + module_list_release(&list); clear_pathspec(&pathspec); return ret; } @@ -1904,6 +1915,12 @@ struct submodule_update_clone { }; #define SUBMODULE_UPDATE_CLONE_INIT { 0 } +static void submodule_update_clone_release(struct submodule_update_clone *suc) +{ + free(suc->update_clone); + free(suc->failed_clones); +} + struct update_data { const char *prefix; const char *displaypath; @@ -1942,6 +1959,11 @@ struct update_data { .max_jobs = 1, \ } +static void update_data_release(struct update_data *ud) +{ + module_list_release(&ud->list); +} + static void next_submodule_warn_missing(struct submodule_update_clone *suc, struct strbuf *out, const char *displaypath) { @@ -2595,6 +2617,7 @@ static int update_submodules(struct update_data *update_data) } cleanup: + submodule_update_clone_release(&suc); string_list_clear(&update_data->references, 0); return res; } @@ -2694,6 +2717,7 @@ static int module_update(int argc, const char **argv, const char *prefix) if (module_list_compute(argc, argv, opt.prefix, &pathspec2, &list) < 0) { + module_list_release(&list); ret = 1; goto cleanup; } @@ -2710,10 +2734,12 @@ static int module_update(int argc, const char **argv, const char *prefix) info.flags |= OPT_QUIET; for_each_listed_submodule(&list, init_submodule_cb, &info); + module_list_release(&list); } ret = update_submodules(&opt); cleanup: + update_data_release(&opt); list_objects_filter_release(&filter_options); clear_pathspec(&pathspec); clear_pathspec(&pathspec2); @@ -2833,6 +2859,7 @@ static int absorb_git_dirs(int argc, const char **argv, const char *prefix) ret = 0; cleanup: clear_pathspec(&pathspec); + module_list_release(&list); return ret; } diff --git a/t/t6134-pathspec-in-submodule.sh b/t/t6134-pathspec-in-submodule.sh index 0f1cb49cedc..3a241f259de 100755 --- a/t/t6134-pathspec-in-submodule.sh +++ b/t/t6134-pathspec-in-submodule.sh @@ -2,6 +2,7 @@ test_description='test case exclude pathspec' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup a submodule' '
Add release functions for "struct module_list", "struct submodule_update_clone" and "struct update_data". Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- builtin/submodule--helper.c | 29 ++++++++++++++++++++++++++++- t/t6134-pathspec-in-submodule.sh | 1 + 2 files changed, 29 insertions(+), 1 deletion(-)