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

> Glen Choo <chooglen@google.com> writes:
>
>> Æ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....
>
> Ah, here's a wacky idea (whose feasibility I haven't thought about at
> all) that would make things a lot cleaner. If we had something like
> OPT_STRING_DUP, that xstrdup()-s the value from argv when we parse it,
> then we could drop the "const" from clone_data.path and just free() it.