From 6a75658c0ae7c822c50ab09d56388e48f22e6c31 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 7 Oct 2021 12:01:31 +0200 Subject: [PATCH 1/7] tests: fix a memory leak in test-prio-queue.c MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix a memory leak in t/helper/test-prio-queue.c, the lack of freeing the memory with clear_prio_queue() has been there ever since this code was originally added in b4b594a3154 (prio-queue: priority queue of pointers to structs, 2013-06-06). By fixing this leak we can cleanly run t0009-prio-queue.sh under SANITIZE=leak, so annotate it as such with TEST_PASSES_SANITIZE_LEAK=true. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- t/helper/test-prio-queue.c | 2 ++ t/t0009-prio-queue.sh | 2 ++ 2 files changed, 4 insertions(+) diff --git a/t/helper/test-prio-queue.c b/t/helper/test-prio-queue.c index f4028442e3..133b5e6f4a 100644 --- a/t/helper/test-prio-queue.c +++ b/t/helper/test-prio-queue.c @@ -46,5 +46,7 @@ int cmd__prio_queue(int argc, const char **argv) } } + clear_prio_queue(&pq); + return 0; } diff --git a/t/t0009-prio-queue.sh b/t/t0009-prio-queue.sh index 3941ad2528..eea99107a4 100755 --- a/t/t0009-prio-queue.sh +++ b/t/t0009-prio-queue.sh @@ -1,6 +1,8 @@ #!/bin/sh test_description='basic tests for priority queue implementation' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh cat >expect <<'EOF' From c0b80e05f79a3a81af202203237687b1f4b25605 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 7 Oct 2021 12:01:32 +0200 Subject: [PATCH 2/7] tests: fix a memory leak in test-parse-options.c MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix a memory leak in t/helper/test-parse-options.c, we were not freeing the allocated "struct string_list" or its items. Let's move the declaration of the "list" variable into the cmd__parse_options() and release it at the end. In c8ba1639165 (parse-options: add OPT_STRING_LIST helper, 2011-06-09) the "list" variable was added, and later on in c8ba1639165 (parse-options: add OPT_STRING_LIST helper, 2011-06-09) the "expect" was added. The "list" variable was last touched in 2721ce21e43 (use string_list initializer consistently, 2016-06-13), but it was still left at the static scope, it's better to move it to the function for consistency. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- t/helper/test-parse-options.c | 7 ++++++- t/t0040-parse-options.sh | 1 + 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c index a282b6ff13..48d3cf6692 100644 --- a/t/helper/test-parse-options.c +++ b/t/helper/test-parse-options.c @@ -14,7 +14,6 @@ static int dry_run = 0, quiet = 0; static char *string = NULL; static char *file = NULL; static int ambiguous; -static struct string_list list = STRING_LIST_INIT_NODUP; static struct { int called; @@ -107,6 +106,8 @@ int cmd__parse_options(int argc, const char **argv) NULL }; struct string_list expect = STRING_LIST_INIT_NODUP; + struct string_list list = STRING_LIST_INIT_NODUP; + struct option options[] = { OPT_BOOL(0, "yes", &boolean, "get a boolean"), OPT_BOOL('D', "no-doubt", &boolean, "begins with 'no-'"), @@ -185,5 +186,9 @@ int cmd__parse_options(int argc, const char **argv) for (i = 0; i < argc; i++) show(&expect, &ret, "arg %02d: %s", i, argv[i]); + expect.strdup_strings = 1; + string_list_clear(&expect, 0); + string_list_clear(&list, 0); + return ret; } diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh index da310ed29b..ed422a1a61 100755 --- a/t/t0040-parse-options.sh +++ b/t/t0040-parse-options.sh @@ -5,6 +5,7 @@ test_description='our own option parser' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh cat >expect <<\EOF From 926d2330357a4ee7247b83ec4fd1ed6dba72c54e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 7 Oct 2021 12:01:33 +0200 Subject: [PATCH 3/7] tests: fix a memory leak in test-oidtree.c MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix a memory leak in t/helper/test-oidtree.c, we were not freeing the "struct strbuf" we used for the stdin input we parsed. This leak has been here ever since 92d8ed8ac10 (oidtree: a crit-bit tree for odb_loose_cache, 2021-07-07). Now that it's fixed we can declare that t0069-oidtree.sh will pass under GIT_TEST_PASSING_SANITIZE_LEAK=true. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- t/helper/test-oidtree.c | 3 +++ t/t0069-oidtree.sh | 1 + 2 files changed, 4 insertions(+) diff --git a/t/helper/test-oidtree.c b/t/helper/test-oidtree.c index 180ee28dd9..d48a409f4e 100644 --- a/t/helper/test-oidtree.c +++ b/t/helper/test-oidtree.c @@ -45,5 +45,8 @@ int cmd__oidtree(int argc, const char **argv) die("unknown command: %s", line.buf); } } + + strbuf_release(&line); + return 0; } diff --git a/t/t0069-oidtree.sh b/t/t0069-oidtree.sh index bfb1397d7b..74cc59bf8a 100755 --- a/t/t0069-oidtree.sh +++ b/t/t0069-oidtree.sh @@ -1,6 +1,7 @@ #!/bin/sh test_description='basic tests for the oidtree implementation' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh maxhexsz=$(test_oid hexsz) From 6ad66ab45e5e23378a6abf2c1d680b4cf8b85877 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 7 Oct 2021 12:01:34 +0200 Subject: [PATCH 4/7] tests: fix test-oid-array leak, test in SANITIZE=leak MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix a trivial memory leak present ever since 38d905bf585 (sha1-array: add test-sha1-array and basic tests, 2014-10-01), now that that's fixed we can test this under GIT_TEST_PASSING_SANITIZE_LEAK=true. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- t/helper/test-oid-array.c | 4 ++++ t/t0064-oid-array.sh | 2 ++ 2 files changed, 6 insertions(+) diff --git a/t/helper/test-oid-array.c b/t/helper/test-oid-array.c index b16cd0b11b..d1324d086a 100644 --- a/t/helper/test-oid-array.c +++ b/t/helper/test-oid-array.c @@ -35,5 +35,9 @@ int cmd__oid_array(int argc, const char **argv) else die("unknown command: %s", line.buf); } + + strbuf_release(&line); + oid_array_clear(&array); + return 0; } diff --git a/t/t0064-oid-array.sh b/t/t0064-oid-array.sh index 2e5438ccda..88c89e8f48 100755 --- a/t/t0064-oid-array.sh +++ b/t/t0064-oid-array.sh @@ -1,6 +1,8 @@ #!/bin/sh test_description='basic tests for the oid array implementation' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh echoid () { From eab4ac6a233e505e78fc8b04df03d58628d5ff3e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 7 Oct 2021 12:01:35 +0200 Subject: [PATCH 5/7] ls-files: fix a trivial dir_clear() leak MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Junio C Hamano --- 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 a2000ed6bf..fcc685947f 100644 --- a/builtin/ls-files.c +++ b/builtin/ls-files.c @@ -672,6 +672,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); @@ -775,16 +776,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 727e9ae1a4..6ba8b589cd 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 124e73b8e6..2cbcbc0721 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 4086e1ebbc..283a66955d 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 7948ec392b..cf9697eba9 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 ' From 272f0a574d97ecd245fb0a9ab63c436e8cfe6a06 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 7 Oct 2021 12:01:36 +0200 Subject: [PATCH 6/7] ls-files: add missing string_list_clear() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix a memory leak that's been here ever since 72aeb18772d (clean.c, ls-files.c: respect encapsulation of exclude_list_groups, 2013-01-16), we dup'd the argument in option_parse_exclude(), but never freed the string_list. This makes almost all of t3001-ls-files-others-exclude.sh pass (it had a lot of failures before). Let's mark it as passing with TEST_PASSES_SANITIZE_LEAK=true, and then exclude the tests that still failed with a !SANITIZE_LEAK prerequisite check until we fix those leaks. We can still see the failed tests under GIT_TEST_FAIL_PREREQS=true. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- builtin/ls-files.c | 1 + t/t3001-ls-files-others-exclude.sh | 5 +++-- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/builtin/ls-files.c b/builtin/ls-files.c index fcc685947f..031fef1bca 100644 --- a/builtin/ls-files.c +++ b/builtin/ls-files.c @@ -781,6 +781,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix) ret = 1; } + string_list_clear(&exclude_list, 0); dir_clear(&dir); free(max_prefix); return ret; diff --git a/t/t3001-ls-files-others-exclude.sh b/t/t3001-ls-files-others-exclude.sh index 516c95ea0e..48cec4e5f8 100755 --- a/t/t3001-ls-files-others-exclude.sh +++ b/t/t3001-ls-files-others-exclude.sh @@ -8,6 +8,7 @@ test_description='git ls-files --others --exclude This test runs git ls-files --others and tests --exclude patterns. ' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh rm -fr one three @@ -102,7 +103,7 @@ test_expect_success \ >output && test_cmp expect output' -test_expect_success 'restore gitignore' ' +test_expect_success !SANITIZE_LEAK 'restore gitignore' ' git checkout --ignore-skip-worktree-bits $allignores && rm .git/index ' @@ -125,7 +126,7 @@ cat > expect << EOF # three/ EOF -test_expect_success 'git status honors core.excludesfile' \ +test_expect_success !SANITIZE_LEAK 'git status honors core.excludesfile' \ 'test_cmp expect output' test_expect_success 'trailing slash in exclude allows directory match(1)' ' From 465028e0e25518bfff8b83057775cb6b2df2aade Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 7 Oct 2021 12:01:37 +0200 Subject: [PATCH 7/7] merge: add missing strbuf_release() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We strbuf_reset() this "struct strbuf" in a loop earlier, but never freed it. Plugs a memory leak that's been here ever since this code got introduced in 1c7b76be7d6 (Build in merge, 2008-07-07). This takes us from 68 failed tests in "t7600-merge.sh" to 59 under SANITIZE=leak, and makes "t7604-merge-custom-message.sh" pass! Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- builtin/merge.c | 2 ++ t/t7604-merge-custom-message.sh | 1 + 2 files changed, 3 insertions(+) diff --git a/builtin/merge.c b/builtin/merge.c index 3fbdacc7db..e52f308a1a 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -1576,6 +1576,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix) finish(head_commit, remoteheads, &commit->object.oid, msg.buf); remove_merge_branch_state(the_repository); + strbuf_release(&msg); goto done; } else if (!remoteheads->next && common->next) ; @@ -1746,6 +1747,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix) ret = suggest_conflicts(); done: + strbuf_release(&buf); free(branch_to_free); return ret; } diff --git a/t/t7604-merge-custom-message.sh b/t/t7604-merge-custom-message.sh index cd4f9607dc..eca7555101 100755 --- a/t/t7604-merge-custom-message.sh +++ b/t/t7604-merge-custom-message.sh @@ -4,6 +4,7 @@ test_description='git merge Testing merge when using a custom message for the merge commit.' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh create_merge_msgs() {