Glen Choo <chooglen@xxxxxxxxxx> writes:
> Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 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.
