[PATCH v2 0/6] reroll ma/plugleaks; more `object_array`-fixes

From: Martin Ågren <martin.agren@gmail.com>
Date: Sep 22, 2017, 23:34 UTC
Message-ID: <cover.1506120291.git.martin.agren@gmail.com>
In-Reply-To: <20170920200229.bc4yniz6otng2zyz@sigill.intra.peff.net>

On 20 September 2017 at 22:02, Jeff King <peff@peff.net> wrote:

Show 16 quoted lines

> 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