git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: git@vger.kernel.org
Cc: "Junio C Hamano" <gitster@pobox.com>,
	"Elijah Newren" <newren@gmail.com>,
	"Martin Ågren" <martin.agren@gmail.com>,
	"Andrzej Hunt" <ajrhunt@google.com>, "Jeff King" <peff@peff.net>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Subject: [PATCH 5/7] ls-files: fix a trivial dir_clear() leak
Date: Wed,  6 Oct 2021 12:02:23 +0200	[thread overview]
Message-ID: <patch-5.7-58b5bc67435-20211006T095426Z-avarab@gmail.com> (raw)
In-Reply-To: <cover-0.7-00000000000-20211006T095426Z-avarab@gmail.com>

Fix an edge case that was missed when the dir_clear() call was added
in eceba532141 (dir: fix problematic API to avoid memory leaks,
2020-08-18), we need to also clean up when we're about to exit with
non-zero.

That commit says, on the topic of the dir_clear() API and UNLEAK():

    [...]two of them clearly thought about leaks since they had an
    UNLEAK(dir) directive, which to me suggests that the method to
    free the data was too unclear.

I think that 0e5bba53af7 (add UNLEAK annotation for reducing leak
false positives, 2017-09-08) which added the UNLEAK() makes it clear
that that wasn't the case, rather it was the desire to avoid the
complexity of freeing the memory at the end of the program.

This does add a bit of complexity, but I think it's worth it to just
fix these leaks when it's easy in built-ins. It allows them to serve
as canaries for underlying APIs that shouldn't be leaking, it
encourages us to make those freeing APIs nicer for all their users,
and it prevents other leaking regressions by being able to mark the
entire test as TEST_PASSES_SANITIZE_LEAK=true.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/ls-files.c                | 13 +++++--------
 t/t3005-ls-files-relative.sh      |  1 +
 t/t3020-ls-files-error-unmatch.sh |  2 ++
 t/t3700-add.sh                    |  1 +
 t/t7104-reset-hard.sh             |  1 +
 5 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index e6d415e077a..0fecfa3b0c1 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -674,6 +674,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 			 N_("suppress duplicate entries")),
 		OPT_END()
 	};
+	int ret = 0;
 
 	if (argc == 2 && !strcmp(argv[1], "-h"))
 		usage_with_options(ls_files_usage, builtin_ls_files_options);
@@ -777,16 +778,12 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 	if (show_resolve_undo)
 		show_ru_info(the_repository->index);
 
-	if (ps_matched) {
-		int bad;
-		bad = report_path_error(ps_matched, &pathspec);
-		if (bad)
-			fprintf(stderr, "Did you forget to 'git add'?\n");
-
-		return bad ? 1 : 0;
+	if (ps_matched && report_path_error(ps_matched, &pathspec)) {
+		fprintf(stderr, "Did you forget to 'git add'?\n");
+		ret = 1;
 	}
 
 	dir_clear(&dir);
 	free(max_prefix);
-	return 0;
+	return ret;
 }
diff --git a/t/t3005-ls-files-relative.sh b/t/t3005-ls-files-relative.sh
index 727e9ae1a44..6ba8b589cd0 100755
--- a/t/t3005-ls-files-relative.sh
+++ b/t/t3005-ls-files-relative.sh
@@ -5,6 +5,7 @@ test_description='ls-files tests with relative paths
 This test runs git ls-files with various relative path arguments.
 '
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'prepare' '
diff --git a/t/t3020-ls-files-error-unmatch.sh b/t/t3020-ls-files-error-unmatch.sh
index 124e73b8e60..2cbcbc0721b 100755
--- a/t/t3020-ls-files-error-unmatch.sh
+++ b/t/t3020-ls-files-error-unmatch.sh
@@ -9,6 +9,8 @@ This test runs git ls-files --error-unmatch to ensure it correctly
 returns an error when a non-existent path is provided on the command
 line.
 '
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index 4086e1ebbc9..283a66955d6 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -5,6 +5,7 @@
 
 test_description='Test of git add, including the -- option.'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # Test the file mode "$1" of the file "$2" in the index.
diff --git a/t/t7104-reset-hard.sh b/t/t7104-reset-hard.sh
index 7948ec392b3..cf9697eba9a 100755
--- a/t/t7104-reset-hard.sh
+++ b/t/t7104-reset-hard.sh
@@ -2,6 +2,7 @@
 
 test_description='reset --hard unmerged'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success setup '
-- 
2.33.0.1441.gbbcdb4c3c66


  parent reply	other threads:[~2021-10-06 10:02 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-06 10:02 [PATCH 0/7] leak tests: fix "test-tool" & other small leaks Ævar Arnfjörð Bjarmason
2021-10-06 10:02 ` [PATCH 1/7] tests: fix a memory leak in test-prio-queue.c Ævar Arnfjörð Bjarmason
2021-10-06 10:02 ` [PATCH 2/7] tests: fix a memory leak in test-parse-options.c Ævar Arnfjörð Bjarmason
2021-10-06 16:37   ` Elijah Newren
2021-10-06 10:02 ` [PATCH 3/7] tests: fix a memory leak in test-oidtree.c Ævar Arnfjörð Bjarmason
2021-10-06 10:02 ` [PATCH 4/7] tests: fix test-oid-array leak, test in SANITIZE=leak Ævar Arnfjörð Bjarmason
2021-10-06 10:02 ` Ævar Arnfjörð Bjarmason [this message]
2021-10-06 10:02 ` [PATCH 6/7] ls-files: add missing string_list_clear() Ævar Arnfjörð Bjarmason
2021-10-06 10:02 ` [PATCH 7/7] merge: add missing strbuf_release() Ævar Arnfjörð Bjarmason
2021-10-06 16:47 ` [PATCH 0/7] leak tests: fix "test-tool" & other small leaks Elijah Newren
2021-10-07 10:01 ` [PATCH v2 " Ævar Arnfjörð Bjarmason
2021-10-07 10:01   ` [PATCH v2 1/7] tests: fix a memory leak in test-prio-queue.c Ævar Arnfjörð Bjarmason
2021-10-07 10:01   ` [PATCH v2 2/7] tests: fix a memory leak in test-parse-options.c Ævar Arnfjörð Bjarmason
2021-10-07 10:01   ` [PATCH v2 3/7] tests: fix a memory leak in test-oidtree.c Ævar Arnfjörð Bjarmason
2021-10-07 10:01   ` [PATCH v2 4/7] tests: fix test-oid-array leak, test in SANITIZE=leak Ævar Arnfjörð Bjarmason
2021-10-07 10:01   ` [PATCH v2 5/7] ls-files: fix a trivial dir_clear() leak Ævar Arnfjörð Bjarmason
2021-10-07 22:46     ` Junio C Hamano
2021-10-13 13:39       ` Ævar Arnfjörð Bjarmason
2021-10-13 19:01         ` Junio C Hamano
2021-10-14  0:15           ` Ævar Arnfjörð Bjarmason
2021-10-14 17:38             ` Junio C Hamano
2021-10-07 10:01   ` [PATCH v2 6/7] ls-files: add missing string_list_clear() Ævar Arnfjörð Bjarmason
2021-10-07 10:01   ` [PATCH v2 7/7] merge: add missing strbuf_release() Ævar Arnfjörð Bjarmason

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=patch-5.7-58b5bc67435-20211006T095426Z-avarab@gmail.com \
    --to=avarab@gmail.com \
    --cc=ajrhunt@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=martin.agren@gmail.com \
    --cc=newren@gmail.com \
    --cc=peff@peff.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).