Re: [PATCH v5 03/17] submodule--helper: fix most "struct pathspec" memory leaks

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Wed, Aug 03 2022, Glen Choo wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> Call clear_pathspec() at the end of various functions that work with
>> and allocate a "struct pathspec".
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>  builtin/submodule--helper.c | 74 +++++++++++++++++++++++++------------
>>  1 file changed, 51 insertions(+), 23 deletions(-)
>>
>> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
>> index d958da7dddc..92d32f2877f 100644
>> --- a/builtin/submodule--helper.c
>> +++ b/builtin/submodule--helper.c
>> @@ -367,7 +367,7 @@ static void runcommand_in_submodule_cb(const struct cache_entry *list_item,
>>  static int module_foreach(int argc, const char **argv, const char *prefix)
>>  {
>>  	struct foreach_cb info = FOREACH_CB_INIT;
>> -	struct pathspec pathspec;
>> +	struct pathspec pathspec = { 0 };
>
> Out of curiousity, does this zero-initialization do anything for us
> leaks-wise?

No, because if we leak module_list_compute() must have filled the
pathspec with something it allocated, but...

>>  	struct module_list list = MODULE_LIST_INIT;
>>  	struct option module_foreach_options[] = {
>>  		OPT__QUIET(&info.quiet, N_("suppress output of entering each submodule command")),
>> @@ -379,12 +379,13 @@ static int module_foreach(int argc, const char **argv, const char *prefix)
>>  		N_("git submodule foreach [--quiet] [--recursive] [--] <command>"),
>>  		NULL
>>  	};
>> +	int ret = 1;
>>  
>>  	argc = parse_options(argc, argv, prefix, module_foreach_options,
>>  			     git_submodule_helper_usage, 0);
>>  
>>  	if (module_list_compute(0, NULL, prefix, &pathspec, &list) < 0)
>> -		return 1;
>> +		goto cleanup;

...if we don't initialize it then we can't "goto cleanup".

Now, right now this is redundant, we could just "return 1", as we don't
have a pathspec yet.

But it's generally worth just using the same "cleanup" pattern
everywhere, and not worrying about in your cleanunp code that you only
init'd N/TOTAL variables already.