Re: [PATCH] commit-reach: plug minor memory leak after using is_descendant_of()
- From
Derrick Stolee <stolee@gmail.com>
- Date
- Jun 19, 2020, 14:07 UTC
- Message-ID
- <a7621cce-b2f9-d4ce-2fca-85e23cf185c0@gmail.com>
- In-Reply-To
- <20200619133159.GB2540897@coredump.intra.peff.net>
On 6/19/2020 9:31 AM, Jeff King wrote:
Show 7 quoted lines
> 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.
I concur, thanks!
Show 17 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.
Yeah, this constant parameter-conversion between the different methods in commit-reach.c is certainly unfortunate, but persists due to historical reasons that Peff mentions.
Thanks, -Stolee