Re: [PATCH] commit-reach: plug minor memory leak after using is_descendant_of()

From: Jeff King <peff@peff.net>
Date: Jun 19, 2020, 13:31 UTC
Message-ID: <20200619133159.GB2540897@coredump.intra.peff.net>
In-Reply-To: <ada13c16-d964-c6ee-80ac-626edbc5f52d@web.de>

On Fri, Jun 19, 2020 at 03:13:46PM +0200, René Scharfe wrote:

> ref_newer() builds a commit_list to pass a single potential ancestor to
> is_descendant_of().  The latter leaves the list intact.  Release the
> allocated memory after the call.

Looks obviously correct.

Show 7 quoted lines

> ---
> We could allocate the commit_list on the stack, which would simplify such
> glue code quite a bit.  That would be dangerous in case is_descendant_of()
> or some other function that is handed such a list tries to consume/free()
> it.  How can we be tell a function is safe to be given a stack-allocated
> list?  Perhaps by marking its argument as const.  Or by converting all
> functions to arrays.

Yeah, if we're not worried about the performance implications of the extra allocation, I think it's better to err on the side of safety.

I do agree that if we consistently passed an array (and length), some of these functions would get less awkward. I tried a few years ago to convert many of the commit_list uses to arrays, but it was a bit of a yak shave, since often they get lists from callers, who get it from rev_info, etc. And some of those callers _do_ like having lists, because they want to do O(1) splicing, etc.

-Peff