Thread (22 messages) 22 messages, 3 authors, 2023-01-07

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?
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help