Re: [PATCH 05/11] submodule--helper: free() leaking {run,capture}_command() argument

[Glen Choo <chooglen@google.com>]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> Free the "dir" member of "struct child_process" that various functions
> in submodule-helper.c allocate allocates with xstrdup().
>
> Since the "dir" argument is "const char *" let's keep a
> "char *to_free" variable around for this rather than casting when we
> call free().
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  builtin/submodule--helper.c | 41 +++++++++++++++++++++++++++----------
>  1 file changed, 30 insertions(+), 11 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index a8e439e59b8..2099c5774b2 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -2198,27 +2198,36 @@ static int is_tip_reachable(const char *path, struct object_id *oid)
>  	struct child_process cp = CHILD_PROCESS_INIT;
>  	struct strbuf rev = STRBUF_INIT;
>  	char *hex = oid_to_hex(oid);
> +	char *to_free;
> +	int ret;
>  
>  	cp.git_cmd = 1;
> -	cp.dir = xstrdup(path);
> +	cp.dir = to_free = xstrdup(path);
>  	cp.no_stderr = 1;
>  	strvec_pushl(&cp.args, "rev-list", "-n", "1", hex, "--not", "--all", NULL);
>  
>  	prepare_submodule_repo_env(&cp.env);
>  
> -	if (capture_command(&cp, &rev, GIT_MAX_HEXSZ + 1) || rev.len)
> -		return 0;
> +	if (capture_command(&cp, &rev, GIT_MAX_HEXSZ + 1) || rev.len) {
> +		ret = 0;
> +		goto cleanup;
> +	}
>  
> -	return 1;
> +	ret = 1;
> +cleanup:
> +	free(to_free);
> +	return ret;
>  }
>  
>  static int fetch_in_submodule(const char *module_path, int depth, int quiet, struct object_id *oid)
>  {
>  	struct child_process cp = CHILD_PROCESS_INIT;
> +	char *to_free;
> +	int ret;
>  
>  	prepare_submodule_repo_env(&cp.env);
>  	cp.git_cmd = 1;
> -	cp.dir = xstrdup(module_path);
> +	cp.dir = to_free = xstrdup(module_path);
>  
>  	strvec_push(&cp.args, "fetch");
>  	if (quiet)
> @@ -2232,7 +2241,9 @@ static int fetch_in_submodule(const char *module_path, int depth, int quiet, str
>  		free(remote);
>  	}
>  
> -	return run_command(&cp);
> +	ret = run_command(&cp);
> +	free(to_free);
> +	return ret;
>  }
>  
>  static int run_update_command(struct update_data *ud, int subforce)
> @@ -2240,6 +2251,8 @@ static int run_update_command(struct update_data *ud, int subforce)
>  	struct child_process cp = CHILD_PROCESS_INIT;
>  	char *oid = oid_to_hex(&ud->oid);
>  	int must_die_on_failure = 0;
> +	char *to_free;
> +	int ret;
>  
>  	switch (ud->update_strategy.type) {
>  	case SM_UPDATE_CHECKOUT:
> @@ -2273,7 +2286,7 @@ static int run_update_command(struct update_data *ud, int subforce)
>  	}
>  	strvec_push(&cp.args, oid);
>  
> -	cp.dir = xstrdup(ud->sm_path);
> +	cp.dir = to_free = xstrdup(ud->sm_path);
>  	prepare_submodule_repo_env(&cp.env);
>  	if (run_command(&cp)) {
>  		switch (ud->update_strategy.type) {
> @@ -2301,11 +2314,14 @@ static int run_update_command(struct update_data *ud, int subforce)
>  			exit(128);
>  
>  		/* the command failed, but update must continue */
> -		return 1;
> +		ret = 1;
> +		goto cleanup;
>  	}
>  
> -	if (ud->quiet)
> -		return 0;
> +	if (ud->quiet) {
> +		ret = 0;
> +		goto cleanup;
> +	}
>  
>  	switch (ud->update_strategy.type) {
>  	case SM_UPDATE_CHECKOUT:
> @@ -2329,7 +2345,10 @@ static int run_update_command(struct update_data *ud, int subforce)
>  		    submodule_strategy_to_string(&ud->update_strategy));
>  	}
>  
> -	return 0;
> +	ret = 0;
> +cleanup:
> +	free(to_free);
> +	return ret;
>  }
>  
>  static int run_update_procedure(struct update_data *ud)

I assume I'm missing something, but couldn't we achieve the same result
by just removing the xstrdup() calls? i.e. we only leak .dir (which we
don't clear because it's const), so couldn't we just assign to it
without xstrdup() and not have to worry about free()-ing it?

I didn't see a correctness reason for us to xstrdup(), and indeed, t7406
passes with the change I just described (which I believe covers all of
the sites here). In fact, we already have one site that does exactly
this in the recursive part of update_submodule():

	if (update_data->recursive) {
		struct child_process cp = CHILD_PROCESS_INIT;
		struct update_data next = *update_data;
		int res;

		next.prefix = NULL;
		oidcpy(&next.oid, null_oid());
		oidcpy(&next.suboid, null_oid());

		cp.dir = update_data->sm_path;

Tangentially related question (primarily for my own learning): in all of
these hunks, the string being xstrdup()-ed is update_data.sm_path, which
is only ever set in update_submodules():

		update_data->sm_path = ucd.sub->path;

where ucd.sub->path is a "const char *". So we never have to worry about
free()-ing update_data.sm_path, right? (Your patch doesn't attempt to
free() it, which sounds correct.)