Re: [PATCH v2 02/24] submodule--helper: fix a leak in "clone_submodule"
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jul 19, 2022, 21:31 UTC
- Message-ID
- <xmqqpmi094cs.fsf@gitster.g>
- In-Reply-To
- <patch-v2-02.24-130a396b837-20220719T204458Z-avarab@gmail.com>
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
Show 22 quoted lines
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 73717be957c..4155d2450e0 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -1644,6 +1644,7 @@ static int clone_submodule(struct module_clone_data *clone_data)
> char *sm_alternate = NULL, *error_strategy = NULL;
> struct strbuf sb = STRBUF_INIT;
> struct child_process cp = CHILD_PROCESS_INIT;
> + char *to_free = NULL;
>
> submodule_name_to_gitdir(&sb, the_repository, clone_data->name);
> sm_gitdir = absolute_pathdup(sb.buf);
> @@ -1651,9 +1652,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 = to_free = strbuf_detach(&sb, NULL);
> } else {
> - clone_data->path = xstrdup(clone_data->path);
> + clone_data->path = clone_data->path;
> }WTH? Shouldn't the entire else-clause just go?
Show 9 quoted lines
> > if (validate_submodule_git_dir(sm_gitdir, clone_data->name) < 0) > @@ -1737,6 +1738,7 @@ static int clone_submodule(struct module_clone_data *clone_data) > strbuf_release(&sb); > free(sm_gitdir); > free(p); > + free(to_free); > return 0; > }
The caller passes clone_data to us, we may have stuffed an allocated piece of memory in there when we had to make it absolute, and we free it but let the clone_data structure smuggle the now-stale pointer out of the function, so that the caller may be able to abuse it?
That leaves a bad taste in my mouth. Doesn't it in yours?
If the caller is *NOT* allowed to rely on the value in clone_data->path after we return, perhaps
+ free(to_free); + clone_data->path = NULL;
But stepping back a bit, would it make more sense to introduce a local variable "path" and leave clone_data->path alone, after we decide to either compute an absolute path out of it, or we decide to use the path as is, i.e.
if (!is_absolute_path(...)) {
...
to_free = path = strbuf_detach(&sb, NULL);
} ... {
path = clone_data->path;
to_free = NULL;
}and after that, never use clone_data->path but work solely on the local "path"? A quick scan tells me that no line in the rest of the function passes the whole clone_data to other helpers, so it may just be a matter of s/clone_data->path/path/g perhaps?