[PATCH] commit: fix "author_ident" leak
- From
Junio C Hamano <gitster@pobox.com>
- Date
- May 12, 2022, 22:51 UTC
- Message-ID
- <xmqqzgjmcqlg.fsf@gitster.g>
- In-Reply-To
- <cover-0.2-00000000000-20220216T081844Z-avarab@gmail.com>
Since 4c28e4ada03 (commit: die before asking to edit the log message, 2010-12-20), we have been "leaking" the "author_ident" when prepare_to_commit() fails. Instead of returning from right there, introduce an exit status variable and jump to the clean-up label at the end.
Instead of explicitly releasing the resource with strbuf_release(), mark the variable with UNLEAK() at the end, together with two other variables that are already marked as such. If this were in a utility function that is called number of times, but these are different, we should explicitly release resources that grow proportionally to the size of the problem being solved, but cmd_commit() is like main() and there is no point in spending extra cycles to release individual pieces of resource at the end, just before process exit will clean everything for us for free anyway.
This fixes a leak demonstrated by e.g. "t3505-cherry-pick-empty.sh", but unfortunately we cannot mark it or other affected tests as passing now with "TEST_PASSES_SANITIZE_LEAK=true" as we'll need to fix many other memory leaks before doing so.
Incidentally there are two tests that always passes the leak checker with or without this change. Mark them as such.
This is based on an earlier patch by Ævar, but takes a different approach that is more maintainable.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> --- builtin/commit.c | 9 ++++++--- t/t2203-add-intent.sh | 1 + t/t7011-skip-worktree-reading.sh | 1 + 3 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c index b9ed0374e3..4e8b3d3251 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -1688,6 +1688,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix) struct commit *current_head = NULL; struct commit_extra_header *extra = NULL; struct strbuf err = STRBUF_INIT; + int ret = 0; if (argc == 2 && !strcmp(argv[1], "-h")) usage_with_options(builtin_commit_usage, builtin_commit_options); @@ -1722,8 +1723,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix) running hooks, writing the trees, and interacting with the user. */ if (!prepare_to_commit(index_file, prefix, current_head, &s, &author_ident)) { + ret = 1; rollback_index_files(); - return 1; + goto cleanup; } /* Determine parents */ @@ -1821,7 +1823,6 @@ int cmd_commit(int argc, const char **argv, const char *prefix) rollback_index_files(); die(_("failed to write commit object")); } - strbuf_release(&author_ident); free_commit_extra_headers(extra); if (update_head_with_reflog(current_head, &oid, reflog_msg, &sb, @@ -1862,7 +1863,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix) apply_autostash(git_path_merge_autostash(the_repository)); +cleanup: + UNLEAK(author_ident); UNLEAK(err); UNLEAK(sb); - return 0; + return ret; } diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh index db7ca55998..ebf58db2d1 100755 --- a/t/t2203-add-intent.sh +++ b/t/t2203-add-intent.sh @@ -2,6 +2,7 @@ test_description='Intent to add' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'intent to add' ' diff --git a/t/t7011-skip-worktree-reading.sh b/t/t7011-skip-worktree-reading.sh index 1761a2b1b9..4adac5acd5 100755 --- a/t/t7011-skip-worktree-reading.sh +++ b/t/t7011-skip-worktree-reading.sh @@ -5,6 +5,7 @@ test_description='skip-worktree bit test' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh cat >expect.full <<EOF
-- 2.36.1-338-g1c7f76a54c