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 <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/git.c b/git.c
index 88356afe5fb568ccc147f055e3ab253c53a1befa..159dd45b08204c4a89d1dc4ab6990978e2454eb6 100644 (file)
--- 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 dddc130560ba6150fc5f5eac36c65ff76a2d31a1..91260ce97e5bdb39550a9e1e4abbc4d5fea48a21 100755 (executable)
--- 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.