Re: [PATCH 10/26] git: refactor alias handling to use a `struct strvec`

["Rubén Justo" <rjusto@gmail.com>]

On Wed, Nov 06, 2024 at 04:10:47PM +0100, Patrick Steinhardt wrote:
> In `handle_alias()` we use both `argcp` and `argv` as in-out parameters.
> Callers mostly pass through the static array from `main()`, but once we
> handle an alias we replace it with an allocated array that may contain
> some allocated strings. Callers do not handle this scenario at all and
> thus leak memory.
> 
> We could in theory handle the lifetime of `argv` in a hacky fashion by
> letting callers free it in case they see that an alias was handled. But
> while that would likely work, we still wouldn't be able to easily handle
> the lifetime of strings referenced by `argv`.
> 
> Refactor the code to instead use a `struct strvec`, which effectively
> removes the need for us to manually track lifetimes.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  git.c            | 58 ++++++++++++++++++++++++++----------------------
>  t/t0014-alias.sh |  1 +
>  2 files changed, 33 insertions(+), 26 deletions(-)
> 
> diff --git a/git.c b/git.c
> index c2c1b8e22c2..88356afe5fb 100644
> --- a/git.c
> +++ b/git.c
> @@ -362,7 +362,7 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
>  	return (*argv) - orig_argv;
>  }
>  
> -static int handle_alias(int *argcp, const char ***argv)
> +static int handle_alias(struct strvec *args)
>  {
>  	int envchanged = 0, ret = 0, saved_errno = errno;
>  	int count, option_count;
> @@ -370,10 +370,10 @@ static int handle_alias(int *argcp, const char ***argv)
>  	const char *alias_command;
>  	char *alias_string;
>  
> -	alias_command = (*argv)[0];
> +	alias_command = args->v[0];
>  	alias_string = alias_lookup(alias_command);
>  	if (alias_string) {
> -		if (*argcp > 1 && !strcmp((*argv)[1], "-h"))
> +		if (args->nr > 1 && !strcmp(args->v[1], "-h"))
>  			fprintf_ln(stderr, _("'%s' is aliased to '%s'"),
>  				   alias_command, alias_string);
>  		if (alias_string[0] == '!') {
> @@ -390,7 +390,7 @@ static int handle_alias(int *argcp, const char ***argv)
>  			child.wait_after_clean = 1;
>  			child.trace2_child_class = "shell_alias";
>  			strvec_push(&child.args, alias_string + 1);
> -			strvec_pushv(&child.args, (*argv) + 1);
> +			strvec_pushv(&child.args, args->v + 1);
>  
>  			trace2_cmd_alias(alias_command, child.args.v);
>  			trace2_cmd_name("_run_shell_alias_");
> @@ -423,15 +423,13 @@ static int handle_alias(int *argcp, const char ***argv)
>  		trace_argv_printf(new_argv,
>  				  "trace: alias expansion: %s =>",
>  				  alias_command);
> -
> -		REALLOC_ARRAY(new_argv, count + *argcp);
> -		/* insert after command name */
> -		COPY_ARRAY(new_argv + count, *argv + 1, *argcp);
> -
>  		trace2_cmd_alias(alias_command, new_argv);
>  
> -		*argv = new_argv;
> -		*argcp += count - 1;
> +		/* Replace the alias with the new arguments. */
> +		strvec_splice(args, 0, 1, new_argv, count);

So, we take advantage here of the `replacement_len` argument for
`strvec_splice()`.  OK. 

> +
> +		free(alias_string);
> +		free(new_argv);
>  
>  		ret = 1;
>  	}
> @@ -800,10 +798,10 @@ static void execv_dashed_external(const char **argv)
>  		exit(128);
>  }
>  
> -static int run_argv(int *argcp, const char ***argv)
> +static int run_argv(struct strvec *args)
>  {
>  	int done_alias = 0;
> -	struct string_list cmd_list = STRING_LIST_INIT_NODUP;
> +	struct string_list cmd_list = STRING_LIST_INIT_DUP;
>  	struct string_list_item *seen;
>  
>  	while (1) {
> @@ -817,8 +815,8 @@ static int run_argv(int *argcp, const char ***argv)
>  		 * process.
>  		 */
>  		if (!done_alias)
> -			handle_builtin(*argcp, *argv);
> -		else if (get_builtin(**argv)) {
> +			handle_builtin(args->nr, args->v);
> +		else if (get_builtin(args->v[0])) {
>  			struct child_process cmd = CHILD_PROCESS_INIT;
>  			int i;
>  
> @@ -834,8 +832,8 @@ static int run_argv(int *argcp, const char ***argv)
>  			commit_pager_choice();
>  
>  			strvec_push(&cmd.args, "git");
> -			for (i = 0; i < *argcp; i++)
> -				strvec_push(&cmd.args, (*argv)[i]);
> +			for (i = 0; i < args->nr; i++)
> +				strvec_push(&cmd.args, args->v[i]);
>  
>  			trace_argv_printf(cmd.args.v, "trace: exec:");
>  
> @@ -850,13 +848,13 @@ static int run_argv(int *argcp, const char ***argv)
>  			i = run_command(&cmd);
>  			if (i >= 0 || errno != ENOENT)
>  				exit(i);
> -			die("could not execute builtin %s", **argv);
> +			die("could not execute builtin %s", args->v[0]);
>  		}
>  
>  		/* .. then try the external ones */
> -		execv_dashed_external(*argv);
> +		execv_dashed_external(args->v);
>  
> -		seen = unsorted_string_list_lookup(&cmd_list, *argv[0]);
> +		seen = unsorted_string_list_lookup(&cmd_list, args->v[0]);
>  		if (seen) {
>  			int i;
>  			struct strbuf sb = STRBUF_INIT;
> @@ -873,14 +871,14 @@ static int run_argv(int *argcp, const char ***argv)
>  			      " not terminate:%s"), cmd_list.items[0].string, sb.buf);
>  		}
>  
> -		string_list_append(&cmd_list, *argv[0]);
> +		string_list_append(&cmd_list, args->v[0]);
>  
>  		/*
>  		 * It could be an alias -- this works around the insanity
>  		 * of overriding "git log" with "git show" by having
>  		 * alias.log = show
>  		 */
> -		if (!handle_alias(argcp, argv))
> +		if (!handle_alias(args))
>  			break;
>  		done_alias = 1;
>  	}
> @@ -892,6 +890,7 @@ static int run_argv(int *argcp, const char ***argv)
>  
>  int cmd_main(int argc, const char **argv)
>  {
> +	struct strvec args = STRVEC_INIT;
>  	const char *cmd;
>  	int done_help = 0;
>  
> @@ -951,25 +950,32 @@ int cmd_main(int argc, const char **argv)
>  	 */
>  	setup_path();
>  
> +	for (size_t i = 0; i < argc; i++)
> +		strvec_push(&args, argv[i]);
> +
>  	while (1) {
> -		int was_alias = run_argv(&argc, &argv);
> +		int was_alias = run_argv(&args);
>  		if (errno != ENOENT)
>  			break;
>  		if (was_alias) {
>  			fprintf(stderr, _("expansion of alias '%s' failed; "
>  					  "'%s' is not a git command\n"),
> -				cmd, argv[0]);
> +				cmd, args.v[0]);
> +			strvec_clear(&args);
>  			exit(1);
>  		}
>  		if (!done_help) {
> -			cmd = argv[0] = help_unknown_cmd(cmd);
> +			strvec_replace(&args, 0, help_unknown_cmd(cmd));
> +			cmd = args.v[0];
>  			done_help = 1;
> -		} else
> +		} else {
>  			break;
> +		}
>  	}
>  
>  	fprintf(stderr, _("failed to run command '%s': %s\n"),
>  		cmd, strerror(errno));
> +	strvec_clear(&args);
>  
>  	return 1;
>  }
> diff --git a/t/t0014-alias.sh b/t/t0014-alias.sh
> index 854d59ec58c..2a6f39ad9c8 100755
> --- a/t/t0014-alias.sh
> +++ b/t/t0014-alias.sh
> @@ -2,6 +2,7 @@
>  
>  test_description='git command aliasing'
>  
> +TEST_PASSES_SANITIZE_LEAK=true
>  . ./test-lib.sh
>  
>  test_expect_success 'nested aliases - internal execution' '
> -- 
> 2.47.0.229.g8f8d6eee53.dirty
>