On Fri, Aug 18, 2023 at 2:41 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> "Kevin Backhouse via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
>
> > From: Kevin Backhouse <kevinbackhouse@xxxxxxxxxx>
> >
> > To reproduce (with an ASAN build):
> >
> > ```
> > mkdir test
> > cd test
> > git init
> > echo x > x.txt
> > git add .
> > git commit -m "WIP"
> > git checkout -b dev
> > echo y > x.txt
> > git add .
> > git commit -m "WIP"
> > git checkout main
> > echo z > x.txt
> > git add .
> > git commit -m "WIP"
> > echo a > x.txt
> > git add .
> > git merge dev
> > ```
>
> We'd rather not to see the above in the proposed log message; can't
> we add (a variation of) it to our test suite?
>
> > The fix is to call free_commit_list(merge_bases) when an error occurs.
>
> We usually have the description of what the problem is and give an
> analysis on why/how it happens, before presenting a solution. Write
> it more like:
>
> The caller of merge_ort_recursive() expects the commit list
> passed in as the merge_bases parameter to be fully consumed by
> the function and does not free it when the function returns. In
> normal cases, the commit list does get consumed, but when the
> function returns early upon encountering an error, it forgets to
> clean it up.
>
> Fix this by freeing the list in the code paths for error returns.
>
> > merge-ort-wrappers.c | 4 +++-
> > merge-ort.c | 4 +++-
>
> These two places and their fixes seem OK, but I have to wonder if
> these are complete fixes.
>
> > diff --git a/merge-ort-wrappers.c b/merge-ort-wrappers.c
> > index 4acedf3c338..aeb56c9970c 100644
> > --- a/merge-ort-wrappers.c
> > +++ b/merge-ort-wrappers.c
> > @@ -54,8 +54,10 @@ int merge_ort_recursive(struct merge_options *opt,
> > struct tree *head = repo_get_commit_tree(opt->repo, side1);
> > struct merge_result tmp;
> >
> > - if (unclean(opt, head))
> > + if (unclean(opt, head)) {
> > + free_commit_list(merge_bases);
> > return -1;
> > + }
> >
> > memset(&tmp, 0, sizeof(tmp));
> > merge_incore_recursive(opt, merge_bases, side1, side2, &tmp);
>
> The function before this hunk appears to have very similar code
> structure. Does it need the same fix, or if not why not?
>
> > diff --git a/merge-ort.c b/merge-ort.c
> > index 8631c997002..a0eb91fb011 100644
> > --- a/merge-ort.c
> > +++ b/merge-ort.c
> > @@ -5070,8 +5070,10 @@ static void merge_ort_internal(struct merge_options *opt,
> > opt->branch1 = "Temporary merge branch 1";
> > opt->branch2 = "Temporary merge branch 2";
> > merge_ort_internal(opt, NULL, prev, next, result);
> > - if (result->clean < 0)
> > + if (result->clean < 0) {
> > + free_commit_list(merge_bases);
> > return;
> > + }
>
> Before this function, there is a comment that this came from another
> function and it seems to still have a very similar code structure.
> Does the other function need the same fix, or if not why not?
The other function would need a more involved fix, which would
basically involve porting a59b8dd94f (merge-ort: fix memory leak in
merge_ort_internal(), 2022-01-20) to merge-recursive as a preparatory
step. This particular cleanup cannot be ported in its current form to
merge-recursive.c until then.
