Re: [PATCH v5 07/19] repack: fix leaks on error with "goto cleanup"
- From
Elijah Newren <newren@gmail.com>
- Date
- Jan 26, 2023, 01:36 UTC
- Message-ID
- <CABPp-BHg00gpwo6emQXPK63Ub=7xRR=3w2jw6uWdpnpJWPZruQ@mail.gmail.com>
- In-Reply-To
- <patch-v5-07.19-1fac90c306a-20230118T120334Z-avarab@gmail.com>
On Wed, Jan 18, 2023 at 5:10 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> > Change cmd_repack() to "goto cleanup" rather than "return ret" on > error, when we returned we'd potentially skip cleaning up the > string_lists and other data we'd allocated in this function.
This is hard to parse; the comma followed by "when" suggests you are only changing things under a certain set of conditions rather than explaining why you are making an unconditional change. Perhaps:
In cmd_repack() when we hit an error, replace "return ret" with "goto cleanup" to ensure we free the necessary data structures.
Show 82 quoted lines
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> builtin/repack.c | 13 +++++++------
> t/t6011-rev-list-with-bad-commit.sh | 1 +
> 2 files changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/builtin/repack.c b/builtin/repack.c
> index c1402ad038f..f6493795318 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -948,7 +948,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
>
> ret = start_command(&cmd);
> if (ret)
> - return ret;
> + goto cleanup;
>
> if (geometry) {
> FILE *in = xfdopen(cmd.in, "w");
> @@ -977,7 +977,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
> fclose(out);
> ret = finish_command(&cmd);
> if (ret)
> - return ret;
> + goto cleanup;
>
> if (!names.nr && !po_args.quiet)
> printf_ln(_("Nothing new to pack."));
> @@ -1007,7 +1007,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
> &existing_nonkept_packs,
> &existing_kept_packs);
> if (ret)
> - return ret;
> + goto cleanup;
>
> if (delete_redundant && expire_to) {
> /*
> @@ -1039,7 +1039,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
> &existing_nonkept_packs,
> &existing_kept_packs);
> if (ret)
> - return ret;
> + goto cleanup;
> }
> }
>
> @@ -1115,7 +1115,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
> string_list_clear(&include, 0);
>
> if (ret)
> - return ret;
> + goto cleanup;
> }
>
> reprepare_packed_git(the_repository);
> @@ -1172,10 +1172,11 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
> write_midx_file(get_object_directory(), NULL, NULL, flags);
> }
>
> +cleanup:
> string_list_clear(&names, 1);
> string_list_clear(&existing_nonkept_packs, 0);
> string_list_clear(&existing_kept_packs, 0);
> clear_pack_geometry(geometry);
>
> - return 0;
> + return ret;
> }
> diff --git a/t/t6011-rev-list-with-bad-commit.sh b/t/t6011-rev-list-with-bad-commit.sh
> index bad02cf5b83..b2e422cf0f7 100755
> --- a/t/t6011-rev-list-with-bad-commit.sh
> +++ b/t/t6011-rev-list-with-bad-commit.sh
> @@ -2,6 +2,7 @@
>
> test_description='git rev-list should notice bad commits'
>
> +TEST_PASSES_SANITIZE_LEAK=true
> . ./test-lib.sh
>
> # Note:
> --
> 2.39.0.1225.g30a3d88132dCode changes look fine.