Re: [PATCH] am: don't pass strvec to apply_parse_options()
flat view
From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2022-12-13 08:43:16
On Tue, Dec 13 2022, René Scharfe wrote:
quoted hunk ↗ jump to hunk
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 <redacted> --- 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?