Re: [PATCH 03/11] submodule--helper: fix "module_clone_data" memory leaks

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
>> index 73717be957c..23ab9c7e349 100644
>> --- a/builtin/submodule--helper.c
>> +++ b/builtin/submodule--helper.c
>> @@ -1511,6 +1511,7 @@ static int module_deinit(int argc, const char **argv, const char *prefix)
>>  struct module_clone_data {
>>  	const char *prefix;
>>  	const char *path;
>> +	char *path_free;
>>  	const char *name;
>>  	const char *url;
>>  	const char *depth;
>> @@ -1527,6 +1528,11 @@ struct module_clone_data {
>>  	.single_branch = -1, \
>>  }
>>  
>> +static void module_clone_data_release(struct module_clone_data *cd)
>> +{
>> +	free(cd->path_free);
>> +}
>> +
>>  struct submodule_alternate_setup {
>>  	const char *submodule_name;
>>  	enum SUBMODULE_ALTERNATE_ERROR_MODE {
>> @@ -1651,9 +1657,9 @@ static int clone_submodule(struct module_clone_data *clone_data)
>>  
>>  	if (!is_absolute_path(clone_data->path)) {
>>  		strbuf_addf(&sb, "%s/%s", get_git_work_tree(), clone_data->path);
>> -		clone_data->path = strbuf_detach(&sb, NULL);
>> +		clone_data->path = clone_data->path_free = strbuf_detach(&sb, NULL);
>>  	} else {
>> -		clone_data->path = xstrdup(clone_data->path);
>> +		clone_data->path = clone_data->path_free = xstrdup(clone_data->path);
>>  	}
>
> Hm, having .path_free doesn't seem necessary. If I'm reading the code
> correctly,
>
> - in module_clone() we set clone_data.path from argv
> - then we unconditionally call clone_submodule(), which does all of the
>   hard work
> - in clone_submodule(), we always hit this conditional, which means that
>   past this point, clone_data.path is always free()-able.
>
> which suggests that we don't need clone_data.path_free at all. I suspect
> that just this
>
>    static void module_clone_data_release(struct module_clone_data *cd)
>    {
>    	free(cd->path);
>    }
>
> is enough to plug the leak (though admittedly, I haven't properly tested
> the leak because it's been a PITA to set up locally).
>
> That's a pretty hacky suggestion though, since we're still using the
> same member to hold free()-able and non-free()-able memory....