From e5e37517dd961b70250107f565539950fea94e76 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Tue, 8 Nov 2022 19:17:37 +0100 Subject: [PATCH 01/15] tests: mark tests as passing with SANITIZE=leak MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This marks tests that have been leak-free since various recent commits, but which were not marked us such when the memory leak was fixed. These were mostly discovered with the "check" mode added in faececa53f9 (test-lib: have the "check" mode for SANITIZE=leak consider leak logs, 2022-07-28). Commits that fixed the last memory leak in these tests. Per narrowing down when they started to pass under SANITIZE=leak with "bisect": - t1022-read-tree-partial-clone.sh: 7e2619d8ff0 (list_objects_filter_options: plug leak of filter_spec strings, 2022-09-08) - t4053-diff-no-index.sh: 07a6f94a6d0 (diff-no-index: release prefixed filenames, 2022-09-07) - t6415-merge-dir-to-symlink.sh: bac92b1f39f (Merge branch 'js/ort-clean-up-after-failed-merge', 2022-08-08). - t5554-noop-fetch-negotiator.sh: 66eede4a37c (prepare_repo_settings(): plug leak of config values, 2022-09-08) - t2012-checkout-last.sh, t7504-commit-msg-hook.sh, t91{15,46,60}-git-svn-*.sh: The in-flight "pw/rebase-no-reflog-action" series, upon which this is based: https://lore.kernel.org/git/pull.1405.git.1667575142.gitgitgadget@gmail.com/ Let's mark all of these as passing with "TEST_PASSES_SANITIZE_LEAK=true", to have it regression tested, including as part of the "linux-leaks" CI job. Additionally, let's remove the "!SANITIZE_LEAK" prerequisite from tests that now pass, these were marked as failing in: - 77e56d55ba6 (diff.c: fix a double-free regression in a18d66cefb, 2022-03-17) - c4d1d526312 (tests: change some 'test $(git) = "x"' to test_cmp, 2022-03-07) These were not spotted with the new "check" mode, but manually, it doesn't cover these sort of prerequisites. There's few enough that we shouldn't bother to automate it. They'll be going away sooner than later. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Taylor Blau --- t/t1022-read-tree-partial-clone.sh | 2 +- t/t2012-checkout-last.sh | 1 + t/t4015-diff-whitespace.sh | 4 ++-- t/t4053-diff-no-index.sh | 1 + t/t5554-noop-fetch-negotiator.sh | 2 ++ t/t6415-merge-dir-to-symlink.sh | 1 + t/t7103-reset-bare.sh | 2 +- t/t7504-commit-msg-hook.sh | 1 + t/t9115-git-svn-dcommit-funky-renames.sh | 1 - t/t9146-git-svn-empty-dirs.sh | 1 - t/t9160-git-svn-preserve-empty-dirs.sh | 1 - 11 files changed, 10 insertions(+), 7 deletions(-) diff --git a/t/t1022-read-tree-partial-clone.sh b/t/t1022-read-tree-partial-clone.sh index a9953b6a71..d08563de5c 100755 --- a/t/t1022-read-tree-partial-clone.sh +++ b/t/t1022-read-tree-partial-clone.sh @@ -3,7 +3,7 @@ test_description='git read-tree in partial clones' TEST_NO_CREATE_REPO=1 - +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'read-tree in partial clone prefetches in one batch' ' diff --git a/t/t2012-checkout-last.sh b/t/t2012-checkout-last.sh index 1f6c4ed042..4b6372f4c3 100755 --- a/t/t2012-checkout-last.sh +++ b/t/t2012-checkout-last.sh @@ -5,6 +5,7 @@ test_description='checkout can switch to last branch and merge base' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup' ' diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh index f3e20dd5bb..b298f220e0 100755 --- a/t/t4015-diff-whitespace.sh +++ b/t/t4015-diff-whitespace.sh @@ -1638,7 +1638,7 @@ test_expect_success 'no effect on diff from --color-moved with --word-diff' ' test_cmp expect actual ' -test_expect_success !SANITIZE_LEAK 'no effect on show from --color-moved with --word-diff' ' +test_expect_success 'no effect on show from --color-moved with --word-diff' ' git show --color-moved --word-diff >actual && git show --word-diff >expect && test_cmp expect actual @@ -2024,7 +2024,7 @@ test_expect_success '--color-moved rewinds for MIN_ALNUM_COUNT' ' test_cmp expected actual ' -test_expect_success !SANITIZE_LEAK 'move detection with submodules' ' +test_expect_success 'move detection with submodules' ' test_create_repo bananas && echo ripe >bananas/recipe && git -C bananas add recipe && diff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh index 3feadf0e35..4e9fa0403d 100755 --- a/t/t4053-diff-no-index.sh +++ b/t/t4053-diff-no-index.sh @@ -2,6 +2,7 @@ test_description='diff --no-index' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup' ' diff --git a/t/t5554-noop-fetch-negotiator.sh b/t/t5554-noop-fetch-negotiator.sh index 2ac7b5859e..06991e8e8a 100755 --- a/t/t5554-noop-fetch-negotiator.sh +++ b/t/t5554-noop-fetch-negotiator.sh @@ -1,6 +1,8 @@ #!/bin/sh test_description='test noop fetch negotiator' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'noop negotiator does not emit any "have"' ' diff --git a/t/t6415-merge-dir-to-symlink.sh b/t/t6415-merge-dir-to-symlink.sh index 2655e295f5..ae00492c76 100755 --- a/t/t6415-merge-dir-to-symlink.sh +++ b/t/t6415-merge-dir-to-symlink.sh @@ -4,6 +4,7 @@ test_description='merging when a directory was replaced with a symlink' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'create a commit where dir a/b changed to symlink' ' diff --git a/t/t7103-reset-bare.sh b/t/t7103-reset-bare.sh index a60153f9f3..18bbd9975e 100755 --- a/t/t7103-reset-bare.sh +++ b/t/t7103-reset-bare.sh @@ -63,7 +63,7 @@ test_expect_success '"mixed" reset is not allowed in bare' ' test_must_fail git reset --mixed HEAD^ ' -test_expect_success !SANITIZE_LEAK '"soft" reset is allowed in bare' ' +test_expect_success '"soft" reset is allowed in bare' ' git reset --soft HEAD^ && git show --pretty=format:%s >out && echo one >expect && diff --git a/t/t7504-commit-msg-hook.sh b/t/t7504-commit-msg-hook.sh index a39de8c112..07ca46fb0d 100755 --- a/t/t7504-commit-msg-hook.sh +++ b/t/t7504-commit-msg-hook.sh @@ -5,6 +5,7 @@ test_description='commit-msg hook' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'with no hook' ' diff --git a/t/t9115-git-svn-dcommit-funky-renames.sh b/t/t9115-git-svn-dcommit-funky-renames.sh index 419f055721..743fbe1fe4 100755 --- a/t/t9115-git-svn-dcommit-funky-renames.sh +++ b/t/t9115-git-svn-dcommit-funky-renames.sh @@ -5,7 +5,6 @@ test_description='git svn dcommit can commit renames of files with ugly names' -TEST_FAILS_SANITIZE_LEAK=true . ./lib-git-svn.sh test_expect_success 'load repository with strange names' ' diff --git a/t/t9146-git-svn-empty-dirs.sh b/t/t9146-git-svn-empty-dirs.sh index 79c26ed69c..09606f1b3c 100755 --- a/t/t9146-git-svn-empty-dirs.sh +++ b/t/t9146-git-svn-empty-dirs.sh @@ -4,7 +4,6 @@ test_description='git svn creates empty directories' -TEST_FAILS_SANITIZE_LEAK=true . ./lib-git-svn.sh test_expect_success 'initialize repo' ' diff --git a/t/t9160-git-svn-preserve-empty-dirs.sh b/t/t9160-git-svn-preserve-empty-dirs.sh index 9cf7a1427a..36c6b1a12f 100755 --- a/t/t9160-git-svn-preserve-empty-dirs.sh +++ b/t/t9160-git-svn-preserve-empty-dirs.sh @@ -9,7 +9,6 @@ This test uses git to clone a Subversion repository that contains empty directories, and checks that corresponding directories are created in the local Git repository with placeholder files.' -TEST_FAILS_SANITIZE_LEAK=true . ./lib-git-svn.sh GIT_REPO=git-svn-repo From ab2cf37183eb461f1a5176a4d58650e7c8f642eb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Tue, 8 Nov 2022 19:17:38 +0100 Subject: [PATCH 02/15] {reset,merge}: call discard_index() before returning MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit These two built-ins both deal with the index, but weren't discarding it. In subsequent commits we'll add more free()-ing to discard_index() that we've missed, but let's first call the existing function. We can doubtless add discard_index() (or its alias discard_cache()) to a lot more places, but let's just add it here for now. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Taylor Blau --- builtin/merge.c | 1 + builtin/reset.c | 2 ++ 2 files changed, 3 insertions(+) diff --git a/builtin/merge.c b/builtin/merge.c index 5900b81729..a6698adbfd 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -1794,5 +1794,6 @@ done: } strbuf_release(&buf); free(branch_to_free); + discard_index(&the_index); return ret; } diff --git a/builtin/reset.c b/builtin/reset.c index fdce6f8c85..69f18a248e 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -481,5 +481,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix) if (!pathspec.nr) remove_branch_state(the_repository, 0); + discard_index(&the_index); + return update_ref_status; } From 03267e8656c23cf1e2d1df8204d4cee236fb0077 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Tue, 8 Nov 2022 19:17:39 +0100 Subject: [PATCH 03/15] commit: discard partial cache before (re-)reading it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The read_cache() in prepare_to_commit() would end up clobbering the pointer we had for a previously populated "the_index.cache_tree" in the very common case of "git commit" stressed by e.g. the tests being changed here. We'd populate "the_index.cache_tree" by calling "update_main_cache_tree" in prepare_index(), but would not end up with a "fully prepared" index. What constitutes an existing index is clearly overly fuzzy, here we'll check "active_nr" (aka "the_index.cache_nr"), but our "the_index.cache_tree" might have been malloc()'d already. Thus the code added in 11c8a74a64a (commit: write cache-tree data when writing index anyway, 2011-12-06) would end up allocating the "cache_tree", and would interact here with code added in 7168624c353 (Do not generate full commit log message if it is not going to be used, 2007-11-28). The result was a very common memory leak. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Taylor Blau --- builtin/commit.c | 7 +++++-- t/t0068-for-each-repo.sh | 1 + t/t0070-fundamental.sh | 1 + t/t1404-update-ref-errors.sh | 2 ++ t/t1409-avoid-packing-refs.sh | 1 + t/t1413-reflog-detach.sh | 1 + t/t1501-work-tree.sh | 2 ++ t/t2025-checkout-no-overlay.sh | 1 + t/t3009-ls-files-others-nonsubmodule.sh | 1 + t/t3010-ls-files-killed-modified.sh | 2 ++ t/t4045-diff-relative.sh | 2 ++ t/t4111-apply-subdir.sh | 1 + t/t4135-apply-weird-filenames.sh | 1 + t/t4213-log-tabexpand.sh | 1 + t/t5618-alternate-refs.sh | 2 ++ t/t6301-for-each-ref-errors.sh | 1 + t/t7520-ignored-hook-warning.sh | 1 + t/t7614-merge-signoff.sh | 1 + t/t9003-help-autocorrect.sh | 2 ++ 19 files changed, 29 insertions(+), 2 deletions(-) diff --git a/builtin/commit.c b/builtin/commit.c index e22bdf23f5..c291199b70 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -987,8 +987,11 @@ static int prepare_to_commit(const char *index_file, const char *prefix, struct object_id oid; const char *parent = "HEAD"; - if (!active_nr && read_cache() < 0) - die(_("Cannot read index")); + if (!active_nr) { + discard_cache(); + if (read_cache() < 0) + die(_("Cannot read index")); + } if (amend) parent = "HEAD^1"; diff --git a/t/t0068-for-each-repo.sh b/t/t0068-for-each-repo.sh index 4675e85251..9f63f61242 100755 --- a/t/t0068-for-each-repo.sh +++ b/t/t0068-for-each-repo.sh @@ -2,6 +2,7 @@ test_description='git for-each-repo builtin' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'run based on configured value' ' diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh index 8d59905ef0..574de34198 100755 --- a/t/t0070-fundamental.sh +++ b/t/t0070-fundamental.sh @@ -6,6 +6,7 @@ test_description='check that the most basic functions work Verify wrappers and compatibility functions. ' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'character classes (isspace, isalpha etc.)' ' diff --git a/t/t1404-update-ref-errors.sh b/t/t1404-update-ref-errors.sh index 13c2b43bba..b5606d93b5 100755 --- a/t/t1404-update-ref-errors.sh +++ b/t/t1404-update-ref-errors.sh @@ -1,6 +1,8 @@ #!/bin/sh test_description='Test git update-ref error handling' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh # Create some references, perhaps run pack-refs --all, then try to diff --git a/t/t1409-avoid-packing-refs.sh b/t/t1409-avoid-packing-refs.sh index be12fb6350..f23c0152a8 100755 --- a/t/t1409-avoid-packing-refs.sh +++ b/t/t1409-avoid-packing-refs.sh @@ -2,6 +2,7 @@ test_description='avoid rewriting packed-refs unnecessarily' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh # Add an identifying mark to the packed-refs file header line. This diff --git a/t/t1413-reflog-detach.sh b/t/t1413-reflog-detach.sh index 934688a1ee..d2a4822d46 100755 --- a/t/t1413-reflog-detach.sh +++ b/t/t1413-reflog-detach.sh @@ -4,6 +4,7 @@ test_description='Test reflog interaction with detached HEAD' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh reset_state () { diff --git a/t/t1501-work-tree.sh b/t/t1501-work-tree.sh index b75558040f..ae6528aece 100755 --- a/t/t1501-work-tree.sh +++ b/t/t1501-work-tree.sh @@ -1,6 +1,8 @@ #!/bin/sh test_description='test separate work tree' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup' ' diff --git a/t/t2025-checkout-no-overlay.sh b/t/t2025-checkout-no-overlay.sh index 8f13341cf8..3832c3de81 100755 --- a/t/t2025-checkout-no-overlay.sh +++ b/t/t2025-checkout-no-overlay.sh @@ -2,6 +2,7 @@ test_description='checkout --no-overlay -- ' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup' ' diff --git a/t/t3009-ls-files-others-nonsubmodule.sh b/t/t3009-ls-files-others-nonsubmodule.sh index 963f3462b7..14218b3424 100755 --- a/t/t3009-ls-files-others-nonsubmodule.sh +++ b/t/t3009-ls-files-others-nonsubmodule.sh @@ -18,6 +18,7 @@ This test runs git ls-files --others with the following working tree: git repository with a commit and an untracked file ' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup: directories' ' diff --git a/t/t3010-ls-files-killed-modified.sh b/t/t3010-ls-files-killed-modified.sh index 580e158f99..054178703d 100755 --- a/t/t3010-ls-files-killed-modified.sh +++ b/t/t3010-ls-files-killed-modified.sh @@ -41,6 +41,8 @@ Also for modification test, the cache and working tree have: We should report path0, path1, path2/file2, path3/file3, path7 and path8 modified without reporting path9 and path10. submod1 is also modified. ' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'git update-index --add to add various paths.' ' diff --git a/t/t4045-diff-relative.sh b/t/t4045-diff-relative.sh index fab351b48a..198dfc9190 100755 --- a/t/t4045-diff-relative.sh +++ b/t/t4045-diff-relative.sh @@ -1,6 +1,8 @@ #!/bin/sh test_description='diff --relative tests' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup' ' diff --git a/t/t4111-apply-subdir.sh b/t/t4111-apply-subdir.sh index 1618a6dbc7..e9a87d761d 100755 --- a/t/t4111-apply-subdir.sh +++ b/t/t4111-apply-subdir.sh @@ -2,6 +2,7 @@ test_description='patching from inconvenient places' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup' ' diff --git a/t/t4135-apply-weird-filenames.sh b/t/t4135-apply-weird-filenames.sh index 6bc3fb97a7..d3502c6fdd 100755 --- a/t/t4135-apply-weird-filenames.sh +++ b/t/t4135-apply-weird-filenames.sh @@ -2,6 +2,7 @@ test_description='git apply with weird postimage filenames' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup' ' diff --git a/t/t4213-log-tabexpand.sh b/t/t4213-log-tabexpand.sh index 53a4af3244..590fce95e9 100755 --- a/t/t4213-log-tabexpand.sh +++ b/t/t4213-log-tabexpand.sh @@ -2,6 +2,7 @@ test_description='log/show --expand-tabs' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh HT=" " diff --git a/t/t5618-alternate-refs.sh b/t/t5618-alternate-refs.sh index 3353216f09..f905db0a3f 100755 --- a/t/t5618-alternate-refs.sh +++ b/t/t5618-alternate-refs.sh @@ -1,6 +1,8 @@ #!/bin/sh test_description='test handling of --alternate-refs traversal' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh # Avoid test_commit because we want a specific and known set of refs: diff --git a/t/t6301-for-each-ref-errors.sh b/t/t6301-for-each-ref-errors.sh index 40edf9dab5..bfda1f46ad 100755 --- a/t/t6301-for-each-ref-errors.sh +++ b/t/t6301-for-each-ref-errors.sh @@ -2,6 +2,7 @@ test_description='for-each-ref errors for broken refs' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh ZEROS=$ZERO_OID diff --git a/t/t7520-ignored-hook-warning.sh b/t/t7520-ignored-hook-warning.sh index dc57526e6f..184b258989 100755 --- a/t/t7520-ignored-hook-warning.sh +++ b/t/t7520-ignored-hook-warning.sh @@ -2,6 +2,7 @@ test_description='ignored hook warning' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success setup ' diff --git a/t/t7614-merge-signoff.sh b/t/t7614-merge-signoff.sh index fee258d4f0..cf96a35e8e 100755 --- a/t/t7614-merge-signoff.sh +++ b/t/t7614-merge-signoff.sh @@ -8,6 +8,7 @@ This test runs git merge --signoff and makes sure that it works. GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh # Setup test files diff --git a/t/t9003-help-autocorrect.sh b/t/t9003-help-autocorrect.sh index f00deaf381..4b9cb4c942 100755 --- a/t/t9003-help-autocorrect.sh +++ b/t/t9003-help-autocorrect.sh @@ -1,6 +1,8 @@ #!/bin/sh test_description='help.autocorrect finding a match' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup' ' From b5fcb1c006cd566b567a5249a53d3ee5a51f299a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Tue, 8 Nov 2022 19:17:40 +0100 Subject: [PATCH 04/15] read-cache.c: clear and free "sparse_checkout_patterns" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The "sparse_checkout_patterns" member was added to the "struct index_state" in 836e25c51b2 (sparse-checkout: hold pattern list in index, 2021-03-30), but wasn't added to discard_index(). Let's do that. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Taylor Blau --- read-cache.c | 5 +++++ t/t1011-read-tree-sparse-checkout.sh | 1 + t/t2018-checkout-branch.sh | 1 + t/t6435-merge-sparse.sh | 1 + 4 files changed, 8 insertions(+) diff --git a/read-cache.c b/read-cache.c index 3202402927..7c6477487a 100644 --- a/read-cache.c +++ b/read-cache.c @@ -2558,6 +2558,11 @@ int discard_index(struct index_state *istate) free_untracked_cache(istate->untracked); istate->untracked = NULL; + if (istate->sparse_checkout_patterns) { + clear_pattern_list(istate->sparse_checkout_patterns); + FREE_AND_NULL(istate->sparse_checkout_patterns); + } + if (istate->ce_mem_pool) { mem_pool_discard(istate->ce_mem_pool, should_validate_cache_entries()); FREE_AND_NULL(istate->ce_mem_pool); diff --git a/t/t1011-read-tree-sparse-checkout.sh b/t/t1011-read-tree-sparse-checkout.sh index 742f0fa909..595b24c0ad 100755 --- a/t/t1011-read-tree-sparse-checkout.sh +++ b/t/t1011-read-tree-sparse-checkout.sh @@ -12,6 +12,7 @@ test_description='sparse checkout tests ' TEST_CREATE_REPO_NO_TEMPLATE=1 +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-read-tree.sh diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh index 771c3c3c50..8581ad3437 100755 --- a/t/t2018-checkout-branch.sh +++ b/t/t2018-checkout-branch.sh @@ -3,6 +3,7 @@ test_description='checkout' TEST_CREATE_REPO_NO_TEMPLATE=1 +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh # Arguments: [!] [] diff --git a/t/t6435-merge-sparse.sh b/t/t6435-merge-sparse.sh index fde4aa3cd1..78628fb248 100755 --- a/t/t6435-merge-sparse.sh +++ b/t/t6435-merge-sparse.sh @@ -3,6 +3,7 @@ test_description='merge with sparse files' TEST_CREATE_REPO_NO_TEMPLATE=1 +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh # test_file $filename $content From 083fd1a264a78b50bc2339d85f4d1113c01a4172 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Tue, 8 Nov 2022 19:17:41 +0100 Subject: [PATCH 05/15] dir.c: free "ident" and "exclude_per_dir" in "struct untracked_cache" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When the "ident" member of the structure was added in 1e8fef609e7 (untracked cache: guard and disable on system changes, 2015-03-08) this function wasn't updated to free it. Let's do so. Let's also free the "exclude_per_dir" memory we've been leaking since[1], while making sure not to free() the constant ".gitignore" string we add by default[2]. As we now have three struct members we're freeing let's change free_untracked_cache() to return early if "uc" isn't defined. We won't hand it to free() now, but that was just for convenience, once we're dealing with >=2 struct members this pattern is more convenient. 1. f9e6c649589 (untracked cache: load from UNTR index extension, 2015-03-08) 2. 039bc64e886 (core.excludesfile clean-up, 2007-11-14) Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Taylor Blau --- dir.c | 10 +++++++--- dir.h | 1 + 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/dir.c b/dir.c index d604d1bab9..fbdb24fc81 100644 --- a/dir.c +++ b/dir.c @@ -3581,8 +3581,12 @@ static void free_untracked(struct untracked_cache_dir *ucd) void free_untracked_cache(struct untracked_cache *uc) { - if (uc) - free_untracked(uc->root); + if (!uc) + return; + + free(uc->exclude_per_dir_to_free); + strbuf_release(&uc->ident); + free_untracked(uc->root); free(uc); } @@ -3739,7 +3743,7 @@ struct untracked_cache *read_untracked_extension(const void *data, unsigned long next + offset + hashsz); uc->dir_flags = get_be32(next + ouc_offset(dir_flags)); exclude_per_dir = (const char *)next + exclude_per_dir_offset; - uc->exclude_per_dir = xstrdup(exclude_per_dir); + uc->exclude_per_dir = uc->exclude_per_dir_to_free = xstrdup(exclude_per_dir); /* NUL after exclude_per_dir is covered by sizeof(*ouc) */ next += exclude_per_dir_offset + strlen(exclude_per_dir) + 1; if (next >= end) diff --git a/dir.h b/dir.h index 674747d93a..8acfc04418 100644 --- a/dir.h +++ b/dir.h @@ -188,6 +188,7 @@ struct untracked_cache { struct oid_stat ss_info_exclude; struct oid_stat ss_excludes_file; const char *exclude_per_dir; + char *exclude_per_dir_to_free; struct strbuf ident; /* * dir_struct#flags must match dir_flags or the untracked From b6046abc0c27134d52c56699a88d1fadfe7e66ce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Tue, 8 Nov 2022 19:17:42 +0100 Subject: [PATCH 06/15] built-ins & libs & helpers: add/move destructors, fix leaks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix various leaks in built-ins, libraries and a test helper here we were missing a call to strbuf_release(), string_list_clear() etc, or were calling them after a potential "return". Comments on individual changes: - builtin/checkout.c: Fix a memory leak that was introduced in [1]. A sibling leak introduced in [2] was recently fixed in [3]. As with [3] we should be using the wt_status_state_free_buffers() API introduced in [4]. - builtin/repack.c: Fix a leak that's been here since this use of "strbuf_release()" was added in a1bbc6c0176 (repack: rewrite the shell script in C, 2013-09-15). We don't use the variable for anything except this loop, so we can instead free it right afterwards. - builtin/rev-parse: Fix a leak that's been here since this code was added in 21d47835386 (Add a parseopt mode to git-rev-parse to bring parse-options to shell scripts., 2007-11-04). - builtin/stash.c: Fix a couple of leaks that have been here since this code was added in d4788af875c (stash: convert create to builtin, 2019-02-25), we strbuf_release()'d only some of the "struct strbuf" we allocated earlier in the function, let's release all of them. - ref-filter.c: Fix a leak in 482c1191869 (gpg-interface: improve interface for parsing tags, 2021-02-11), we don't use the "payload" variable that we ask parse_signature() to populate for us, so let's free it. - t/helper/test-fake-ssh.c: Fix a leak that's been here since this code was added in 3064d5a38c7 (mingw: fix t5601-clone.sh, 2016-01-27). Let's free the "struct strbuf" as soon as we don't need it anymore. 1. c45f0f525de (switch: reject if some operation is in progress, 2019-03-29) 2. 2708ce62d21 (branch: sort detached HEAD based on a flag, 2021-01-07) 3. abcac2e19fa (ref-filter.c: fix a leak in get_head_description, 2022-09-25) 4. 962dd7ebc3e (wt-status: introduce wt_status_state_free_buffers(), 2020-09-27). Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Taylor Blau --- builtin/checkout.c | 2 ++ builtin/rebase.c | 3 +++ builtin/repack.c | 2 +- builtin/rev-parse.c | 1 + builtin/stash.c | 2 ++ ref-filter.c | 1 + t/helper/test-fake-ssh.c | 1 + t/t3409-rebase-environ.sh | 1 + t/t3428-rebase-signoff.sh | 1 + t/t3433-rebase-across-mode-change.sh | 1 + 10 files changed, 14 insertions(+), 1 deletion(-) diff --git a/builtin/checkout.c b/builtin/checkout.c index 2a132392fb..659dd5c430 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -1470,6 +1470,8 @@ static void die_if_some_operation_in_progress(void) "or \"git worktree add\".")); if (state.bisect_in_progress) warning(_("you are switching branch while bisecting")); + + wt_status_state_free_buffers(&state); } static int checkout_branch(struct checkout_opts *opts, diff --git a/builtin/rebase.c b/builtin/rebase.c index 4d6839a578..6a83132031 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -1828,10 +1828,13 @@ cleanup: strbuf_release(&revisions); free(options.reflog_action); free(options.head_name); + strvec_clear(&options.git_am_opts); free(options.gpg_sign_opt); free(options.cmd); free(options.strategy); strbuf_release(&options.git_format_patch_opt); free(squash_onto_name); + string_list_clear(&exec, 0); + string_list_clear(&strategy_options, 0); return !!ret; } diff --git a/builtin/repack.c b/builtin/repack.c index 10e23f9ee1..fb3ac19742 100644 --- a/builtin/repack.c +++ b/builtin/repack.c @@ -956,6 +956,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix) item = string_list_append(&names, line.buf); item->util = populate_pack_exts(item->string); } + strbuf_release(&line); fclose(out); ret = finish_command(&cmd); if (ret) @@ -1124,7 +1125,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix) string_list_clear(&existing_nonkept_packs, 0); string_list_clear(&existing_kept_packs, 0); clear_pack_geometry(geometry); - strbuf_release(&line); return 0; } diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c index 8f61050bde..e0244d63de 100644 --- a/builtin/rev-parse.c +++ b/builtin/rev-parse.c @@ -530,6 +530,7 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix) strbuf_addstr(&parsed, " --"); sq_quote_argv(&parsed, argv); puts(parsed.buf); + strbuf_release(&parsed); return 0; } diff --git a/builtin/stash.c b/builtin/stash.c index bb5485b409..8a64d564a1 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -1686,8 +1686,10 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q } done: + strbuf_release(&patch); free_stash_info(&info); strbuf_release(&stash_msg_buf); + strbuf_release(&untracked_files); return ret; } diff --git a/ref-filter.c b/ref-filter.c index 914908fac5..b40b76c380 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -1358,6 +1358,7 @@ static void find_subpos(const char *buf, /* parse signature first; we might not even have a subject line */ parse_signature(buf, end - buf, &payload, &signature); + strbuf_release(&payload); /* skip past header until we hit empty line */ while (*buf && *buf != '\n') { diff --git a/t/helper/test-fake-ssh.c b/t/helper/test-fake-ssh.c index 12beee99ad..d5e511633a 100644 --- a/t/helper/test-fake-ssh.c +++ b/t/helper/test-fake-ssh.c @@ -17,6 +17,7 @@ int cmd_main(int argc, const char **argv) f = fopen(buf.buf, "w"); if (!f) die("Could not write to %s", buf.buf); + strbuf_release(&buf); for (i = 0; i < argc; i++) fprintf(f, "%s%s", i > 0 ? " " : "", i > 0 ? argv[i] : "ssh:"); fprintf(f, "\n"); diff --git a/t/t3409-rebase-environ.sh b/t/t3409-rebase-environ.sh index 83ffb39d9f..acaf5558db 100755 --- a/t/t3409-rebase-environ.sh +++ b/t/t3409-rebase-environ.sh @@ -2,6 +2,7 @@ test_description='git rebase interactive environment' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup' ' diff --git a/t/t3428-rebase-signoff.sh b/t/t3428-rebase-signoff.sh index f6993b7e14..e1b1e94764 100755 --- a/t/t3428-rebase-signoff.sh +++ b/t/t3428-rebase-signoff.sh @@ -5,6 +5,7 @@ test_description='git rebase --signoff This test runs git rebase --signoff and make sure that it works. ' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh # A simple file to commit diff --git a/t/t3433-rebase-across-mode-change.sh b/t/t3433-rebase-across-mode-change.sh index 05df964670..c8172b0852 100755 --- a/t/t3433-rebase-across-mode-change.sh +++ b/t/t3433-rebase-across-mode-change.sh @@ -2,6 +2,7 @@ test_description='git rebase across mode change' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup' ' From e84a26e32faba0c2ae7a19fa9bf805645f620e82 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Tue, 8 Nov 2022 19:17:43 +0100 Subject: [PATCH 07/15] unpack-file: fix ancient leak in create_temp_file() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix a leak that's been with us since 3407bb4940c (Add "unpack-file" helper that unpacks a sha1 blob into a tmpfile., 2005-04-18). See 00c8fd493af (cat-file: use streaming API to print blobs, 2012-03-07) for prior art which shows the same API pattern, i.e. free()-ing the result of read_object_file() after it's used. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Taylor Blau --- builtin/unpack-file.c | 1 + t/t6060-merge-index.sh | 2 ++ t/t6401-merge-criss-cross.sh | 2 ++ t/t6406-merge-attr.sh | 1 + t/t6407-merge-binary.sh | 1 + t/t7605-merge-resolve.sh | 1 + 6 files changed, 8 insertions(+) diff --git a/builtin/unpack-file.c b/builtin/unpack-file.c index 9e8119dd35..88de32b7d7 100644 --- a/builtin/unpack-file.c +++ b/builtin/unpack-file.c @@ -19,6 +19,7 @@ static char *create_temp_file(struct object_id *oid) if (write_in_full(fd, buf, size) < 0) die_errno("unable to write temp-file"); close(fd); + free(buf); return path; } diff --git a/t/t6060-merge-index.sh b/t/t6060-merge-index.sh index ed449abe55..1a8b64cce1 100755 --- a/t/t6060-merge-index.sh +++ b/t/t6060-merge-index.sh @@ -1,6 +1,8 @@ #!/bin/sh test_description='basic git merge-index / git-merge-one-file tests' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup diverging branches' ' diff --git a/t/t6401-merge-criss-cross.sh b/t/t6401-merge-criss-cross.sh index 9d5e992878..1962310408 100755 --- a/t/t6401-merge-criss-cross.sh +++ b/t/t6401-merge-criss-cross.sh @@ -8,6 +8,8 @@ test_description='Test criss-cross merge' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'prepare repository' ' diff --git a/t/t6406-merge-attr.sh b/t/t6406-merge-attr.sh index 8650a88c40..5e4e4dd6d9 100755 --- a/t/t6406-merge-attr.sh +++ b/t/t6406-merge-attr.sh @@ -8,6 +8,7 @@ test_description='per path merge controlled by merge attribute' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success setup ' diff --git a/t/t6407-merge-binary.sh b/t/t6407-merge-binary.sh index e8a28717ce..0753fc95f4 100755 --- a/t/t6407-merge-binary.sh +++ b/t/t6407-merge-binary.sh @@ -5,6 +5,7 @@ test_description='ask merge-recursive to merge binary files' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success setup ' diff --git a/t/t7605-merge-resolve.sh b/t/t7605-merge-resolve.sh index 5d56c38546..62d935d31c 100755 --- a/t/t7605-merge-resolve.sh +++ b/t/t7605-merge-resolve.sh @@ -4,6 +4,7 @@ test_description='git merge Testing the resolve strategy.' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup' ' From fc47252d5b36fe2267e34aa9471dd7bb37d40545 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Tue, 8 Nov 2022 19:17:44 +0100 Subject: [PATCH 08/15] revision API: call graph_clear() in release_revisions() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Call graph_clear() in release_revisions(), this will free memory allocated by e.g. this command, which will now run without memory leaks: git -P log -1 --graph --no-graph --graph Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Taylor Blau --- revision.c | 1 + t/t3413-rebase-hook.sh | 1 + t/t4052-stat-output.sh | 1 + 3 files changed, 3 insertions(+) diff --git a/revision.c b/revision.c index 0760e78936..650dd17599 100644 --- a/revision.c +++ b/revision.c @@ -3020,6 +3020,7 @@ void release_revisions(struct rev_info *revs) date_mode_release(&revs->date_mode); release_revisions_mailmap(revs->mailmap); free_grep_patterns(&revs->grep_filter); + graph_clear(revs->graph); /* TODO (need to handle "no_free"): diff_free(&revs->diffopt) */ diff_free(&revs->pruning); reflog_walk_info_release(revs->reflog_info); diff --git a/t/t3413-rebase-hook.sh b/t/t3413-rebase-hook.sh index 9fab0d779b..e8456831e8 100755 --- a/t/t3413-rebase-hook.sh +++ b/t/t3413-rebase-hook.sh @@ -5,6 +5,7 @@ test_description='git rebase with its hook(s)' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success setup ' diff --git a/t/t4052-stat-output.sh b/t/t4052-stat-output.sh index b5c281edaa..3ee27e277d 100755 --- a/t/t4052-stat-output.sh +++ b/t/t4052-stat-output.sh @@ -8,6 +8,7 @@ test_description='test --stat output of various commands' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-terminal.sh From c07ce0602a7958299c7fbbf2a5b70d1510c03981 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Tue, 8 Nov 2022 19:17:45 +0100 Subject: [PATCH 09/15] ls-files: fix a --with-tree memory leak MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix a memory leak in overlay_tree_on_index(), we need to clear_pathspec() at some point, which might as well be after the last time we use it in the function. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Taylor Blau --- builtin/ls-files.c | 1 + t/t3060-ls-files-with-tree.sh | 2 ++ t/t9148-git-svn-propset.sh | 1 - 3 files changed, 3 insertions(+), 1 deletion(-) diff --git a/builtin/ls-files.c b/builtin/ls-files.c index 4cf8a23648..a03b559eca 100644 --- a/builtin/ls-files.c +++ b/builtin/ls-files.c @@ -613,6 +613,7 @@ void overlay_tree_on_index(struct index_state *istate, if (!fn) fn = read_one_entry_quick; err = read_tree(the_repository, tree, &pathspec, fn, istate); + clear_pathspec(&pathspec); if (err) die("unable to read tree entries %s", tree_name); diff --git a/t/t3060-ls-files-with-tree.sh b/t/t3060-ls-files-with-tree.sh index 52f76f7b57..c4a72ae446 100755 --- a/t/t3060-ls-files-with-tree.sh +++ b/t/t3060-ls-files-with-tree.sh @@ -8,6 +8,8 @@ test_description='git ls-files test (--with-tree). This test runs git ls-files --with-tree and in particular in a scenario known to trigger a crash with some versions of git. ' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup' ' diff --git a/t/t9148-git-svn-propset.sh b/t/t9148-git-svn-propset.sh index 6cc76a07b3..aebb28995e 100755 --- a/t/t9148-git-svn-propset.sh +++ b/t/t9148-git-svn-propset.sh @@ -5,7 +5,6 @@ test_description='git svn propset tests' -TEST_FAILS_SANITIZE_LEAK=true . ./lib-git-svn.sh test_expect_success 'setup propset via import' ' From f1f4ebf432978eb363ed210d0de40f5826ff91e5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Tue, 8 Nov 2022 19:17:46 +0100 Subject: [PATCH 10/15] sequencer.c: fix "opts->strategy" leak in read_strategy_opts() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When "read_strategy_opts()" is called we may have populated the "opts->strategy" before, so we'll need to free() it to avoid leaking memory. We populate it before because we cal get_replay_opts() from within "rebase.c" with an already populated "opts", which we then copy. Then if we're doing a "rebase -i" the sequencer API itself will promptly clobber our alloc'd version of it with its own. If this code is changed to do, instead of the added free() here a: if (opts->strategy) opts->strategy = xstrdup("another leak"); We get a couple of stacktraces from -fsanitize=leak showing how we ended up clobbering the already allocated value, i.e.: Direct leak of 6 byte(s) in 1 object(s) allocated from: #0 0x7f2e8cd45545 in __interceptor_malloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:75 #1 0x7f2e8cb0fcaa in __GI___strdup string/strdup.c:42 #2 0x6c4778 in xstrdup wrapper.c:39 #3 0x66bcb8 in read_strategy_opts sequencer.c:2902 #4 0x66bf7b in read_populate_opts sequencer.c:2969 #5 0x6723f9 in sequencer_continue sequencer.c:5063 #6 0x4a4f74 in run_sequencer_rebase builtin/rebase.c:348 #7 0x4a64c8 in run_specific_rebase builtin/rebase.c:753 #8 0x4a9b8b in cmd_rebase builtin/rebase.c:1824 #9 0x407a32 in run_builtin git.c:466 #10 0x407e0a in handle_builtin git.c:721 #11 0x40803d in run_argv git.c:788 #12 0x40850f in cmd_main git.c:923 #13 0x4eee79 in main common-main.c:57 #14 0x7f2e8ca9f209 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58 #15 0x7f2e8ca9f2bb in __libc_start_main_impl ../csu/libc-start.c:389 #16 0x405fd0 in _start (git+0x405fd0) Direct leak of 4 byte(s) in 1 object(s) allocated from: #0 0x7f2e8cd45545 in __interceptor_malloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:75 #1 0x7f2e8cb0fcaa in __GI___strdup string/strdup.c:42 #2 0x6c4778 in xstrdup wrapper.c:39 #3 0x4a3c31 in xstrdup_or_null git-compat-util.h:1169 #4 0x4a447a in get_replay_opts builtin/rebase.c:163 #5 0x4a4f5b in run_sequencer_rebase builtin/rebase.c:346 #6 0x4a64c8 in run_specific_rebase builtin/rebase.c:753 #7 0x4a9b8b in cmd_rebase builtin/rebase.c:1824 #8 0x407a32 in run_builtin git.c:466 #9 0x407e0a in handle_builtin git.c:721 #10 0x40803d in run_argv git.c:788 #11 0x40850f in cmd_main git.c:923 #12 0x4eee79 in main common-main.c:57 #13 0x7f2e8ca9f209 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58 #14 0x7f2e8ca9f2bb in __libc_start_main_impl ../csu/libc-start.c:389 #15 0x405fd0 in _start (git+0x405fd0) This can be seen in e.g. the 4th test of "t3404-rebase-interactive.sh". In the larger picture the ownership of the "struct replay_opts" is quite a mess, e.g. in this case rebase.c's static "get_replay_opts()" function partially creates it, but nothing in rebase.c will free() it. The structure is "mostly owned" by the sequencer API, but it also expects to get these partially populated versions of it. It would be better to have rebase keep track of what it allocated, and free() that, and to pass that as a "const" to the sequencer API, which would copy what it needs to its own version, and to free() that. But doing so is a much larger change, and however messy the ownership boundary is here is consistent with what we're doing already, so let's just free() this to fix the leak. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Taylor Blau --- sequencer.c | 1 + 1 file changed, 1 insertion(+) diff --git a/sequencer.c b/sequencer.c index e23f6f0b71..433fec2156 100644 --- a/sequencer.c +++ b/sequencer.c @@ -2897,6 +2897,7 @@ static void read_strategy_opts(struct replay_opts *opts, struct strbuf *buf) strbuf_reset(buf); if (!read_oneliner(buf, rebase_path_strategy(), 0)) return; + free(opts->strategy); opts->strategy = strbuf_detach(buf, NULL); if (!read_oneliner(buf, rebase_path_strategy_opts(), 0)) return; From dd4143e7bf406a5907ef86c50c77c4667f62375d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Tue, 8 Nov 2022 19:17:47 +0100 Subject: [PATCH 11/15] connected.c: free the "struct packed_git" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The "new_pack" we allocate in check_connected() wasn't being free'd. Let's do that before we return from the function. This has leaked ever since "new_pack" was added to this function in c6807a40dcd (clone: open a shortcut for connectivity check, 2013-05-26). Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Taylor Blau --- connected.c | 6 +++++- t/t3050-subprojects-fetch.sh | 1 + t/t4067-diff-partial-clone.sh | 1 + t/t5544-pack-objects-hook.sh | 2 ++ t/t5610-clone-detached.sh | 1 + t/t5611-clone-config.sh | 1 + t/t5614-clone-submodules-shallow.sh | 1 + t/t5617-clone-submodules-remote.sh | 1 + 8 files changed, 13 insertions(+), 1 deletion(-) diff --git a/connected.c b/connected.c index 74a20cb32e..b7770825c7 100644 --- a/connected.c +++ b/connected.c @@ -85,6 +85,7 @@ int check_connected(oid_iterate_fn fn, void *cb_data, promisor_pack_found: ; } while ((oid = fn(cb_data)) != NULL); + free(new_pack); return 0; } @@ -118,8 +119,10 @@ no_promisor_pack_found: else rev_list.no_stderr = opt->quiet; - if (start_command(&rev_list)) + if (start_command(&rev_list)) { + free(new_pack); return error(_("Could not run 'git rev-list'")); + } sigchain_push(SIGPIPE, SIG_IGN); @@ -151,5 +154,6 @@ no_promisor_pack_found: err = error_errno(_("failed to close rev-list's stdin")); sigchain_pop(SIGPIPE); + free(new_pack); return finish_command(&rev_list) || err; } diff --git a/t/t3050-subprojects-fetch.sh b/t/t3050-subprojects-fetch.sh index f1f09abdd9..3884694165 100755 --- a/t/t3050-subprojects-fetch.sh +++ b/t/t3050-subprojects-fetch.sh @@ -2,6 +2,7 @@ test_description='fetching and pushing project with subproject' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success setup ' diff --git a/t/t4067-diff-partial-clone.sh b/t/t4067-diff-partial-clone.sh index 28f42a4046..f60f5cbd65 100755 --- a/t/t4067-diff-partial-clone.sh +++ b/t/t4067-diff-partial-clone.sh @@ -2,6 +2,7 @@ test_description='behavior of diff when reading objects in a partial clone' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'git show batches blobs' ' diff --git a/t/t5544-pack-objects-hook.sh b/t/t5544-pack-objects-hook.sh index 54f54f8d2e..1a9e14bbcc 100755 --- a/t/t5544-pack-objects-hook.sh +++ b/t/t5544-pack-objects-hook.sh @@ -1,6 +1,8 @@ #!/bin/sh test_description='test custom script in place of pack-objects' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'create some history to fetch' ' diff --git a/t/t5610-clone-detached.sh b/t/t5610-clone-detached.sh index a7ec21eda5..022ed3d87c 100755 --- a/t/t5610-clone-detached.sh +++ b/t/t5610-clone-detached.sh @@ -4,6 +4,7 @@ test_description='test cloning a repository with detached HEAD' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh head_is_detached() { diff --git a/t/t5611-clone-config.sh b/t/t5611-clone-config.sh index 4b3877216e..727caff443 100755 --- a/t/t5611-clone-config.sh +++ b/t/t5611-clone-config.sh @@ -4,6 +4,7 @@ test_description='tests for git clone -c key=value' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'clone -c sets config in cloned repo' ' diff --git a/t/t5614-clone-submodules-shallow.sh b/t/t5614-clone-submodules-shallow.sh index 0c85ef834a..c2a2bb453e 100755 --- a/t/t5614-clone-submodules-shallow.sh +++ b/t/t5614-clone-submodules-shallow.sh @@ -2,6 +2,7 @@ test_description='Test shallow cloning of repos with submodules' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh pwd=$(pwd) diff --git a/t/t5617-clone-submodules-remote.sh b/t/t5617-clone-submodules-remote.sh index 6884338249..5a4d7936a7 100755 --- a/t/t5617-clone-submodules-remote.sh +++ b/t/t5617-clone-submodules-remote.sh @@ -5,6 +5,7 @@ test_description='Test cloning repos with submodules using remote-tracking branc GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh pwd=$(pwd) From 5ff6e8afac66db0dddb7b12744786e08a293784e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Tue, 8 Nov 2022 19:17:48 +0100 Subject: [PATCH 12/15] rebase: don't leak on "--abort" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix a leak in the recent 6159e7add49 (rebase --abort: improve reflog message, 2022-10-12). Before that commit we'd strbuf_release() the reflog message we were formatting, but when that code was refactored to use "ropts.head_msg" the strbuf_release() was omitted. Ideally the three users of "ropts" in cmd_rebase() should use different "ropts" variables, in practice they're completely separate, as this and the other user in the "switch" statement will "goto cleanup", which won't touch "ropts". The third caller after the "switch" is then unreachable if we take these two branches, so all of them are getting a "{ 0 }" init'd "ropts". So it's OK that we're leaving a stale pointer in "ropts.head_msg", cleaning it up was our responsibility, and it won't be used again. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Taylor Blau --- builtin/rebase.c | 1 + t/t7517-per-repo-email.sh | 1 + 2 files changed, 2 insertions(+) diff --git a/builtin/rebase.c b/builtin/rebase.c index 6a83132031..3f360eb2f3 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -1322,6 +1322,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) if (reset_head(the_repository, &ropts) < 0) die(_("could not move back to %s"), oid_to_hex(&options.orig_head->object.oid)); + strbuf_release(&head_msg); remove_branch_state(the_repository, 0); ret = finish_rebase(&options); goto cleanup; diff --git a/t/t7517-per-repo-email.sh b/t/t7517-per-repo-email.sh index 163ae80468..efc6496e2b 100755 --- a/t/t7517-per-repo-email.sh +++ b/t/t7517-per-repo-email.sh @@ -9,6 +9,7 @@ test_description='per-repo forced setting of email address' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup a likely user.useConfigOnly use case' ' From d1ec656d68fe6dfc421a5c96d009fbd2e1fa76f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Tue, 8 Nov 2022 19:17:49 +0100 Subject: [PATCH 13/15] cherry-pick: free "struct replay_opts" members MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Call the release_revisions() function added in 1878b5edc03 (revision.[ch]: provide and start using a release_revisions(), 2022-04-13) in cmd_cherry_pick(), as well as freeing the xmalloc()'d "revs" member itself. This is the same change as the one made for cmd_revert() a few lines above it in fd74ac95ac3 (revert: free "struct replay_opts" members, 2022-07-01). Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Taylor Blau --- builtin/revert.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/builtin/revert.c b/builtin/revert.c index ee32c714a7..0f81c8a795 100644 --- a/builtin/revert.c +++ b/builtin/revert.c @@ -261,6 +261,9 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix) opts.action = REPLAY_PICK; sequencer_init_config(&opts); res = run_sequencer(argc, argv, &opts); + if (opts.revs) + release_revisions(opts.revs); + free(opts.revs); if (res < 0) die(_("cherry-pick failed")); return res; From 603f2f5719f8a85996d5f20cf0e56186c149b923 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Tue, 8 Nov 2022 19:17:50 +0100 Subject: [PATCH 14/15] revert: fix parse_options_concat() leak MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Free memory from parse_options_concat(), which comes from code originally added (then extended) in [1]. At this point we could get several more tests leak-free by free()-ing the xstrdup() just above the line being changed, but that one's trickier than it seems. The sequencer_remove_state() function supposedly owns it, but sometimes we don't call it. I have a fix for it, but it's non-trivial, so let's fix the easy one first. 1. c62f6ec341b (revert: add --ff option to allow fast forward when cherry-picking, 2010-03-06) Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Taylor Blau --- builtin/revert.c | 1 + t/t3429-rebase-edit-todo.sh | 1 + 2 files changed, 2 insertions(+) diff --git a/builtin/revert.c b/builtin/revert.c index 0f81c8a795..8bc87e4c77 100644 --- a/builtin/revert.c +++ b/builtin/revert.c @@ -221,6 +221,7 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts) opts->strategy = xstrdup_or_null(opts->strategy); if (!opts->strategy && getenv("GIT_TEST_MERGE_ALGORITHM")) opts->strategy = xstrdup(getenv("GIT_TEST_MERGE_ALGORITHM")); + free(options); if (cmd == 'q') { int ret = sequencer_remove_state(opts); diff --git a/t/t3429-rebase-edit-todo.sh b/t/t3429-rebase-edit-todo.sh index abd66f3602..8e0d03969a 100755 --- a/t/t3429-rebase-edit-todo.sh +++ b/t/t3429-rebase-edit-todo.sh @@ -2,6 +2,7 @@ test_description='rebase should reread the todo file if an exec modifies it' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-rebase.sh From ac95f5d36adb42980064587fd7910fa275ff853e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Tue, 8 Nov 2022 19:17:51 +0100 Subject: [PATCH 15/15] built-ins: use free() not UNLEAK() if trivial, rm dead code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit For a lot of uses of UNLEAK() it would be quite tricky to release the memory involved, or we're missing the relevant *_(release|clear)() functions. But in these cases we have them already, and can just invoke them on the variable(s) involved, instead of UNLEAK(). For "builtin/worktree.c" the UNLEAK() was also added in [1], but the struct member it's unleaking was removed in [2]. The only non-"int" member of that structure is "const char *keep_locked", which comes to us via "argv" or a string literal[3]. We have good visibility via the compiler and tooling (e.g. SANITIZE=address) on bad free()-ing, but none on UNLEAK() we don't need anymore. So let's prefer releasing the memory when it's easy. For "bugreport", "worktree" and "config" we need to start using a "ret = ..." return pattern. For "builtin/bugreport.c" these UNLEAK() were added in [4], and for "builtin/config.c" in [1]. For "config" the code seen here was the only user of the "value" variable. For "ACTION_{RENAME,REMOVE}_SECTION" we need to be sure to return the right exit code in the cases where we were relying on falling through to the top-level. I think there's still a use-case for UNLEAK(), but hat it's changed since then. Using it so that "we can see the real leaks" is counter-productive in these cases. It's more useful to have UNLEAK() be a marker of the remaining odd cases where it's hard to free() the memory for whatever reason. With this change less than 20 of them remain in-tree. 1. 0e5bba53af7 (add UNLEAK annotation for reducing leak false positives, 2017-09-08) 2. d861d34a6ed (worktree: remove extra members from struct add_opts, 2018-04-24) 3. 0db4961c49b (worktree: teach `add` to accept --reason with --lock, 2021-07-15) 4. 0e5bba53af7 and 00d8c311050 (commit: fix "author_ident" leak, 2022-05-12). Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Taylor Blau --- builtin/add.c | 2 +- builtin/bugreport.c | 9 ++++++--- builtin/commit.c | 6 +++--- builtin/config.c | 42 ++++++++++++++++++++---------------------- builtin/diff.c | 2 +- builtin/worktree.c | 7 ++++--- 6 files changed, 35 insertions(+), 33 deletions(-) diff --git a/builtin/add.c b/builtin/add.c index f84372964c..c68ebafed5 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -695,6 +695,6 @@ finish: die(_("Unable to write new index file")); dir_clear(&dir); - UNLEAK(pathspec); + clear_pathspec(&pathspec); return exit_status; } diff --git a/builtin/bugreport.c b/builtin/bugreport.c index 96052541cb..5bc254be80 100644 --- a/builtin/bugreport.c +++ b/builtin/bugreport.c @@ -106,6 +106,7 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix) const char *user_relative_path = NULL; char *prefixed_filename; size_t output_path_len; + int ret; const struct option bugreport_options[] = { OPT_CALLBACK_F(0, "diagnose", &diagnose, N_("mode"), @@ -182,7 +183,9 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix) user_relative_path); free(prefixed_filename); - UNLEAK(buffer); - UNLEAK(report_path); - return !!launch_editor(report_path.buf, NULL, NULL); + strbuf_release(&buffer); + + ret = !!launch_editor(report_path.buf, NULL, NULL); + strbuf_release(&report_path); + return ret; } diff --git a/builtin/commit.c b/builtin/commit.c index c291199b70..f88a29167f 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -1874,8 +1874,8 @@ 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); + strbuf_release(&author_ident); + strbuf_release(&err); + strbuf_release(&sb); return ret; } diff --git a/builtin/config.c b/builtin/config.c index 753e5fac29..060cf9f3e0 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -639,8 +639,9 @@ static char *default_user_config(void) int cmd_config(int argc, const char **argv, const char *prefix) { int nongit = !startup_info->have_repository; - char *value; + char *value = NULL; int flags = 0; + int ret = 0; given_config_source.file = xstrdup_or_null(getenv(CONFIG_ENVIRONMENT)); @@ -856,44 +857,38 @@ int cmd_config(int argc, const char **argv, const char *prefix) free(config_file); } else if (actions == ACTION_SET) { - int ret; check_write(); check_argc(argc, 2, 2); value = normalize_value(argv[0], argv[1]); - UNLEAK(value); ret = git_config_set_in_file_gently(given_config_source.file, argv[0], value); if (ret == CONFIG_NOTHING_SET) error(_("cannot overwrite multiple values with a single value\n" " Use a regexp, --add or --replace-all to change %s."), argv[0]); - return ret; } else if (actions == ACTION_SET_ALL) { check_write(); check_argc(argc, 2, 3); value = normalize_value(argv[0], argv[1]); - UNLEAK(value); - return git_config_set_multivar_in_file_gently(given_config_source.file, - argv[0], value, argv[2], - flags); + ret = git_config_set_multivar_in_file_gently(given_config_source.file, + argv[0], value, argv[2], + flags); } else if (actions == ACTION_ADD) { check_write(); check_argc(argc, 2, 2); value = normalize_value(argv[0], argv[1]); - UNLEAK(value); - return git_config_set_multivar_in_file_gently(given_config_source.file, - argv[0], value, - CONFIG_REGEX_NONE, - flags); + ret = git_config_set_multivar_in_file_gently(given_config_source.file, + argv[0], value, + CONFIG_REGEX_NONE, + flags); } else if (actions == ACTION_REPLACE_ALL) { check_write(); check_argc(argc, 2, 3); value = normalize_value(argv[0], argv[1]); - UNLEAK(value); - return git_config_set_multivar_in_file_gently(given_config_source.file, - argv[0], value, argv[2], - flags | CONFIG_FLAGS_MULTI_REPLACE); + ret = git_config_set_multivar_in_file_gently(given_config_source.file, + argv[0], value, argv[2], + flags | CONFIG_FLAGS_MULTI_REPLACE); } else if (actions == ACTION_GET) { check_argc(argc, 1, 2); @@ -934,26 +929,28 @@ int cmd_config(int argc, const char **argv, const char *prefix) flags | CONFIG_FLAGS_MULTI_REPLACE); } else if (actions == ACTION_RENAME_SECTION) { - int ret; check_write(); check_argc(argc, 2, 2); ret = git_config_rename_section_in_file(given_config_source.file, argv[0], argv[1]); if (ret < 0) return ret; - if (ret == 0) + else if (!ret) die(_("no such section: %s"), argv[0]); + else + ret = 0; } else if (actions == ACTION_REMOVE_SECTION) { - int ret; check_write(); check_argc(argc, 1, 1); ret = git_config_rename_section_in_file(given_config_source.file, argv[0], NULL); if (ret < 0) return ret; - if (ret == 0) + else if (!ret) die(_("no such section: %s"), argv[0]); + else + ret = 0; } else if (actions == ACTION_GET_COLOR) { check_argc(argc, 1, 2); @@ -966,5 +963,6 @@ int cmd_config(int argc, const char **argv, const char *prefix) return get_colorbool(argv[0], argc == 2); } - return 0; + free(value); + return ret; } diff --git a/builtin/diff.c b/builtin/diff.c index 854d2c5a5c..cb63f157dd 100644 --- a/builtin/diff.c +++ b/builtin/diff.c @@ -609,7 +609,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix) if (1 < rev.diffopt.skip_stat_unmatch) refresh_index_quietly(); release_revisions(&rev); - UNLEAK(ent); + object_array_clear(&ent); UNLEAK(blob); return result; } diff --git a/builtin/worktree.c b/builtin/worktree.c index 4a24d53be1..591d659fae 100644 --- a/builtin/worktree.c +++ b/builtin/worktree.c @@ -629,6 +629,7 @@ static int add(int ac, const char **av, const char *prefix) N_("try to match the new branch name with a remote-tracking branch")), OPT_END() }; + int ret; memset(&opts, 0, sizeof(opts)); opts.checkout = 1; @@ -705,9 +706,9 @@ static int add(int ac, const char **av, const char *prefix) die(_("--[no-]track can only be used if a new branch is created")); } - UNLEAK(path); - UNLEAK(opts); - return add_worktree(path, branch, &opts); + ret = add_worktree(path, branch, &opts); + free(path); + return ret; } static void show_worktree_porcelain(struct worktree *wt, int line_terminator)