git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Patrick Steinhardt <ps@pks.im>
Date
Nov 20, 2024, 13:39 UTC
Message-ID
<20241120-b4-pks-leak-fixes-pt10-v3-10-d67f08f45c74@pks.im>
In-Reply-To
<20241120-b4-pks-leak-fixes-pt10-v3-0-d67f08f45c74@pks.im>

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 c2c1b8e22c2d91824ad6d631ea9374424ab53435..88356afe5fb568ccc147f055e3ab253c53a1befa 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);
+
+		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 854d59ec58c25ab2cfb58113bf5ea8d670829fe0..2a6f39ad9c81958d5152139995feab57c4371363 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.274.g962d0b743d.dirty
Previous: Patrick SteinhardtNext: Patrick Steinhardt
Message 15 of 33 in “Memory leak fixes (pt.10, final)”
  1. 00/27 Memory leak fixes (pt.10, final)Patrick Steinhardt, Nov 20, 2024
  2. 01/27 builtin/blame: fix leaking blame entries with `--incremental`Patrick Steinhardt, Nov 20, 2024
  3. 02/27 bisect: fix leaking good/bad terms when reading multipe timesPatrick Steinhardt, Nov 20, 2024
  4. 04/27 bisect: fix leaking `current_bad_oid`Patrick Steinhardt, Nov 20, 2024
  5. 03/27 bisect: fix leaking string in `handle_bad_merge_base()`Patrick Steinhardt, Nov 20, 2024
  6. 05/27 bisect: fix multiple leaks in `bisect_next_all()`Patrick Steinhardt, Nov 20, 2024
  7. 06/27 bisect: fix leaking commit list items in `check_merge_base()`Patrick Steinhardt, Nov 20, 2024
  8. 07/27 bisect: fix various cases where we leak commit list itemsPatrick Steinhardt, Nov 20, 2024
  9. Jeff KingNov 25, 2024
  10. Patrick SteinhardtNov 25, 2024
  11. Jeff KingNov 25, 2024
  12. Patrick SteinhardtNov 25, 2024
  13. 08/27 line-log: fix leak when rewriting commit parentsPatrick Steinhardt, Nov 20, 2024
  14. 09/27 strvec: introduce new `strvec_splice()` functionPatrick Steinhardt, Nov 20, 2024
  15. 10/27 git: refactor alias handling to use a `struct strvec`Patrick Steinhardt, Nov 20, 2024
  16. 11/27 git: refactor builtin handling to use a `struct strvec`Patrick Steinhardt, Nov 20, 2024
  17. 12/27 split-index: fix memory leak in `move_cache_to_base_index()`Patrick Steinhardt, Nov 20, 2024
  18. 13/27 builtin/sparse-checkout: fix leaking sanitized patternsPatrick Steinhardt, Nov 20, 2024
  19. 14/27 help: refactor to not use globals for reading configPatrick Steinhardt, Nov 20, 2024
  20. 15/27 help: fix leaking `struct cmdnames`Patrick Steinhardt, Nov 20, 2024
  21. 16/27 help: fix leaking return value from `help_unknown_cmd()`Patrick Steinhardt, Nov 20, 2024
  22. 17/27 builtin/help: fix leaks in `check_git_cmd()`Patrick Steinhardt, Nov 20, 2024
  23. 18/27 builtin/init-db: fix leaking directory pathsPatrick Steinhardt, Nov 20, 2024
  24. 20/27 t/helper: fix leaking commit graph in "read-graph" subcommandPatrick Steinhardt, Nov 20, 2024
  25. 19/27 builtin/branch: fix leaking sorting optionsPatrick Steinhardt, Nov 20, 2024
  26. 21/27 global: drop `UNLEAK()` annotationPatrick Steinhardt, Nov 20, 2024
  27. 22/27 git-compat-util: drop now-unused `UNLEAK()` macroPatrick Steinhardt, Nov 20, 2024
  28. 23/27 t5601: work around leak sanitizer issuePatrick Steinhardt, Nov 20, 2024
  29. 24/27 t: mark some tests as leak freePatrick Steinhardt, Nov 20, 2024
  30. 25/27 t: remove unneeded !SANITIZE_LEAK prerequisitesPatrick Steinhardt, Nov 20, 2024
  31. 26/27 test-lib: unconditionally enable leak checkingPatrick Steinhardt, Nov 20, 2024
  32. 27/27 t: remove TEST_PASSES_SANITIZE_LEAK annotationsPatrick Steinhardt, Nov 20, 2024
  33. Toon ClaesNov 21, 2024

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.