On Wed, Jul 13 2022, Glen Choo wrote:
> 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.
I suppose so, it might make some things simpler, of course at the cost
of needlessly duplicating things.
But we also have various common patterns such as string_lists where some
elements are dup'd, some aren't, and need to deal with that. I think
just having common idioms for tracking the dupe is usually better,
e.g. in the case of a string list stick the pointer to free in "util".
I think in this case the patch as-is is better than your suggestions,
because it's a less fragile pattern, we explicitly mark when we dup
something that's sometimes a dup, and sometimes not.
Whereas if we do it with the xstrdup() at the start it requires more
moving things around, and if we have a new user who parses the same
argument we'll bug out on that free().
What do you think?
