| Message ID | 20210620151204.19260-1-andrzej@ahunt.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | Fix all leaks in tests t0002-t0099: Part 2 | expand |
On Sun, Jun 20, 2021 at 8:14 AM <andrzej@ahunt.org> wrote: > > From: Andrzej Hunt <andrzej@ahunt.org> > > This series plugs more of the leaks that were found while running > t0002-t0099 with LSAN. > > See also the first series (already merged) at [1]. I'm currently > expecting at least another 2 series before t0002-t0099 run leak free. > I'm not being particularly systematic about the order of patches - > although I am trying to send out "real" (if mostly small) leaks first, > before sending out the more boring patches that add free()/UNLEAK() to > cmd_* and direct helpers thereof. I've read over the series. It provides some good clear fixes. I noted on patches 2, 6, and 12 that a some greps suggested that leaks similar to the ones being fixed likely also affect other places of the codebase. Those other places don't need to be fixed as part of this series, but they might be good items for #leftoverbits or GSoC early tasks (cc: Christian in case he wants to record those somewhere). I cc'ed Stolee on patch 4 because he suggested he wanted to read it in an earlier discussion. Phillip noted some issues with patch 11, and I added a couple more. The ownership of opts->strategy appears to be pretty messy and in need of cleanup. All the patches other than 11 look good to me.
On 21/06/2021 23:54, Elijah Newren wrote: > On Sun, Jun 20, 2021 at 8:14 AM <andrzej@ahunt.org> wrote: >> >> From: Andrzej Hunt <andrzej@ahunt.org> >> >> This series plugs more of the leaks that were found while running >> t0002-t0099 with LSAN. >> >> See also the first series (already merged) at [1]. I'm currently >> expecting at least another 2 series before t0002-t0099 run leak free. >> I'm not being particularly systematic about the order of patches - >> although I am trying to send out "real" (if mostly small) leaks first, >> before sending out the more boring patches that add free()/UNLEAK() to >> cmd_* and direct helpers thereof. > > I've read over the series. It provides some good clear fixes. I > noted on patches 2, 6, and 12 that a some greps suggested that leaks > similar to the ones being fixed likely also affect other places of the > codebase. Those other places don't need to be fixed as part of this > series, but they might be good items for #leftoverbits or GSoC early > tasks (cc: Christian in case he wants to record those somewhere). > > I cc'ed Stolee on patch 4 because he suggested he wanted to read it in > an earlier discussion. > > Phillip noted some issues with patch 11, and I added a couple more. > The ownership of opts->strategy appears to be pretty messy and in need > of cleanup. > > All the patches other than 11 look good to me. > Thank you for the careful reviews - and especially for pointing out when a given pattern does occur elsewhere in the codebase! As suggested I will skip the additional locations that you've found while reviewing this series - but I'm starting a separate series where I can address those. I have been focused on getting tests to pass leak-free one-by-one, but spotting and fixing patterns is probably more efficient (since author and reviewer already have the right context) and in some cases might fix leaks that aren't occurring during the tests. ATB, Andrzej
On Mon, Jun 21, 2021 at 11:54 PM Elijah Newren <newren@gmail.com> wrote: > > On Sun, Jun 20, 2021 at 8:14 AM <andrzej@ahunt.org> wrote: > > > > From: Andrzej Hunt <andrzej@ahunt.org> > > > > This series plugs more of the leaks that were found while running > > t0002-t0099 with LSAN. > > > > See also the first series (already merged) at [1]. I'm currently > > expecting at least another 2 series before t0002-t0099 run leak free. > > I'm not being particularly systematic about the order of patches - > > although I am trying to send out "real" (if mostly small) leaks first, > > before sending out the more boring patches that add free()/UNLEAK() to > > cmd_* and direct helpers thereof. > > I've read over the series. It provides some good clear fixes. I > noted on patches 2, 6, and 12 that a some greps suggested that leaks > similar to the ones being fixed likely also affect other places of the > codebase. Those other places don't need to be fixed as part of this > series, but they might be good items for #leftoverbits or GSoC early > tasks (cc: Christian in case he wants to record those somewhere). Yeah, thanks for letting me know!
From: Andrzej Hunt <andrzej@ahunt.org> This series plugs more of the leaks that were found while running t0002-t0099 with LSAN. See also the first series (already merged) at [1]. I'm currently expecting at least another 2 series before t0002-t0099 run leak free. I'm not being particularly systematic about the order of patches - although I am trying to send out "real" (if mostly small) leaks first, before sending out the more boring patches that add free()/UNLEAK() to cmd_* and direct helpers thereof. ATB, Andrzej [1] https://lore.kernel.org/git/pull.929.git.1617994052.gitgitgadget@gmail.com/ Andrzej Hunt (12): fmt-merge-msg: free newly allocated temporary strings when done environment: move strbuf into block to plug leak builtin/submodule--helper: release unused strbuf to avoid leak for-each-repo: remove unnecessary argv copy to plug leak diffcore-rename: move old_dir/new_dir definition to plug leak ref-filter: also free head for ATOM_HEAD to avoid leak read-cache: call diff_setup_done to avoid leak convert: release strbuf to avoid leak builtin/mv: free or UNLEAK multiple pointers at end of cmd_mv builtin/merge: free found_ref when done builtin/rebase: fix options.strategy memory lifecycle reset: clear_unpack_trees_porcelain to plug leak builtin/for-each-repo.c | 14 ++++---------- builtin/merge.c | 3 ++- builtin/mv.c | 5 +++++ builtin/rebase.c | 5 ++--- builtin/submodule--helper.c | 6 ++++-- convert.c | 2 ++ diffcore-rename.c | 10 +++++++--- environment.c | 7 +++---- fmt-merge-msg.c | 6 ++++-- read-cache.c | 1 + ref-filter.c | 8 ++++++-- reset.c | 4 ++-- 12 files changed, 42 insertions(+), 29 deletions(-)