Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes:
> 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".
Hm, sounds fair. "Sometimes dup and sometimes not" sounds like an
inevitability. I'm not experienced enough to know better, and folks
whose opinion I sought seem to agree with you.
> 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?
Frankly I'm ok with moving things around; I think the code could use
a little cleaning up :) But yeah, I think my suggestion isn't so great -
it's a bit weird to keep around an auto variable that exists only to be
dup-ed to the thing we care about. We can forget about that.
I do think that it's worth avoiding the "sometimes dup, sometimes not"
pattern if we can, though (of course these are just my non-C instincts
talking), and we can do that here if we just choose not to assign back
to .path. Something like:
struct module_clone_data {
const char *prefix;
- const char *path;
+ char *path;
+ const char *path_argv;
...
if (!is_absolute_path(clone_data->path)) {
- strbuf_addf(&sb, "%s/%s", get_git_work_tree(), clone_data->path);
+ strbuf_addf(&sb, "%s/%s", get_git_work_tree(), clone_data->path_argv);
+ clone_data->path = strbuf_detach(&sb, NULL);
} else {
- clone_data->path = xstrdup(clone_data->path);
+ clone_data->path = xstrdup(clone_data->path_argv);
}
would be clearer to me since the const pointer never points to something
that the struct actually owns.
But if the "= .to_free = " idiom is well-understood and accepted to the
point that we don't need to actively avoid "sometimes dup, sometimes
not", then we should drop my suggestion and just go with your patch :)
