On 20 September 2017 at 22:02, Jeff King <peff@xxxxxxxx> wrote: > On Wed, Sep 20, 2017 at 09:47:24PM +0200, Martin Ågren wrote: > >> Instead of conditionally freeing `rev.pending.objects`, just call >> `object_array_clear()` on `rev.pending`. This means we don't poke as >> much into the implementation, which is already a good thing, but also >> that we free the individual entries as well, thereby fixing a >> memory-leak. > > Looks good. A similar bug was the exact reason for adding the function > in 46be82312. I did a grep for 'free.*\.objects' to see if there were > other cases. > > There are some hits. E.g., the one in orphaned_commit_warning(). It does > something funny with setting revs.leak_pending. But I _think_ after the > whole thing is done and we call free(refs.objects), that probably ought > to be an object_array_clear(). Looking into this, I found various classes of leaks and oddities. I ended up replacing this patch with four patches. Since Junio collected my "independent" patches into ma/leakplugs, this is a re-roll of that whole topic. I got the first two patches from Junio's tree. The only difference there is "builtin/" at the very start of the first commit message. The third patch handles the places where we use `leak_pending`. The fourth addresses the rest of what your grep found and includes the original patch from this thread. While working on those, I felt that some callers fiddle a bit too much with the internals of `object_array`. One pattern leaks, another only looks like it. That resulted in the last two patches. Since ma/plugleaks is branched off maint, I did the same with this series. It applies on master and next and has a minor conflict on pu (`handle_commit()` got a new argument). Martin Martin Ågren (6): builtin/commit: fix memory leak in `prepare_index()` commit: fix memory leak in `reduce_heads()` leak_pending: use `object_array_clear()`, not `free()` object_array: use `object_array_clear()`, not `free()` object_array: add and use `object_array_pop()` pack-bitmap[-write]: use `object_array_clear()`, don't leak bisect.c | 3 ++- builtin/checkout.c | 9 ++++++++- builtin/commit.c | 15 ++++++++++----- builtin/fast-export.c | 3 +-- builtin/fsck.c | 7 +------ builtin/reflog.c | 6 +++--- bundle.c | 9 ++++++++- commit.c | 1 + diff-lib.c | 3 +-- object.c | 13 +++++++++++++ object.h | 7 +++++++ pack-bitmap-write.c | 4 +--- pack-bitmap.c | 10 +++------- revision.h | 11 +++++++++++ shallow.c | 2 +- submodule.c | 4 ++-- upload-pack.c | 2 +- 17 files changed, 74 insertions(+), 35 deletions(-) -- 2.14.1.727.g9ddaf86
