Re: [PATCH v2] merge-tree: fix segmentation fault in read-only repositories

From: Elijah Newren <newren@gmail.com>
Date: Sep 23, 2022, 01:47 UTC
Message-ID: <CABPp-BEW9RdH8RSFk2BtPwYSyP0mKTz-7oCQQP7jsR6ChmF3aQ@mail.gmail.com>
In-Reply-To: <n920r07n-8443-2r5o-09pr-q20r99q6o12o@tzk.qr>

On Thu, Sep 22, 2022 at 12:50 PM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 15 quoted lines

>
> Hi Elijah,
>
> now that I have studied more of the code, hunted down Coverity reports
> about `merge-ort.c` and `builtin/merge-tree.c` and essentially have grown
> a lot more confidence about my patch, I'll take the time to respond in a
> bit more detail.
>
> On Wed, 21 Sep 2022, Elijah Newren wrote:
>
> > On Wed, Sep 21, 2022 at 3:08 PM Johannes Schindelin via GitGitGadget
> > <gitgitgadget@gmail.com> wrote:
> > >
> > > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> > >

[...]

Show 16 quoted lines

> > > @@ -446,7 +447,8 @@ static int real_merge(struct merge_tree_options *o,
> > >         if (o->show_messages == -1)
> > >                 o->show_messages = !result.clean;
> > >
> > > -       printf("%s%c", oid_to_hex(&result.tree->object.oid), line_termination);
> > > +       tree_oid = result.tree ? &result.tree->object.oid : null_oid();
> > > +       printf("%s%c", oid_to_hex(tree_oid), line_termination);
> >
> > Perhaps also print a warning to stderr when result.tree is NULL?
>
> I ended up not even touching `builtin/merge-tree.c` but instead ensuring
> that `result.clean` is negative if we fail to write an object (which could
> happen even in read/write repositories, think "disk full").
>
> As a consequence, we do not even reach this `printf()` anymore, as you
> pointed out, a negative `result.clean` is handled much earlier.

Works for me.  :-)

> It is handled via a `die()` in `real_merge()`, and that will need to
> change, I think, if we want to continue on the batched merges.

Indeed, good point.

Show 40 quoted lines

> > >         if (!result.clean) {
> > >                 struct string_list conflicted_files = STRING_LIST_INIT_NODUP;
> > >                 const char *last = NULL;
> > > @@ -473,7 +475,7 @@ static int real_merge(struct merge_tree_options *o,
> > >                                               &result);
> > >         }
> > >         merge_finalize(&opt, &result);
> > > -       return !result.clean; /* result.clean < 0 handled above */
> > > +       return !result.tree || !result.clean; /* result.clean < 0 handled above */
> >
> > Thinking out loud, should this logic be at the merge-ort.c level,
> > perhaps something like this:
> >
> > @@ -4940,6 +4941,9 @@ static void
> > merge_ort_nonrecursive_internal(struct merge_options *opt,
> >         result->tree = parse_tree_indirect(&working_tree_oid);
> >         /* existence of conflicted entries implies unclean */
> >         result->clean &= strmap_empty(&opt->priv->conflicted);
> > +       if (!result->tree)
> > +               /* This shouldn't happen, because if we did fail to
> > write a tree we should have returned early before getting here.  But
> > just in case we have some bugs... */
> > +               result->clean = -1;
> >         if (!opt->priv->call_depth) {
> >                 result->priv = opt->priv;
> >                 result->_properly_initialized = RESULT_INITIALIZED;
> >
> > That might benefit callers other than merge-tree, though maybe it
> > makes it harder to print a helpful error message (unless we're fine
> > with the library always throwing one?)
>
> The error messages are already thrown about (this is how it looks like
> with v3 of this patch):
>
>         [...]
>         + git -C read-only merge-tree side1 side2
>         error: insufficient permission for adding an object to repository database ./objects
>         error: error: Unable to add numbers to database
>         error: insufficient permission for adding an object to repository database ./objects
>         error: error: Unable to add greeting to database

Ah, this confirms my suspicions. These lines look like two failures, both for adding blob objects. This shows that the return value from the write_object_file() call within handle_content_merge() is not being correctly propagated upwards, and the merge is (erroneously) continuing on despite that.

>         error: insufficient permission for adding an object to repository database ./objects

And I'm guessing the lack of the "Unable to add %s to database" following message means this was the attempt to write a new tree.

>         fatal: failure to merge

And this shows that with your patch, you are propagating and catching the failure to write the tree object and aborting, so we at least catch failures to write trees now.

[...]

Show 15 quoted lines

> > And then, possibly post-v2.38.0 though we may be able to get it in
> > before, getting correct propagation of a -1 return value from the
> > source of the error would be good.  Would you like to look into that,
> > or would you prefer I did?
>
> It turned out not to be too bad.
>
> Essentially, to propagate `write_object_file()` failures I only had to
> change a couple of signatures (with the corresponding `return`s):
> `write_tree()`, `write_completed_directory()`, and `process_entries()`.
> And then, of course, I needed to change
> `merge_ort_nonrecursive_internal()` to respect the propagated error by
> setting `result->clean = -1`.
>
> Pretty straight-forward, really, much less involved than I had expected.

Nice!