Re: [GSoC][PATCH] merge: use reverse_commit_list() for list reversal
From: Elijah Newren <hidden>
Date: 2023-02-02 23:47:16
On Thu, Feb 2, 2023 at 9:10 AM Kousik Sanagavarapu [off-list ref] wrote:
Instead of manually doing an in-place list reversal, use the helper function reverse_commit_list(), hence improving code readability. Signed-off-by: Kousik Sanagavarapu <redacted> --- This patch addresses the issue #1156(Use reverse_commit_list() in more places) on gitgitgadget. I also would like to submit this patch as the microproject for GSoC 2023.
Looks like this was a suggestion from me, so I'm to blame. But looking at it again, I'm not sure builtin/merge.c is actually a good candidate because...
quoted hunk ↗ jump to hunk
builtin/merge.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)diff --git a/builtin/merge.c b/builtin/merge.c index 5900b81729..4503dbfeb3 100644 --- a/builtin/merge.c +++ b/builtin/merge.c@@ -736,7 +736,6 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common, struct commit *result; struct commit_list *reversed = NULL; struct merge_options o; - struct commit_list *j; if (remoteheads->next) { error(_("Not handling anything other than two heads merge."));@@ -757,8 +756,7 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common, o.branch1 = head_arg; o.branch2 = merge_remote_util(remoteheads->item)->name; - for (j = common; j; j = j->next) - commit_list_insert(j->item, &reversed);
This is not an in-place list reversal; it's creating a new list that is reversed. "common" can continue to be used separately from "reversed" after this point. (And the new list, "reversed" is consumed by merge_recursive()/merge_ort_recursive(), so there's nothing to free.)
+ reversed = reverse_commit_list(common);
This is an in-place list reversal. If there's only one merge strategy being attempted, this may work, since "common" probably won't be used afterwards. However, if we have multiple merge strategies that expect to be passed the common commits -- a situation we probably don't have a testcase for in the testsuite -- then I think this code change will result in a use-after-free error.
hold_locked_index(&lock, LOCK_DIE_ON_ERROR);
if (!strcmp(strategy, "ort"))
--
2.25.1