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

Re: [RFC PATCH 4/5] strvec API users: fix leaks by using "STRVEC_INIT_NODUP"

From
René Scharfe <l.s.r@web.de>
Date
Dec 17, 2022, 12:45 UTC
Message-ID
<9a1731aa-e20c-b11e-ba38-349d321a11c3@web.de>
In-Reply-To
<RFC-patch-4.5-6051c309d0d-20221215T090226Z-avarab@gmail.com>
Am 15.12.22 um 10:11 schrieb Ævar Arnfjörð Bjarmason:
Show 23 quoted lines
> For these cases where all of the strings we're pushing to the "struct
> strvec" are fixed strings we can fix widespread memory leaks by
> skipping the xstrdup() on strvec_push().
>
> More in-tree users could benefit from this to save needless
> xstrdup()-ing, but for all of these we were munging the "v" member, so
> the subsequent strvec_clear() wouldn't free the memory.
>
> Now there's no need to free the individual elements, but we'll still
> need to free the container with the strvec_clear().
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  builtin/am.c                  | 2 +-
>  builtin/annotate.c            | 2 +-
>  builtin/stash.c               | 2 +-
>  t/t0023-crlf-am.sh            | 1 +
>  t/t4152-am-subjects.sh        | 2 ++
>  t/t4254-am-corrupt.sh         | 2 ++
>  t/t4256-am-format-flowed.sh   | 1 +
>  t/t4257-am-interactive.sh     | 2 ++
>  t/t5403-post-checkout-hook.sh | 1 +
>  9 files changed, 12 insertions(+), 3 deletions(-)

Looks nice and easy, though the test updates are a bit distracting. They are all enabled by the change to builtin/am.c, right?

Show 115 quoted lines
>
> diff --git a/builtin/am.c b/builtin/am.c
> index 30c9b3a9cd7..691ec1d152d 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -1471,7 +1471,7 @@ static int parse_mail_rebase(struct am_state *state, const char *mail)
>  static int run_apply(const struct am_state *state, const char *index_file)
>  {
>  	struct strvec apply_paths = STRVEC_INIT;
> -	struct strvec apply_opts = STRVEC_INIT;
> +	struct strvec apply_opts = STRVEC_INIT_NODUP;
>  	struct apply_state apply_state;
>  	int res, opts_left;
>  	int force_apply = 0;
> diff --git a/builtin/annotate.c b/builtin/annotate.c
> index de58deadfc7..99d97c1a8c0 100644
> --- a/builtin/annotate.c
> +++ b/builtin/annotate.c
> @@ -9,7 +9,7 @@
>
>  int cmd_annotate(int argc UNUSED, const char **argv, const char *prefix)
>  {
> -	struct strvec args = STRVEC_INIT;
> +	struct strvec args = STRVEC_INIT_NODUP;
>  	int ret;
>
>  	strvec_pushl(&args, argv[0], "-c", NULL);
> diff --git a/builtin/stash.c b/builtin/stash.c
> index e504e22e0b9..b15dd2ebb3c 100644
> --- a/builtin/stash.c
> +++ b/builtin/stash.c
> @@ -1823,7 +1823,7 @@ int cmd_stash(int argc, const char **argv, const char *prefix)
>  {
>  	pid_t pid = getpid();
>  	const char *index_file;
> -	struct strvec args = STRVEC_INIT;
> +	struct strvec args = STRVEC_INIT_NODUP;
>  	parse_opt_subcommand_fn *fn = NULL;
>  	struct option options[] = {
>  		OPT_SUBCOMMAND("apply", &fn, apply_stash),
> diff --git a/t/t0023-crlf-am.sh b/t/t0023-crlf-am.sh
> index f9bbb91f64e..575805513a3 100755
> --- a/t/t0023-crlf-am.sh
> +++ b/t/t0023-crlf-am.sh
> @@ -2,6 +2,7 @@
>
>  test_description='Test am with auto.crlf'
>
> +TEST_PASSES_SANITIZE_LEAK=true
>  . ./test-lib.sh
>
>  cat >patchfile <<\EOF
> diff --git a/t/t4152-am-subjects.sh b/t/t4152-am-subjects.sh
> index 4c68245acad..9f2edba1f83 100755
> --- a/t/t4152-am-subjects.sh
> +++ b/t/t4152-am-subjects.sh
> @@ -1,6 +1,8 @@
>  #!/bin/sh
>
>  test_description='test subject preservation with format-patch | am'
> +
> +TEST_PASSES_SANITIZE_LEAK=true
>  . ./test-lib.sh
>
>  make_patches() {
> diff --git a/t/t4254-am-corrupt.sh b/t/t4254-am-corrupt.sh
> index 54be7da1611..45f1d4f95e5 100755
> --- a/t/t4254-am-corrupt.sh
> +++ b/t/t4254-am-corrupt.sh
> @@ -1,6 +1,8 @@
>  #!/bin/sh
>
>  test_description='git am with corrupt input'
> +
> +TEST_PASSES_SANITIZE_LEAK=true
>  . ./test-lib.sh
>
>  make_mbox_with_nul () {
> diff --git a/t/t4256-am-format-flowed.sh b/t/t4256-am-format-flowed.sh
> index 2369c4e17ad..1015273bc82 100755
> --- a/t/t4256-am-format-flowed.sh
> +++ b/t/t4256-am-format-flowed.sh
> @@ -2,6 +2,7 @@
>
>  test_description='test format=flowed support of git am'
>
> +TEST_PASSES_SANITIZE_LEAK=true
>  . ./test-lib.sh
>
>  test_expect_success 'setup' '
> diff --git a/t/t4257-am-interactive.sh b/t/t4257-am-interactive.sh
> index aed8f4de3d6..f26d7fd2dbd 100755
> --- a/t/t4257-am-interactive.sh
> +++ b/t/t4257-am-interactive.sh
> @@ -1,6 +1,8 @@
>  #!/bin/sh
>
>  test_description='am --interactive tests'
> +
> +TEST_PASSES_SANITIZE_LEAK=true
>  . ./test-lib.sh
>
>  test_expect_success 'set up patches to apply' '
> diff --git a/t/t5403-post-checkout-hook.sh b/t/t5403-post-checkout-hook.sh
> index 978f240cdac..cfaae547398 100755
> --- a/t/t5403-post-checkout-hook.sh
> +++ b/t/t5403-post-checkout-hook.sh
> @@ -7,6 +7,7 @@ test_description='Test the post-checkout hook.'
>  GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
>  export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>
> +TEST_PASSES_SANITIZE_LEAK=true
>  . ./test-lib.sh
>
>  test_expect_success setup '
Previous: Ævar Arnfjörð BjarmasonNext: René Scharfe
Message 14 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.