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 '