Re: [PATCH] am: don't pass strvec to apply_parse_options()
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Dec 13, 2022, 08:37 UTC
- Message-ID
- <221213.86mt7r4ru2.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <baf93e4a-7f05-857c-e551-09675496c03c@web.de>
On Tue, Dec 13 2022, René Scharfe wrote:
Show 49 quoted lines
> apply_parse_options() passes the array of argument strings to
> parse_options(), which removes recognized options. The removed strings
> are not freed, though.
>
> Make a copy of the strvec to pass to the function to retain the pointers
> of its strings, so we release them all at the end.
>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
> builtin/am.c | 12 +++++++++++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/am.c b/builtin/am.c
> index 30c9b3a9cd..dddf1b9af0 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -1476,6 +1476,7 @@ static int run_apply(const struct am_state *state, const char *index_file)
> int res, opts_left;
> int force_apply = 0;
> int options = 0;
> + const char **apply_argv;
>
> if (init_apply_state(&apply_state, the_repository, NULL))
> BUG("init_apply_state() failed");
> @@ -1483,7 +1484,15 @@ static int run_apply(const struct am_state *state, const char *index_file)
> strvec_push(&apply_opts, "apply");
> strvec_pushv(&apply_opts, state->git_apply_opts.v);
>
> - opts_left = apply_parse_options(apply_opts.nr, apply_opts.v,
> + /*
> + * Build a copy that apply_parse_options() can rearrange.
> + * apply_opts.v keeps referencing the allocated strings for
> + * strvec_clear() to release.
> + */
> + ALLOC_ARRAY(apply_argv, apply_opts.nr);
> + COPY_ARRAY(apply_argv, apply_opts.v, apply_opts.nr);
> +
> + opts_left = apply_parse_options(apply_opts.nr, apply_argv,
> &apply_state, &force_apply, &options,
> NULL);
>
> @@ -1513,6 +1522,7 @@ static int run_apply(const struct am_state *state, const char *index_file)
> strvec_clear(&apply_paths);
> strvec_clear(&apply_opts);
> clear_apply_state(&apply_state);
> + free(apply_argv);
>
> if (res)
> return res;I don't mind this going in, but it really feels like a bit of a dirty hack.
We have widespread leaks all over the place due to this idiom. I.e. parse_options() and a couple of other APIs expect that they can munge the "argv", which is fine if it arrives via main(), but not if we're editing our own strvecs.
I think less of a hack is to teach the eventual parse_options() that when it munges it it should free() it. I did that for the revisions API in f92dbdbc6a8 (revisions API: don't leak memory on argv elements that need free()-ing, 2022-08-02).
What do you think?