Re: [PATCH v3 1/9] show, log: provide a --remerge-diff capability

[Elijah Newren <newren@gmail.com>]

On Wed, Jan 19, 2022 at 7:53 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
> On Thu, Dec 30 2021, Elijah Newren via GitGitGadget wrote:
>
> > From: Elijah Newren <newren@gmail.com>
>
> > +static int do_remerge_diff(struct rev_info *opt,
> > +                        struct commit_list *parents,
> > +                        struct object_id *oid,
> > +                        struct commit *commit)
> > +{
> > +     struct merge_options o;
> > +     struct commit_list *bases;
> > +     struct merge_result res = {0};
> > +     struct pretty_print_context ctx = {0};
> > +     struct commit *parent1 = parents->item;
> > +     struct commit *parent2 = parents->next->item;
> > +     struct strbuf parent1_desc = STRBUF_INIT;
> > +     struct strbuf parent2_desc = STRBUF_INIT;
> > +
> > +     /* Setup merge options */
> > +     init_merge_options(&o, the_repository);
> > +     o.show_rename_progress = 0;
> > +
> > +     ctx.abbrev = DEFAULT_ABBREV;
> > +     format_commit_message(parent1, "%h (%s)", &parent1_desc, &ctx);
> > +     format_commit_message(parent2, "%h (%s)", &parent2_desc, &ctx);
> > +     o.branch1 = parent1_desc.buf;
> > +     o.branch2 = parent2_desc.buf;
> > +
> > +     /* Parse the relevant commits and get the merge bases */
> > +     parse_commit_or_die(parent1);
> > +     parse_commit_or_die(parent2);
> > +     bases = get_merge_bases(parent1, parent2);
>
> There's existing leaks all over the place here unrelated to this new
> code, so this is no big deal, but I noticed that get_merge_bases() here
> leaks.

Interesting.

> Shouldn't it call free_commit_list() like e.g. diff_get_merge_base()
> which invokes get_merge_bases() does on the return value?

See the comment describing merge_incore_recursive() in merge-ort.h,
particularly this part:

* merge_bases will be consumed (emptied) so make a copy if you need it.

So free_commit_list() seems like it'd lead to a double free or use-after-free.

However, looking at merge_ort_internal() it looks like there is a bug
in its consumption of the merge bases (which I copied from
merge_recursive; oops).  It pops the first one off the commit list,
but then merely iterates through the remainder of the list without
popping.  So, if there's only one merge base, it'll consume it and the
code will look leak free (which must have been the cases I was looking
at when I was doing leak testing).  But in recursive cases, it leaks
the second and later ones.

Since the caller still has a pointer referring to the first (already
free'd) commit, I think that if they attempt to use it then it would
probably cause a use-after-free.


So, yes, I think there's a leak, but it's not due to this patch.  It's
one that has been around since...the introduction of merge-recursive
(though it originally computed the merge bases internally rather than
allowing them to be passed in).  So, it's been around for quite a
while.

I'll look into it, and see if I can come up with a fix, but it doesn't
really belong in this series.  I'll submit it separately.

Thanks for the report.

> > +test_description='remerge-diff handling'
> > +
> > +. ./test-lib.sh
> > +
> > +test_expect_success 'setup basic merges' '
> > +     test_write_lines 1 2 3 4 5 6 7 8 9 >numbers &&
> > +     git add numbers &&
> > +     git commit -m base &&
> > +
> > +     git branch feature_a &&
> > +     git branch feature_b &&
> > +     git branch feature_c &&
> > +
> > +     git branch ab_resolution &&
> > +     git branch bc_resolution &&
> > +
> > +     git checkout feature_a &&
> > +     test_write_lines 1 2 three 4 5 6 7 eight 9 >numbers &&
> > +     git commit -a -m change_a &&
> > +
> > +     git checkout feature_b &&
> > +     test_write_lines 1 2 tres 4 5 6 7 8 9 >numbers &&
> > +     git commit -a -m change_b &&
> > +
> > +     git checkout feature_c &&
> > +     test_write_lines 1 2 3 4 5 6 7 8 9 10 >numbers &&
> > +     git commit -a -m change_c &&
> > +
> > +     git checkout bc_resolution &&
> > +     git merge --ff-only feature_b &&
> > +     # no conflict
> > +     git merge feature_c &&
> > +
> > +     git checkout ab_resolution &&
> > +     git merge --ff-only feature_a &&
> > +     # conflicts!
> > +     test_must_fail git merge feature_b &&
> > +     # Resolve conflict...and make another change elsewhere
> > +     test_write_lines 1 2 drei 4 5 6 7 acht 9 >numbers &&
> > +     git add numbers &&
>
> Just a matter of taste, but FWIW some of the custom
> test_write_lines/commit here could nowadays use test_commit with
> --printf: 47c88d16ba6 (test-lib functions: add --printf option to
> test_commit, 2021-05-10)
>
> I don't think it's worth the churn to change it here, just an FYI.

Good to know; thanks for the heads up.

Note, though, and this has nothing to do with your patches, but I'm
not sure I'll ever use this particular feature since I don't much care
for test_commit except in trivial cases.  Others have recommended the
function to me before, but my attempts to use it have cost me far more
time than it has saved due to its quirks not working well with the
merges I have attempted to setup.  Beyond the fact that its
documentation is a lie and the filename defaults to <message>.t, one
also has to memorize the order of three positional arguments and add a
smattering of additional flags (--printf, --append, --no-tag) and add
a bunch of newline directives to get things right.  The function can
be useful and nice on non-merge tests (e.g. when you can pass it just
one positional argument) and I'm happy to use it there, but for
merge-related tests it's a needless time sink where the best you can
hope for after fighting it is getting code that is overall _less_
readable than what you started with.