diff options
| author | Junio C Hamano <gitster@pobox.com> | 2022-05-12 22:51:07 (GMT) |
|---|---|---|
| committer | Junio C Hamano <gitster@pobox.com> | 2022-05-12 22:51:32 (GMT) |
| commit | 00d8c311050824f841617e954c641ec2ebb300ec (patch) | |
| tree | 53d5869ad8619e62e36cebda58ee4bd9f00adfa5 | |
| parent | 4c53a8c20f8984adb226293a3ffd7b88c3f4ac1a (diff) | |
| download | git-00d8c311050824f841617e954c641ec2ebb300ec.zip git-00d8c311050824f841617e954c641ec2ebb300ec.tar.gz git-00d8c311050824f841617e954c641ec2ebb300ec.tar.bz2 | |
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>
| -rw-r--r-- | builtin/commit.c | 9 | ||||
| -rwxr-xr-x | t/t2203-add-intent.sh | 1 | ||||
| -rwxr-xr-x | 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 b9ed037..4e8b3d3 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 db7ca55..ebf58db 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 1761a2b..4adac5a 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 |