Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes:
> 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?
>
> 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?
