@matheusbsbs / matheus-bagliano / commits / 00d8c31105

commit: fix "author_ident" leak

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>

Junio C Hamano committed May 12, 2022 at 15:51 UTC 00d8c311050824f841617e954c641ec2ebb300ec

3 files changed +8 -3

builtin/commit.c

+6 -3

@@ -1688,6 +1688,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)

1688 struct commit *current_head = NULL;

1689 struct commit_extra_header *extra = NULL;

1690 struct strbuf err = STRBUF_INIT;

1691 + int ret = 0;

1692

1693 if (argc == 2 && !strcmp(argv[1], "-h"))

1694 usage_with_options(builtin_commit_usage, builtin_commit_options);

@@ -1722,8 +1723,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix)

1723 running hooks, writing the trees, and interacting with the user. */

1724 if (!prepare_to_commit(index_file, prefix,

1725 current_head, &s, &author_ident)) {

1726 + ret = 1;

1727 rollback_index_files();

1726 - return 1;

1728 + goto cleanup;

1729 }

1730

1731 /* Determine parents */

@@ -1821,7 +1823,6 @@ int cmd_commit(int argc, const char **argv, const char *prefix)

1823 rollback_index_files();

1824 die(_("failed to write commit object"));

1825 }

1824 - strbuf_release(&author_ident);

1826 free_commit_extra_headers(extra);

1827

1828 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)

1863

1864 apply_autostash(git_path_merge_autostash(the_repository));

1865

1866 +cleanup:

1867 + UNLEAK(author_ident);

1868 UNLEAK(err);

1869 UNLEAK(sb);

1867 - return 0;

1870 + return ret;

1871 }

t/t2203-add-intent.sh

@@ -2,6 +2,7 @@

3 test_description='Intent to add'

5 +TEST_PASSES_SANITIZE_LEAK=true

6 . ./test-lib.sh

8 test_expect_success 'intent to add' '

t/t7011-skip-worktree-reading.sh

@@ -5,6 +5,7 @@

6 test_description='skip-worktree bit test'

8 +TEST_PASSES_SANITIZE_LEAK=true

9 . ./test-lib.sh

11 cat >expect.full <<EOF