git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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?
Previous: René ScharfeNext: René Scharfe
Message 2 of 24 in “am: don't pass strvec to apply_parse_options()”
  1. am: don't pass strvec to apply_parse_options()René Scharfe, Dec 13, 2022
  2. Ævar Arnfjörð BjarmasonDec 13, 2022
  3. René ScharfeDec 13, 2022
  4. Ævar Arnfjörð BjarmasonDec 14, 2022
  5. 0/5 strvec: add a "nodup" mode, fix memory leaksÆvar Arnfjörð Bjarmason, Dec 15, 2022
  6. 2/5 various: add missing strvec_clear()Ævar Arnfjörð Bjarmason, Dec 15, 2022
  7. 1/5 builtin/annotate.c: simplify for strvec APIÆvar Arnfjörð Bjarmason, Dec 15, 2022
  8. René ScharfeDec 17, 2022
  9. 3/5 strvec API: add a "STRVEC_INIT_NODUP"Ævar Arnfjörð Bjarmason, Dec 15, 2022
  10. René ScharfeDec 17, 2022
  11. 5/5 strvec API users: fix more leaks by using "STRVEC_INIT_NODUP"Ævar Arnfjörð Bjarmason, Dec 15, 2022
  12. René ScharfeDec 17, 2022
  13. 4/5 strvec API users: fix leaks by using "STRVEC_INIT_NODUP"Ævar Arnfjörð Bjarmason, Dec 15, 2022
  14. René ScharfeDec 17, 2022
  15. René ScharfeDec 17, 2022
  16. Jeff KingDec 17, 2022
  17. Ævar Arnfjörð BjarmasonDec 19, 2022
  18. Jeff KingJan 7, 2023
  19. René ScharfeDec 17, 2022
  20. Jeff KingDec 17, 2022
  21. René ScharfeDec 17, 2022
  22. Jeff KingDec 17, 2022
  23. Junio C HamanoDec 18, 2022
  24. Junio C HamanoDec 20, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.