diff mbox series

merge-tree: fix segmentation fault in read-only repositories

Message ID pull.1362.git.1663774248660.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series merge-tree: fix segmentation fault in read-only repositories | expand

Commit Message

Johannes Schindelin Sept. 21, 2022, 3:30 p.m. UTC 
From: Johannes Schindelin <johannes.schindelin@gmx.de>

Independent of the question whether we want `git merge-tree` to report a
tree name even when it failed to write the tree objects in a read-only
repository, there is no question that we should avoid a segmentation
fault.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
    merge-tree: fix segmentation fault in read-only repositories
    
    Turns out that the segmentation fault reported by Taylor
    [https://lore.kernel.org/git/YyopQD+LvPucnz3w@nand.local/] happened
    while testing merge-ort in a read-only repository, and that the upstream
    version of git merge-tree is as affected as GitHub's internal version.
    
    Note: I briefly considered using the OID of the_hash_algo->empty_tree
    instead of null_oid() when no tree object could be constructed. However,
    I have come to the conclusion that this could potentially cause its own
    set of problems because it would relate to a valid tree object even if
    we do not have any valid tree object to play with.
    
    Also note: The question I hinted at in the commit message, namely
    whether or not to try harder to construct a tree object even if we
    cannot write it out, maybe merits a longer discussion, one that I think
    we should have after v2.38.0 is released, so as not to distract from
    focusing on v2.38.0.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1362%2Fdscho%2Fmerge-tree-in-read-only-repos-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1362/dscho/merge-tree-in-read-only-repos-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1362

 builtin/merge-tree.c             | 4 +++-
 t/t4301-merge-tree-write-tree.sh | 8 ++++++++
 2 files changed, 11 insertions(+), 1 deletion(-)


base-commit: dda7228a83e2e9ff584bf6adbf55910565b41e14

Comments

Taylor Blau Sept. 21, 2022, 3:42 p.m. UTC | #1 
On Wed, Sep 21, 2022 at 03:30:48PM +0000, Johannes Schindelin via GitGitGadget wrote:
> diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
> index ae5782917b9..25c7142a882 100644
> --- a/builtin/merge-tree.c
> +++ b/builtin/merge-tree.c
> @@ -412,6 +412,7 @@ static int real_merge(struct merge_tree_options *o,
>  	struct commit_list *merge_bases = NULL;
>  	struct merge_options opt;
>  	struct merge_result result = { 0 };
> +	const struct object_id *tree_oid;
>
>  	parent1 = get_merge_parent(branch1);
>  	if (!parent1)
> @@ -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);

My understanding is that we can get a clean result from
merge_incore_recursive(), but still have failed to write the physical
tree object out?

In other words, the two sides *could* have been merged, but the actual
result of doing that merge couldn't be written to disk (e.g., because
the repository is read-only or some such)?

If so, then this approach makes sense, and I agree with your idea to
use the all-zeros OID instead of the empty tree one. It would be
nice(r?) if we could just abort this command earlier, since `merge-tree`
promises that we'll write the result out as an object. So I don't think
a non-zero exit before we have to print the resulting tree object would
be unexpected.

But I don't have a strong feeling about it either way. So, if you want
to proceed here and just emit the all-zeros OID, I think that is a fine
approach.

> diff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh
> index 28ca5c38bb5..e56b1ba6e50 100755
> --- a/t/t4301-merge-tree-write-tree.sh
> +++ b/t/t4301-merge-tree-write-tree.sh
> @@ -810,4 +810,12 @@ test_expect_success 'can override merge of unrelated histories' '
>  	test_cmp expect actual
>  '
>
> +test_expect_success 'merge-ort fails gracefully in a read-only repository' '
> +	git init --bare read-only &&
> +	git push read-only side1 side2 &&
> +	test_when_finished "chmod -R u+w read-only" &&

Do we care about keeping this read-only repository around after the
test is done? It seems odd to have a directory called "read-only" be
user-writable. I'd probably just as soon replace this with:

  test_when_finished "rm -fr read-only" &&

Otherwise, this patch looks good. Thanks for working on it!

Thanks,
Taylor
Junio C Hamano Sept. 21, 2022, 6:12 p.m. UTC | #2 
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> +test_expect_success 'merge-ort fails gracefully in a read-only repository' '
> +	git init --bare read-only &&
> +	git push read-only side1 side2 &&
> +	test_when_finished "chmod -R u+w read-only" &&
> +	chmod -R a-w read-only &&
> +	test_must_fail git -C read-only merge-tree side1 side2
> +'

Doesn't this test need a prerequisite to guard against those whose
filesystem lacks SANITY?
Johannes Schindelin Sept. 21, 2022, 8:12 p.m. UTC | #3 
Hi Taylor,

On Wed, 21 Sep 2022, Taylor Blau wrote:

> On Wed, Sep 21, 2022 at 03:30:48PM +0000, Johannes Schindelin via GitGitGadget wrote:
> > diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
> > index ae5782917b9..25c7142a882 100644
> > --- a/builtin/merge-tree.c
> > +++ b/builtin/merge-tree.c
> > @@ -412,6 +412,7 @@ static int real_merge(struct merge_tree_options *o,
> >  	struct commit_list *merge_bases = NULL;
> >  	struct merge_options opt;
> >  	struct merge_result result = { 0 };
> > +	const struct object_id *tree_oid;
> >
> >  	parent1 = get_merge_parent(branch1);
> >  	if (!parent1)
> > @@ -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);
>
> My understanding is that we can get a clean result from
> merge_incore_recursive(), but still have failed to write the physical
> tree object out?

No, we do not get a clean result _because_ we have failed to write the
tree object.

> In other words, the two sides *could* have been merged, but the actual
> result of doing that merge couldn't be written to disk (e.g., because
> the repository is read-only or some such)?

Since we could not write the tree object, we cannot say whether the
overall merge could have been merged or not.

All we can say is whether the merge has been clean _so far_ or not. Which
is not very helpful.

Now, taking that thought a bit further, I did get suspicious that my patch
still contained a bug, and lo and behold, this bug really was there: when
the merge _has_ been clean so far, we report success (via exit code 0),
even if the tree could not be written, and therefore the `git merge-tree`
command should have failed!

I fixed the bug in the upcoming v2 and extended the test case to verify
that this bug no longer exists.

> If so, then this approach makes sense, and I agree with your idea to
> use the all-zeros OID instead of the empty tree one. It would be
> nice(r?) if we could just abort this command earlier, since `merge-tree`
> promises that we'll write the result out as an object. So I don't think
> a non-zero exit before we have to print the resulting tree object would
> be unexpected.

The idea of aborting the command earlier might make sense in isolation,
and when you're familiar with Git's built-ins taking the easy `die()` way
out upon failure without having to bother to release resources.

However, when you keep in mind that the `merge-tree` command is far from
its full potential and that really interesting ideas are still to be
implemented, some even well on their way like batched merges (see
https://github.com/gitgitgadget/git/pull/1361), it starts to become a much
better idea to keep the code as libified as possible, without any aborts.
Such an abort in the middle of the code path would interfere horribly with
the idea of batched merges.

> But I don't have a strong feeling about it either way. So, if you want
> to proceed here and just emit the all-zeros OID, I think that is a fine
> approach.
>
> > diff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh
> > index 28ca5c38bb5..e56b1ba6e50 100755
> > --- a/t/t4301-merge-tree-write-tree.sh
> > +++ b/t/t4301-merge-tree-write-tree.sh
> > @@ -810,4 +810,12 @@ test_expect_success 'can override merge of unrelated histories' '
> >  	test_cmp expect actual
> >  '
> >
> > +test_expect_success 'merge-ort fails gracefully in a read-only repository' '
> > +	git init --bare read-only &&
> > +	git push read-only side1 side2 &&
> > +	test_when_finished "chmod -R u+w read-only" &&
>
> Do we care about keeping this read-only repository around after the
> test is done? It seems odd to have a directory called "read-only" be
> user-writable. I'd probably just as soon replace this with:
>
>   test_when_finished "rm -fr read-only" &&

Speaking from way more experience debugging CI failures than I care to
have, it makes a lot of sense to keep this directory until the complete
test script finishes with utter success. Because if the test case fails,
and the CI tars up the "trash" directory, and the crucial files to
diagnose what might have gone wrong are missing from that tar file, my own
future life will be a lot harder than I like.

Ciao,
Dscho
Johannes Schindelin Sept. 21, 2022, 8:13 p.m. UTC | #4 
Hi Junio,

On Wed, 21 Sep 2022, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > +test_expect_success 'merge-ort fails gracefully in a read-only repository' '
> > +	git init --bare read-only &&
> > +	git push read-only side1 side2 &&
> > +	test_when_finished "chmod -R u+w read-only" &&
> > +	chmod -R a-w read-only &&
> > +	test_must_fail git -C read-only merge-tree side1 side2
> > +'
>
> Doesn't this test need a prerequisite to guard against those whose
> filesystem lacks SANITY?

Yes, of course!

Thank you for the sanity check (pun intended!),
Dscho
diff mbox series

Patch

diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
index ae5782917b9..25c7142a882 100644
--- a/builtin/merge-tree.c
+++ b/builtin/merge-tree.c
@@ -412,6 +412,7 @@  static int real_merge(struct merge_tree_options *o,
 	struct commit_list *merge_bases = NULL;
 	struct merge_options opt;
 	struct merge_result result = { 0 };
+	const struct object_id *tree_oid;
 
 	parent1 = get_merge_parent(branch1);
 	if (!parent1)
@@ -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);
 	if (!result.clean) {
 		struct string_list conflicted_files = STRING_LIST_INIT_NODUP;
 		const char *last = NULL;
diff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh
index 28ca5c38bb5..e56b1ba6e50 100755
--- a/t/t4301-merge-tree-write-tree.sh
+++ b/t/t4301-merge-tree-write-tree.sh
@@ -810,4 +810,12 @@  test_expect_success 'can override merge of unrelated histories' '
 	test_cmp expect actual
 '
 
+test_expect_success 'merge-ort fails gracefully in a read-only repository' '
+	git init --bare read-only &&
+	git push read-only side1 side2 &&
+	test_when_finished "chmod -R u+w read-only" &&
+	chmod -R a-w read-only &&
+	test_must_fail git -C read-only merge-tree side1 side2
+'
+
 test_done