Patrick Steinhardt <ps@xxxxxx> writes:
> 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@xxxxxx>
> ---
> git.c | 66 ++++++++++++++++++++++++--------------------------
> t/t0211-trace2-perf.sh | 2 +-
> 2 files changed, 32 insertions(+), 36 deletions(-)
>
> diff --git a/git.c b/git.c
> index 88356afe5fb568ccc147f055e3ab253c53a1befa..159dd45b08204c4a89d1dc4ab6990978e2454eb6 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);
> }
Just want to give you a little shout out how you've reorganized the code
and it now makes a lot more sense how `argv_copy` is used by putting
it inside the `if (builtin)` code block.
--
Toon
