Subject: Re: [PATCH v2 02/24] submodule--helper: fix a leak in "clone_submodule"

From: Junio C Hamano <[email-masked]>
Date: Tue, 19 Jul 2022 14:31:31 -0700

Ævar Arnfjörð Bjarmason  <[email-masked]> 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?