# [PATCH] commit-reach: plug minor memory leak after using is_descendant_of() 3 messages from 2020-06-19 to 2020-06-19. Participants: René Scharfe, Jeff King, Derrick Stolee. Thread: https://gitlist.dev/t/53714 ## René Scharfe, 2020-06-19 13:13 Subject: [PATCH] commit-reach: plug minor memory leak after using is_descendant_of() Message-ID: URL: https://gitlist.dev/e/ada13c16-d964-c6ee-80ac-626edbc5f52d%40web.de ``` 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. Signed-off-by: René Scharfe --- 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. commit-reach.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/commit-reach.c b/commit-reach.c index 4ca7e706a1..6bba16e7b5 100644 --- a/commit-reach.c +++ b/commit-reach.c @@ -396,6 +396,7 @@ int ref_newer(const struct object_id *new_oid, const struct object_id *old_oid) struct object *o; struct commit *old_commit, *new_commit; struct commit_list *old_commit_list = NULL; + int ret; /* * Both new_commit and old_commit must be commit-ish and new_commit is descendant of @@ -417,7 +418,9 @@ int ref_newer(const struct object_id *new_oid, const struct object_id *old_oid) return 0; commit_list_insert(old_commit, &old_commit_list); - return is_descendant_of(new_commit, old_commit_list); + ret = is_descendant_of(new_commit, old_commit_list); + free_commit_list(old_commit_list); + return ret; } /* -- 2.27.0 ``` ## Jeff King, 2020-06-19 13:31 Subject: Re: [PATCH] commit-reach: plug minor memory leak after using is_descendant_of() Message-ID: <20200619133159.GB2540897@coredump.intra.peff.net> URL: https://gitlist.dev/e/20200619133159.GB2540897%40coredump.intra.peff.net In-Reply-To: ``` 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. > --- > 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 ``` ## Derrick Stolee, 2020-06-19 14:07 Subject: Re: [PATCH] commit-reach: plug minor memory leak after using is_descendant_of() Message-ID: URL: https://gitlist.dev/e/a7621cce-b2f9-d4ce-2fca-85e23cf185c0%40gmail.com In-Reply-To: <20200619133159.GB2540897@coredump.intra.peff.net> ``` On 6/19/2020 9:31 AM, Jeff King wrote: > 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! >> --- >> 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 ```