Re: [PATCH] diff-lib: clear `pending` object-array in `index_differs_from()`

From: Martin Ågren <martin.agren@gmail.com>
Date: Sep 21, 2017, 03:56 UTC
Message-ID: <CAN0heSqVxBreaZN9x1XtX2OpeAoVZNzixr+vzyCWwe8UKtdtgQ@mail.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 7 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.

Ah. I grepped for "pending.objects", but didn't go more general than that.

> 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().

I'll look into that one in the evening.

> As I suspect you're working your way through leak-checker results, I'm
> OK if you want to punt on digging into more cases for now and just fix
> ones that the tool has identified as real leaks.

I am indeed going through ASan results. Your UNLEAK helps immensely! (I'm collecting UNLEAKs on the side. I see now that UNLEAK is in master, so I should probably submit those where I believe things are "UNLEAK-complete".)

Thanks for looking at this.

Martin