From ffc5c046fb4f47e149687963706bb2390f4eed61 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 20 Nov 2024 14:39:39 +0100 Subject: git: refactor alias handling to use a `struct strvec` 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 Signed-off-by: Junio C Hamano --- git.c | 58 ++++++++++++++++++++++++++++++++-------------------------- 1 file changed, 32 insertions(+), 26 deletions(-) (limited to 'git.c') diff --git a/git.c b/git.c index c2c1b8e22c..88356afe5f 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; } -- cgit v1.2.3 From 1dd7c32daa7d8a4b71d9204f1edbb09a7241e18f Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 20 Nov 2024 14:39:40 +0100 Subject: git: refactor builtin handling to use a `struct strvec` Similar as with the preceding commit, `handle_builtin()` does not properly track lifetimes of the `argv` array and its strings. As it may end up modifying the array this can lead to memory leaks in case it contains allocated strings. Refactor the function to use a `struct strvec` instead. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- git.c | 66 ++++++++++++++++++++++++-------------------------- t/t0211-trace2-perf.sh | 2 +- 2 files changed, 32 insertions(+), 36 deletions(-) (limited to 'git.c') diff --git a/git.c b/git.c index 88356afe5f..159dd45b08 100644 --- a/git.c +++ b/git.c @@ -696,63 +696,57 @@ void load_builtin_commands(const char *prefix, struct cmdnames *cmds) } #ifdef STRIP_EXTENSION -static void strip_extension(const char **argv) +static void strip_extension(struct strvec *args) { size_t len; - if (strip_suffix(argv[0], STRIP_EXTENSION, &len)) - argv[0] = xmemdupz(argv[0], len); + if (strip_suffix(args->v[0], STRIP_EXTENSION, &len)) { + char *stripped = xmemdupz(args->v[0], len); + strvec_replace(args, 0, stripped); + free(stripped); + } } #else #define strip_extension(cmd) #endif -static void handle_builtin(int argc, const char **argv) +static void handle_builtin(struct strvec *args) { - struct strvec args = STRVEC_INIT; - const char **argv_copy = NULL; const char *cmd; struct cmd_struct *builtin; - strip_extension(argv); - cmd = argv[0]; + strip_extension(args); + cmd = args->v[0]; /* Turn "git cmd --help" into "git help --exclude-guides cmd" */ - if (argc > 1 && !strcmp(argv[1], "--help")) { - int i; - - argv[1] = argv[0]; - argv[0] = cmd = "help"; - - for (i = 0; i < argc; i++) { - strvec_push(&args, argv[i]); - if (!i) - strvec_push(&args, "--exclude-guides"); - } + if (args->nr > 1 && !strcmp(args->v[1], "--help")) { + const char *exclude_guides_arg[] = { "--exclude-guides" }; + + strvec_replace(args, 1, args->v[0]); + strvec_replace(args, 0, "help"); + cmd = "help"; + strvec_splice(args, 2, 0, exclude_guides_arg, + ARRAY_SIZE(exclude_guides_arg)); + } - argc++; + builtin = get_builtin(cmd); + if (builtin) { + const char **argv_copy = NULL; + int ret; /* * `run_builtin()` will modify the argv array, so we need to * create a shallow copy such that we can free all of its * strings. */ - CALLOC_ARRAY(argv_copy, argc + 1); - COPY_ARRAY(argv_copy, args.v, argc); + if (args->nr) + DUP_ARRAY(argv_copy, args->v, args->nr + 1); - argv = argv_copy; - } - - builtin = get_builtin(cmd); - if (builtin) { - int ret = run_builtin(builtin, argc, argv, the_repository); - strvec_clear(&args); + ret = run_builtin(builtin, args->nr, argv_copy, the_repository); + strvec_clear(args); free(argv_copy); exit(ret); } - - strvec_clear(&args); - free(argv_copy); } static void execv_dashed_external(const char **argv) @@ -815,7 +809,7 @@ static int run_argv(struct strvec *args) * process. */ if (!done_alias) - handle_builtin(args->nr, args->v); + handle_builtin(args); else if (get_builtin(args->v[0])) { struct child_process cmd = CHILD_PROCESS_INIT; int i; @@ -916,8 +910,10 @@ int cmd_main(int argc, const char **argv) * that one cannot handle it. */ if (skip_prefix(cmd, "git-", &cmd)) { - argv[0] = cmd; - handle_builtin(argc, argv); + strvec_push(&args, cmd); + strvec_pushv(&args, argv + 1); + handle_builtin(&args); + strvec_clear(&args); die(_("cannot handle %s as a builtin"), cmd); } diff --git a/t/t0211-trace2-perf.sh b/t/t0211-trace2-perf.sh index dddc130560..91260ce97e 100755 --- a/t/t0211-trace2-perf.sh +++ b/t/t0211-trace2-perf.sh @@ -2,7 +2,7 @@ test_description='test trace2 facility (perf target)' -TEST_PASSES_SANITIZE_LEAK=false +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh # Turn off any inherited trace2 settings for this test. -- cgit v1.2.3 From 7720dbe99b303b3d658898587e02d7cf224a93c3 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 20 Nov 2024 14:39:45 +0100 Subject: help: fix leaking return value from `help_unknown_cmd()` While `help_unknown_cmd()` would usually die on an unknown command, it instead returns an autocorrected command when "help.autocorrect" is set. But while the function is declared to return a string constant, it actually returns an allocated string in that case. Callers thus aren't aware that they have to free the string, leading to a memory leak. Fix the function return type to be non-constant and free the returned value at its only callsite. Note that we cannot simply take ownership of `main_cmds.names[0]->name` and then eventually free it. This is because the `struct cmdname` is using a flex array to allocate the name, so the name pointer points into the middle of the structure and thus cannot be freed. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- git.c | 4 +++- help.c | 7 +++---- help.h | 2 +- 3 files changed, 7 insertions(+), 6 deletions(-) (limited to 'git.c') diff --git a/git.c b/git.c index 159dd45b08..46b3c740c5 100644 --- a/git.c +++ b/git.c @@ -961,7 +961,9 @@ int cmd_main(int argc, const char **argv) exit(1); } if (!done_help) { - strvec_replace(&args, 0, help_unknown_cmd(cmd)); + char *assumed = help_unknown_cmd(cmd); + strvec_replace(&args, 0, assumed); + free(assumed); cmd = args.v[0]; done_help = 1; } else { diff --git a/help.c b/help.c index 8b56cd6e25..8a830ba35c 100644 --- a/help.c +++ b/help.c @@ -612,7 +612,7 @@ static const char bad_interpreter_advice[] = N_("'%s' appears to be a git command, but we were not\n" "able to execute it. Maybe git-%s is broken?"); -const char *help_unknown_cmd(const char *cmd) +char *help_unknown_cmd(const char *cmd) { struct help_unknown_cmd_config cfg = { 0 }; int i, n, best_similarity = 0; @@ -695,9 +695,8 @@ const char *help_unknown_cmd(const char *cmd) ; /* still counting */ } if (cfg.autocorrect && n == 1 && SIMILAR_ENOUGH(best_similarity)) { - const char *assumed = main_cmds.names[0]->name; - main_cmds.names[0] = NULL; - cmdnames_release(&main_cmds); + char *assumed = xstrdup(main_cmds.names[0]->name); + fprintf_ln(stderr, _("WARNING: You called a Git command named '%s', " "which does not exist."), diff --git a/help.h b/help.h index e716ee27ea..67207b3073 100644 --- a/help.h +++ b/help.h @@ -32,7 +32,7 @@ void list_all_other_cmds(struct string_list *list); void list_cmds_by_category(struct string_list *list, const char *category); void list_cmds_by_config(struct string_list *list); -const char *help_unknown_cmd(const char *cmd); +char *help_unknown_cmd(const char *cmd); void load_command_list(const char *prefix, struct cmdnames *main_cmds, struct cmdnames *other_cmds); -- cgit v1.2.3