{"thread":{"id":"53714","subject":"[PATCH] commit-reach: plug minor memory leak after using is_descendant_of()","startedAt":"2020-06-19T13:14:10Z","lastAt":"2020-06-19T14:07:18Z","messageCount":3,"participants":["René Scharfe","Jeff King","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"400112","messageId":"ada13c16-d964-c6ee-80ac-626edbc5f52d@web.de","threadId":"53714","inReplyTo":null,"subject":"[PATCH] commit-reach: plug minor memory leak after using is_descendant_of()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-06-19T13:13:46Z","receivedAt":"2020-06-19T13:14:10Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"ref_newer() builds a commit_list to pass a single potential ancestor to\nis_descendant_of().  The latter leaves the list intact.  Release the\nallocated memory after the call.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\nWe could allocate the commit_list on the stack, which would simplify such\nglue code quite a bit.  That would be dangerous in case is_descendant_of()\nor some other function that is handed such a list tries to consume/free()\nit.  How can we be tell a function is safe to be given a stack-allocated\nlist?  Perhaps by marking its argument as const.  Or by converting all\nfunctions to arrays.\n\n commit-reach.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/commit-reach.c b/commit-reach.c\nindex 4ca7e706a1..6bba16e7b5 100644\n--- a/commit-reach.c\n+++ b/commit-reach.c\n@@ -396,6 +396,7 @@ int ref_newer(const struct object_id *new_oid, const struct object_id *old_oid)\n \tstruct object *o;\n \tstruct commit *old_commit, *new_commit;\n \tstruct commit_list *old_commit_list = NULL;\n+\tint ret;\n\n \t/*\n \t * Both new_commit and old_commit must be commit-ish and new_commit is descendant of\n@@ -417,7 +418,9 @@ int ref_newer(const struct object_id *new_oid, const struct object_id *old_oid)\n \t\treturn 0;\n\n \tcommit_list_insert(old_commit, &old_commit_list);\n-\treturn is_descendant_of(new_commit, old_commit_list);\n+\tret = is_descendant_of(new_commit, old_commit_list);\n+\tfree_commit_list(old_commit_list);\n+\treturn ret;\n }\n\n /*\n--\n2.27.0\n"},{"id":"400119","messageId":"20200619133159.GB2540897@coredump.intra.peff.net","threadId":"53714","inReplyTo":"ada13c16-d964-c6ee-80ac-626edbc5f52d@web.de","subject":"Re: [PATCH] commit-reach: plug minor memory leak after using is_descendant_of()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-06-19T13:31:59Z","receivedAt":"2020-06-19T13:32:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 19, 2020 at 03:13:46PM +0200, René Scharfe wrote:\n\n> ref_newer() builds a commit_list to pass a single potential ancestor to\n> is_descendant_of().  The latter leaves the list intact.  Release the\n> allocated memory after the call.\n\nLooks obviously correct.\n\n> ---\n> We could allocate the commit_list on the stack, which would simplify such\n> glue code quite a bit.  That would be dangerous in case is_descendant_of()\n> or some other function that is handed such a list tries to consume/free()\n> it.  How can we be tell a function is safe to be given a stack-allocated\n> list?  Perhaps by marking its argument as const.  Or by converting all\n> functions to arrays.\n\nYeah, if we're not worried about the performance implications of the\nextra allocation, I think it's better to err on the side of safety.\n\nI do agree that if we consistently passed an array (and length), some of\nthese functions would get less awkward. I tried a few years ago to\nconvert many of the commit_list uses to arrays, but it was a bit of a\nyak shave, since often they get lists from callers, who get it from\nrev_info, etc. And some of those callers _do_ like having lists, because\nthey want to do O(1) splicing, etc.\n\n-Peff\n"},{"id":"400124","messageId":"a7621cce-b2f9-d4ce-2fca-85e23cf185c0@gmail.com","threadId":"53714","inReplyTo":"20200619133159.GB2540897@coredump.intra.peff.net","subject":"Re: [PATCH] commit-reach: plug minor memory leak after using is_descendant_of()","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-06-19T14:07:14Z","receivedAt":"2020-06-19T14:07:18Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/19/2020 9:31 AM, Jeff King wrote:\n> On Fri, Jun 19, 2020 at 03:13:46PM +0200, René Scharfe wrote:\n> \n>> ref_newer() builds a commit_list to pass a single potential ancestor to\n>> is_descendant_of().  The latter leaves the list intact.  Release the\n>> allocated memory after the call.\n> \n> Looks obviously correct.\n\nI concur, thanks!\n\n>> ---\n>> We could allocate the commit_list on the stack, which would simplify such\n>> glue code quite a bit.  That would be dangerous in case is_descendant_of()\n>> or some other function that is handed such a list tries to consume/free()\n>> it.  How can we be tell a function is safe to be given a stack-allocated\n>> list?  Perhaps by marking its argument as const.  Or by converting all\n>> functions to arrays.\n> \n> Yeah, if we're not worried about the performance implications of the\n> extra allocation, I think it's better to err on the side of safety.\n> \n> I do agree that if we consistently passed an array (and length), some of\n> these functions would get less awkward. I tried a few years ago to\n> convert many of the commit_list uses to arrays, but it was a bit of a\n> yak shave, since often they get lists from callers, who get it from\n> rev_info, etc. And some of those callers _do_ like having lists, because\n> they want to do O(1) splicing, etc.\n\nYeah, this constant parameter-conversion between the different\nmethods in commit-reach.c is certainly unfortunate, but persists\ndue to historical reasons that Peff mentions.\n\nThanks,\n-Stolee\n\n"}]}