On Thu, Dec 01 2022, René Scharfe wrote: > Am 01.12.2022 um 17:54 schrieb Ævar Arnfjörð Bjarmason: >> >> On Thu, Dec 01 2022, René Scharfe wrote: >> >>> Calling repo_init_revisions() and release_revisions() in that order >>> leaks the memory allocated for the parseopts array in the embedded >>> struct diff_options member. Get rid of that leak by reducing the >>> lifetime of that array. >>> >>> Original patch: >>> https://lore.kernel.org/git/4fd82dc6-e0f8-0638-5b10-16bfef39a171@xxxxxx/ >>> >>> Submitted separately from that thread because it's independent enough. >>> >>> Change since v1: >>> - Actually remove the parseopts member. Its removal got lost during >>> refactoring in v1. Thank you for spotting that, Junio! >>> >>> diff: factor out add_diff_options() >>> diff: let prep_parse_options() return parseopt array >>> diff: remove parseopts member from struct diff_options >>> >>> builtin/range-diff.c | 2 +- >>> diff-no-index.c | 3 +-- >>> diff.c | 26 +++++++++++++++----------- >>> diff.h | 2 +- >>> 4 files changed, 18 insertions(+), 15 deletions(-) >>> >>> Range-Diff gegen v1: >>> 1: 630f95320f = 1: 4dc8b2632b diff: factor out add_diff_options() >>> 2: 4b56fa795c = 2: 10903d355e diff: let prep_parse_options() return parseopt array >>> 3: 7e54e4370a ! 3: 24bd18ae79 diff: remove parseopts member from struct diff_options >>> @@ diff.c: void diff_free(struct diff_options *options) >>> } >>> >>> void diff_flush(struct diff_options *options) >>> + >>> + ## diff.h ## >>> +@@ diff.h: struct diff_options { >>> + unsigned color_moved_ws_handling; >>> + >>> + struct repository *repo; >>> +- struct option *parseopts; >>> + struct strmap *additional_path_headers; >>> + >>> + int no_free; >> >> This looks good to me. Would you mind running the tests with: >> >> GIT_TEST_PASSING_SANITIZE_LEAK=check GIT_TEST_SANITIZE_LEAK_LOG=true make SANITIZE=leak >> >> And then marking up the ones that now pass with >> TEST_PASSES_SANITIZE_LEAK=true. I think it's all except one of these >> (one isn't marked on "master", I forget which one): >> >> Test Summary Report >> ------------------- >> t1022-read-tree-partial-clone.sh (Wstat: 256 Tests: 1 Failed: 0) >> Non-zero exit status: 1 >> t2012-checkout-last.sh (Wstat: 256 Tests: 22 Failed: 0) >> Non-zero exit status: 1 >> t3210-pack-refs.sh (Wstat: 256 Tests: 30 Failed: 0) >> Non-zero exit status: 1 >> t4053-diff-no-index.sh (Wstat: 256 Tests: 19 Failed: 0) >> Non-zero exit status: 1 >> t5554-noop-fetch-negotiator.sh (Wstat: 256 Tests: 1 Failed: 0) >> Non-zero exit status: 1 >> t5613-info-alternate.sh (Wstat: 256 Tests: 13 Failed: 0) >> Non-zero exit status: 1 >> t6021-rev-list-exclude-hidden.sh (Wstat: 256 Tests: 42 Failed: 0) >> Non-zero exit status: 1 >> t6415-merge-dir-to-symlink.sh (Wstat: 256 Tests: 24 Failed: 0) >> Non-zero exit status: 1 >> t7403-submodule-sync.sh (Wstat: 256 Tests: 18 Failed: 0) >> Non-zero exit status: 1 >> t7504-commit-msg-hook.sh (Wstat: 256 Tests: 30 Failed: 0) >> Non-zero exit status: 1 >> t9115-git-svn-dcommit-funky-renames.sh (Wstat: 256 Tests: 12 Failed: 0) >> Non-zero exit status: 1 >> t9146-git-svn-empty-dirs.sh (Wstat: 256 Tests: 14 Failed: 0) >> Non-zero exit status: 1 >> t9160-git-svn-preserve-empty-dirs.sh (Wstat: 256 Tests: 12 Failed: 0) >> Non-zero exit status: 1 >> >> I.e. this makes a lot more tests pass leak-free, yay! > > With -rc1 (i.e. without this series) I get: > > t1022-read-tree-partial-clone.sh (Wstat: 256 Tests: 1 Failed: 0) > Non-zero exit status: 1 > t2016-checkout-patch.sh (Wstat: 256 Tests: 16 Failed: 0) > Non-zero exit status: 1 > t2012-checkout-last.sh (Wstat: 256 Tests: 22 Failed: 0) > Non-zero exit status: 1 > t4023-diff-rename-typechange.sh (Wstat: 256 Tests: 4 Failed: 0) > Non-zero exit status: 1 > t4053-diff-no-index.sh (Wstat: 256 Tests: 19 Failed: 0) > Non-zero exit status: 1 > t4058-diff-duplicates.sh (Wstat: 256 Tests: 16 Failed: 0) > Non-zero exit status: 1 > t4205-log-pretty-formats.sh (Wstat: 256 Tests: 21 Failed: 0) > Non-zero exit status: 1 > Parse errors: No plan found in TAP output > t5406-remote-rejects.sh (Wstat: 256 Tests: 3 Failed: 0) > Non-zero exit status: 1 > t5507-remote-environment.sh (Wstat: 256 Tests: 5 Failed: 0) > Non-zero exit status: 1 > t5554-noop-fetch-negotiator.sh (Wstat: 256 Tests: 1 Failed: 0) > Non-zero exit status: 1 > t6001-rev-list-graft.sh (Wstat: 256 Tests: 14 Failed: 0) > Non-zero exit status: 1 > t6021-rev-list-exclude-hidden.sh (Wstat: 256 Tests: 42 Failed: 0) > Non-zero exit status: 1 > t6401-merge-criss-cross.sh (Wstat: 256 Tests: 4 Failed: 0) > Non-zero exit status: 1 > t6407-merge-binary.sh (Wstat: 256 Tests: 3 Failed: 0) > Non-zero exit status: 1 > t6415-merge-dir-to-symlink.sh (Wstat: 256 Tests: 24 Failed: 0) > Non-zero exit status: 1 > t7006-pager.sh (Wstat: 256 Tests: 109 Failed: 0) > Non-zero exit status: 1 > t7008-filter-branch-null-sha1.sh (Wstat: 256 Tests: 6 Failed: 0) > Non-zero exit status: 1 > t7504-commit-msg-hook.sh (Wstat: 256 Tests: 30 Failed: 0) > Non-zero exit status: 1 > t7517-per-repo-email.sh (Wstat: 256 Tests: 16 Failed: 0) > Non-zero exit status: 1 > t7605-merge-resolve.sh (Wstat: 256 Tests: 4 Failed: 0) > Non-zero exit status: 1 > > There is some overlap with your results, but also several differences. > I wonder why so many more tests appear to be leak-free for me. I used > Debian clang version 11.0.1-2. Sorry to have sent you down this rabbit hole, I misrecalled that it was relatively clean on "master", but it's "next" that's in that state (due to ab/various-leak-fixes). > In any case it seems we need to update the marks before we can > attribute which tests are made leak-free by any new patches. Yes, let's leave it for now in this series. I think it would make sense if "master" was almost clean, but I'll just submit another change like e5e37517dd9 (tests: mark tests as passing with SANITIZE=leak, 2022-11-08) after this lands.
