threads / patch / 53714

patchcommit-reach: plug minor memory leak after using is_descendant_of()

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

## tl;dr

3 messages between Jun 19, 2020 and Jun 19, 2020. Diffs are folded; open one to read it.

replies: 2people: 3as markdown or json

René Scharfe· Jun 19, 2020, 13:13 UTC · lore

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 <l.s.r@web.de>
---
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(-)

Show changes to commit-reach.c +4 −2

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· Jun 19, 2020, 13:31 UTC · re: René Scharfe · lore

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

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

Derrick Stolee· Jun 19, 2020, 14:07 UTC · re: Jeff King · lore

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

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