| Message ID | cover-00.17-00000000000-20221103T164632Z-avarab@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | leak fixes: use existing constructors & other trivia | expand |
On 03/11/2022 17:05, Ævar Arnfjörð Bjarmason wrote: > With the very minor exceptions of: > > * 03-04/17 (which need trivial oilerplate) > * 05/17 (need to add trivial control flow to a free_*() function) > * 12/17 (narrowing scope of allocation) I've only looked at the rebase related patches. I'd really appreciate it if you could drop patches 12 & 14 as they conflict with [1] that fixes these issues by removing the setenv() calls. Thanks Phillip [1] https://lore.kernel.org/git/pull.1405.git.1667575142.gitgitgadget@gmail.com > * 17/17: Add "goto ret" pattern, combine two "ret" variables > > These are all "one-line" leak fixes where we merely need to make use > of an existing release function. The "one-line" only having the slight > disclaimer of needing to e.g. add braces to an "if" in one case, etc. > > Each commit in this series is tested with: > > GIT_TEST_PASSING_SANITIZE_LEAK=check GIT_TEST_SANITIZE_LEAK_LOG=true \ > make SANITIZE=leak test > > I.e. mark tests as leak-free as we fix the leaks. > > In 17/17 I replace uses of UNLEAK() where we can just as easily free() > instead, i.e. most of it's built-ins doing UNLEAK(x) instead of > strbuf_release(&x) etc. > > As 17/17 notes I still think these's a place for unleak (some of the > remaining ones are quite tricky), but that we gain more from leaving > it for those tricky cases. Before this series we have 28 uses of > UNLEAK(), after it's 15. > > Ævar Arnfjörð Bjarmason (17): > tests: mark tests as passing with SANITIZE=leak > {reset,merge}: call discard_index() before returning > commit: discard partial cache before (re-)reading it > read-cache.c: clear and free "sparse_checkout_patterns" > dir.c: free "ident" and "exclude_per_dir" in "struct untracked_cache" > built-ins & libs & helpers: add/move destructors, fix leaks > unpack-file: fix ancient leak in create_temp_file() > revision API: call graph_clear() in release_revisions() > ls-files: fix a --with-tree memory leak > sequencer.c: fix "opts->strategy" leak in read_strategy_opts() > connected.c: free the "struct packed_git" > sequencer.c: fix a pick_commits() leak > rebase: don't leak on "--abort" > sequencer.c: fix sequencer_continue() leak > cherry-pick: free "struct replay_opts" members > revert: fix parse_options_concat() leak > built-ins: use free() not UNLEAK() if trivial, rm dead code > > builtin/add.c | 2 +- > builtin/bugreport.c | 9 +++-- > builtin/checkout.c | 2 ++ > builtin/commit.c | 13 +++++--- > builtin/config.c | 42 +++++++++++------------- > builtin/diff.c | 2 +- > builtin/ls-files.c | 1 + > builtin/merge.c | 1 + > builtin/rebase.c | 4 +++ > builtin/repack.c | 2 +- > builtin/reset.c | 2 ++ > builtin/rev-parse.c | 1 + > builtin/revert.c | 4 +++ > builtin/stash.c | 2 ++ > builtin/unpack-file.c | 1 + > builtin/worktree.c | 7 ++-- > connected.c | 6 +++- > dir.c | 10 ++++-- > dir.h | 1 + > read-cache.c | 5 +++ > ref-filter.c | 1 + > revision.c | 1 + > sequencer.c | 12 +++++-- > t/helper/test-fake-ssh.c | 1 + > t/t0068-for-each-repo.sh | 1 + > t/t0070-fundamental.sh | 1 + > t/t1011-read-tree-sparse-checkout.sh | 1 + > t/t1022-read-tree-partial-clone.sh | 2 +- > 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/t2012-checkout-last.sh | 1 + > t/t2018-checkout-branch.sh | 1 + > t/t2025-checkout-no-overlay.sh | 1 + > t/t3009-ls-files-others-nonsubmodule.sh | 1 + > t/t3010-ls-files-killed-modified.sh | 2 ++ > t/t3050-subprojects-fetch.sh | 1 + > t/t3060-ls-files-with-tree.sh | 2 ++ > t/t3409-rebase-environ.sh | 1 + > t/t3413-rebase-hook.sh | 1 + > t/t3428-rebase-signoff.sh | 1 + > t/t3429-rebase-edit-todo.sh | 1 + > t/t3433-rebase-across-mode-change.sh | 1 + > t/t4015-diff-whitespace.sh | 4 +-- > t/t4045-diff-relative.sh | 2 ++ > t/t4052-stat-output.sh | 1 + > t/t4053-diff-no-index.sh | 1 + > t/t4067-diff-partial-clone.sh | 1 + > t/t4111-apply-subdir.sh | 1 + > t/t4135-apply-weird-filenames.sh | 1 + > t/t4213-log-tabexpand.sh | 1 + > t/t5544-pack-objects-hook.sh | 2 ++ > t/t5554-noop-fetch-negotiator.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 + > t/t5618-alternate-refs.sh | 2 ++ > t/t6060-merge-index.sh | 2 ++ > t/t6301-for-each-ref-errors.sh | 1 + > t/t6401-merge-criss-cross.sh | 2 ++ > t/t6406-merge-attr.sh | 1 + > t/t6407-merge-binary.sh | 1 + > t/t6415-merge-dir-to-symlink.sh | 1 + > t/t6435-merge-sparse.sh | 1 + > t/t7103-reset-bare.sh | 2 +- > t/t7504-commit-msg-hook.sh | 1 + > t/t7517-per-repo-email.sh | 1 + > t/t7520-ignored-hook-warning.sh | 1 + > t/t7605-merge-resolve.sh | 1 + > t/t7614-merge-signoff.sh | 1 + > t/t9003-help-autocorrect.sh | 2 ++ > t/t9115-git-svn-dcommit-funky-renames.sh | 1 - > t/t9146-git-svn-empty-dirs.sh | 1 - > t/t9148-git-svn-propset.sh | 1 - > t/t9160-git-svn-preserve-empty-dirs.sh | 1 - > 77 files changed, 150 insertions(+), 51 deletions(-) >
On Fri, Nov 04 2022, Phillip Wood wrote: > On 03/11/2022 17:05, Ævar Arnfjörð Bjarmason wrote: >> With the very minor exceptions of: >> * 03-04/17 (which need trivial oilerplate) >> * 05/17 (need to add trivial control flow to a free_*() function) >> * 12/17 (narrowing scope of allocation) > > I've only looked at the rebase related patches. I'd really appreciate > it if you could drop patches 12 & 14 as they conflict with [1] that > fixes these issues by removing the setenv() calls. Sure, sounds good to me. I see Taylor queued it up like that, and the combination of the two passes with the "true" and "check" mode leak checking modes. For context: I saw that you planned to fix those in some follow-up topic, but had this mostly ready before the new leak in the recent topic showed up, and that leak had big knock-on effects in what test files I could mark as passing. So I figured I'd include some more more narrow fixes, but I'm even happier to see the root cause addressed.
On 05/11/2022 12:46, Ævar Arnfjörð Bjarmason wrote: > > On Fri, Nov 04 2022, Phillip Wood wrote: > >> On 03/11/2022 17:05, Ævar Arnfjörð Bjarmason wrote: >>> With the very minor exceptions of: >>> * 03-04/17 (which need trivial oilerplate) >>> * 05/17 (need to add trivial control flow to a free_*() function) >>> * 12/17 (narrowing scope of allocation) >> >> I've only looked at the rebase related patches. I'd really appreciate >> it if you could drop patches 12 & 14 as they conflict with [1] that >> fixes these issues by removing the setenv() calls. > > Sure, sounds good to me. Thanks > I see Taylor queued it up like that, and the combination of the two > passes with the "true" and "check" mode leak checking modes. That's great Best Wishes Phillip > For context: I saw that you planned to fix those in some follow-up > topic, but had this mostly ready before the new leak in the recent topic > showed up, and that leak had big knock-on effects in what test files I > could mark as passing. > > So I figured I'd include some more more narrow fixes, but I'm even > happier to see the root cause addressed.
With the very minor exceptions of: * 03-04/17 (which need trivial oilerplate) * 05/17 (need to add trivial control flow to a free_*() function) * 12/17 (narrowing scope of allocation) * 17/17: Add "goto ret" pattern, combine two "ret" variables These are all "one-line" leak fixes where we merely need to make use of an existing release function. The "one-line" only having the slight disclaimer of needing to e.g. add braces to an "if" in one case, etc. Each commit in this series is tested with: GIT_TEST_PASSING_SANITIZE_LEAK=check GIT_TEST_SANITIZE_LEAK_LOG=true \ make SANITIZE=leak test I.e. mark tests as leak-free as we fix the leaks. In 17/17 I replace uses of UNLEAK() where we can just as easily free() instead, i.e. most of it's built-ins doing UNLEAK(x) instead of strbuf_release(&x) etc. As 17/17 notes I still think these's a place for unleak (some of the remaining ones are quite tricky), but that we gain more from leaving it for those tricky cases. Before this series we have 28 uses of UNLEAK(), after it's 15. Ævar Arnfjörð Bjarmason (17): tests: mark tests as passing with SANITIZE=leak {reset,merge}: call discard_index() before returning commit: discard partial cache before (re-)reading it read-cache.c: clear and free "sparse_checkout_patterns" dir.c: free "ident" and "exclude_per_dir" in "struct untracked_cache" built-ins & libs & helpers: add/move destructors, fix leaks unpack-file: fix ancient leak in create_temp_file() revision API: call graph_clear() in release_revisions() ls-files: fix a --with-tree memory leak sequencer.c: fix "opts->strategy" leak in read_strategy_opts() connected.c: free the "struct packed_git" sequencer.c: fix a pick_commits() leak rebase: don't leak on "--abort" sequencer.c: fix sequencer_continue() leak cherry-pick: free "struct replay_opts" members revert: fix parse_options_concat() leak built-ins: use free() not UNLEAK() if trivial, rm dead code builtin/add.c | 2 +- builtin/bugreport.c | 9 +++-- builtin/checkout.c | 2 ++ builtin/commit.c | 13 +++++--- builtin/config.c | 42 +++++++++++------------- builtin/diff.c | 2 +- builtin/ls-files.c | 1 + builtin/merge.c | 1 + builtin/rebase.c | 4 +++ builtin/repack.c | 2 +- builtin/reset.c | 2 ++ builtin/rev-parse.c | 1 + builtin/revert.c | 4 +++ builtin/stash.c | 2 ++ builtin/unpack-file.c | 1 + builtin/worktree.c | 7 ++-- connected.c | 6 +++- dir.c | 10 ++++-- dir.h | 1 + read-cache.c | 5 +++ ref-filter.c | 1 + revision.c | 1 + sequencer.c | 12 +++++-- t/helper/test-fake-ssh.c | 1 + t/t0068-for-each-repo.sh | 1 + t/t0070-fundamental.sh | 1 + t/t1011-read-tree-sparse-checkout.sh | 1 + t/t1022-read-tree-partial-clone.sh | 2 +- 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/t2012-checkout-last.sh | 1 + t/t2018-checkout-branch.sh | 1 + t/t2025-checkout-no-overlay.sh | 1 + t/t3009-ls-files-others-nonsubmodule.sh | 1 + t/t3010-ls-files-killed-modified.sh | 2 ++ t/t3050-subprojects-fetch.sh | 1 + t/t3060-ls-files-with-tree.sh | 2 ++ t/t3409-rebase-environ.sh | 1 + t/t3413-rebase-hook.sh | 1 + t/t3428-rebase-signoff.sh | 1 + t/t3429-rebase-edit-todo.sh | 1 + t/t3433-rebase-across-mode-change.sh | 1 + t/t4015-diff-whitespace.sh | 4 +-- t/t4045-diff-relative.sh | 2 ++ t/t4052-stat-output.sh | 1 + t/t4053-diff-no-index.sh | 1 + t/t4067-diff-partial-clone.sh | 1 + t/t4111-apply-subdir.sh | 1 + t/t4135-apply-weird-filenames.sh | 1 + t/t4213-log-tabexpand.sh | 1 + t/t5544-pack-objects-hook.sh | 2 ++ t/t5554-noop-fetch-negotiator.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 + t/t5618-alternate-refs.sh | 2 ++ t/t6060-merge-index.sh | 2 ++ t/t6301-for-each-ref-errors.sh | 1 + t/t6401-merge-criss-cross.sh | 2 ++ t/t6406-merge-attr.sh | 1 + t/t6407-merge-binary.sh | 1 + t/t6415-merge-dir-to-symlink.sh | 1 + t/t6435-merge-sparse.sh | 1 + t/t7103-reset-bare.sh | 2 +- t/t7504-commit-msg-hook.sh | 1 + t/t7517-per-repo-email.sh | 1 + t/t7520-ignored-hook-warning.sh | 1 + t/t7605-merge-resolve.sh | 1 + t/t7614-merge-signoff.sh | 1 + t/t9003-help-autocorrect.sh | 2 ++ t/t9115-git-svn-dcommit-funky-renames.sh | 1 - t/t9146-git-svn-empty-dirs.sh | 1 - t/t9148-git-svn-propset.sh | 1 - t/t9160-git-svn-preserve-empty-dirs.sh | 1 - 77 files changed, 150 insertions(+), 51 deletions(-)