commit: fix "author_ident" leak

Commit Message

Junio C Hamano May 12, 2022, 10:51 p.m. UTC

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 <[e-mail]>
Signed-off-by: Junio C Hamano <[e-mail]>
---
 builtin/commit.c                 | 9 ++++++---
 t/t2203-add-intent.sh            | 1 +
 t/t7011-skip-worktree-reading.sh | 1 +
 3 files changed, 8 insertions(+), 3 deletions(-)

Comments

Ævar Arnfjörð Bjarmason May 17, 2022, 1:48 p.m. UTC | #1

On Thu, May 12 2022, Junio C Hamano wrote:

> 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.

We've talked about UNLEAK() v.s. strbuf_release() elsewhere, so let's
leave that aside. I know your preferences in that area.

But even accounting for that, I don't see what the "more maintainable"
here refers to. The approach I suggested would s/UNLEAK/strbuf_release/
in the 4th hunk, but otherwise be equivalent.

Surely both are about as easy to maintain. To the extent that there's
any difference at all I'd think the strbuf_release() would pull ahead,
as it's guaranteed to do the right thing with all of our memory analysis
tooling (some of which will have a noop UNLEAK()).

Just a small question, I see this is in "next" already, and I'm fine
with this change either way.

Junio C Hamano May 18, 2022, 4:30 p.m. UTC | #2

Ævar Arnfjörð Bjarmason <[e-mail]> writes:

> But even accounting for that, I don't see what the "more maintainable"
> here refers to. The approach I suggested would s/UNLEAK/strbuf_release/
> in the 4th hunk, but otherwise be equivalent.

Judicious use of UNLEAK() has documentation value to tell readers
which use of pointer variables need to be explicitly released, and
which pointer variables can just implicitly released by going out of
scope at the end.  There are also a few other small added benefits
(they do not have to be changed when the helper needed to do the
real release changes, and not doing an explicit real release on
resources when there no need to is conceptually cleaner).  But they
are icing on the cake.

Sorry for a late reply---I go offline every other Tuesday and
yesterday was such a Tuesday.

Patch

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