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

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(-)

On Wed, Sep 21, 2022 at 03:30:48PM +0000, Johannes Schindelin via GitGitGadget wrote:

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.

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

Hi Taylor,

On Wed, 21 Sep 2022, Taylor Blau wrote:

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

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.

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.

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 via GitGitGadget" <gitgitgadget@gmail.com> writes:

Doesn't this test need a prerequisite to guard against those whose filesystem lacks SANITY?

Hi Junio,

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

Yes, of course!

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

From: Johannes Schindelin <johannes.schindelin@gmx.de>

Independent of the question whether we want `git merge-tree` to report the 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.

And when we report an invalid tree name (because the tree could not be written), we need the exit status to be non-zero.

Let's make it so.

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.
    
    Changes since v1:
    
     * Using the SANITY prereq now
     * If the merge was aborted due to a write error, exit with a non-zero
       code even if the (potentially partial) merge is clean
     * The test case now also verifies that the git merge-tree command fails
       in a read-only repository even if the merge would have succeeded

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

Range-diff vs v1:

 1:  beba3fd9a95 ! 1:  1de4df3f471 merge-tree: fix segmentation fault in read-only repositories
     @@ Metadata
       ## Commit message ##
          merge-tree: fix segmentation fault in read-only repositories
      
     -    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.
     +    Independent of the question whether we want `git merge-tree` to report
     +    the 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.
     +
     +    And when we report an invalid tree name (because the tree could not be
     +    written), we need the exit status to be non-zero.
     +
     +    Let's make it so.
      
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
     @@ builtin/merge-tree.c: static int real_merge(struct merge_tree_options *o,
       	if (!result.clean) {
       		struct string_list conflicted_files = STRING_LIST_INIT_NODUP;
       		const char *last = NULL;
     +@@ builtin/merge-tree.c: 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 */
     + }
     + 
     + int cmd_merge_tree(int argc, const char **argv, const char *prefix)
      
       ## t/t4301-merge-tree-write-tree.sh ##
      @@ t/t4301-merge-tree-write-tree.sh: 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' '
     ++test_expect_success SANITY 'merge-ort fails gracefully in a read-only repository' '
      +	git init --bare read-only &&
     -+	git push read-only side1 side2 &&
     ++	git push read-only side1 side2 side3 &&
      +	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 side3 &&
      +	test_must_fail git -C read-only merge-tree side1 side2
      +'
      +

 builtin/merge-tree.c             | 6 ++++--
 t/t4301-merge-tree-write-tree.sh | 9 +++++++++
 2 files changed, 13 insertions(+), 2 deletions(-)

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:

As we are not using tree_oid anywhere else, it might be cleaner to switch on result.tree more explicitly, especially given that the return statement also uses result.tree to decide what exit status to use, e.g.

	if (result.tree)
		fputs(oid_to_hex(&result.tree->object.oid), stdout);
	else
		fputs(oid_to_hex(null_oid()), stdout);
	fputc(line_termination, stdout);
	
but that is merely "might be cleaner".

The original returned 1 when we failed to cleanly merge and 0 when we cleanly merged. Now, if we failed to come up with a merged tree, result.tree would be NULL and we return 1 from the function, because we failed to cleanly merge.

OK.  These negations make my head spin X-<.

Hi Junio,

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

I would call it "more readable", even, not just "cleaner" :-)

And here I thought that you couldn't possibly not like if I didn't refrain from not avoiding double negations...

Ciao, Dscho

On Wed, Sep 21, 2022 at 3:08 PM Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com> wrote:

Ah, a read-only repo. Looks like we didn't bother to check the return status of write_object_file() in one of the two cases; oops. (And it looks like the other case does not correctly propagate the error upwards far enough...)

That's fair, but you know I'm going to have a hard time refraining from commenting on it anyway... :-)

[...]

Perhaps also print a warning to stderr when result.tree is NULL?

Thinking out loud, should this logic be at the merge-ort.c level, perhaps something like this:

Elijah Newren <newren@gmail.com> writes:

True.

True, too.

Hi Elijah,

just quickly...

On Wed, 21 Sep 2022, Elijah Newren wrote:

You're right, of course. I have pushed up a tentative v3 that does it as you proposed, and it looks not half bad. I'll look a bit more deeply, still, but chances are that I'll submit this later today as v3 as-is.

And then I will reply a bit more verbosely, either ;-)

Ciao, Dscho

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:

Indeed, that was the actual root cause, but I had failed to even look whether the return values of `write_tree()` and `write_object_file()` were respected. Narrator's voice: they weren't.

Hahaha, of course you would ;-)

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.

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.

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
	error: insufficient permission for adding an object to repository database ./objects
	fatal: failure to merge
	+ exit_code=128
	[...]

You're absolutely right. That's what v3 now does.

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.

Thank you! Dscho

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

[...]

Works for me.  :-)

Indeed, good point.

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.

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.

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.

[...]

Nice!

From: Johannes Schindelin <johannes.schindelin@gmx.de>

If the blob/tree objects cannot be written, we really need the merge operations to fail, and not to continue (and then try to access the tree object which is however still set to `NULL`).

Let's stop ignoring the return value of `write_object_file()` and `write_tree()` and set `clean = -1` in the error case.

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.
    
    Changes since v2:
    
     * Completely changed the approach by no longer touching
       builtin/merge-tree.c but instead fixing the underlying merge-ort
       layer: we no longer ignore the return value of write_tree() and
       write_object_file().
    
    Changes since v1:
    
     * Using the SANITY prereq now
     * If the merge was aborted due to a write error, exit with a non-zero
       code even if the (potentially partial) merge is clean
     * The test case now also verifies that the git merge-tree command fails
       in a read-only repository even if the merge would have succeeded

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

Range-diff vs v2:

 1:  1de4df3f471 ! 1:  198ff455f90 merge-tree: fix segmentation fault in read-only repositories
     @@ Metadata
      Author: Johannes Schindelin <Johannes.Schindelin@gmx.de>
      
       ## Commit message ##
     -    merge-tree: fix segmentation fault in read-only repositories
     +    merge-ort: fix segmentation fault in read-only repositories
      
     -    Independent of the question whether we want `git merge-tree` to report
     -    the 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.
     +    If the blob/tree objects cannot be written, we really need the merge
     +    operations to fail, and not to continue (and then try to access the tree
     +    object which is however still set to `NULL`).
      
     -    And when we report an invalid tree name (because the tree could not be
     -    written), we need the exit status to be non-zero.
     -
     -    Let's make it so.
     +    Let's stop ignoring the return value of `write_object_file()` and
     +    `write_tree()` and set `clean = -1` in the error case.
      
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
     - ## builtin/merge-tree.c ##
     -@@ builtin/merge-tree.c: 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)
     -@@ builtin/merge-tree.c: 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;
     -@@ builtin/merge-tree.c: static int real_merge(struct merge_tree_options *o,
     - 					      &result);
     + ## merge-ort.c ##
     +@@ merge-ort.c: static int tree_entry_order(const void *a_, const void *b_)
     + 				 b->string, strlen(b->string), bmi->result.mode);
     + }
     + 
     +-static void write_tree(struct object_id *result_oid,
     +-		       struct string_list *versions,
     +-		       unsigned int offset,
     +-		       size_t hash_size)
     ++static int write_tree(struct object_id *result_oid,
     ++		      struct string_list *versions,
     ++		      unsigned int offset,
     ++		      size_t hash_size)
     + {
     + 	size_t maxlen = 0, extra;
     + 	unsigned int nr;
     + 	struct strbuf buf = STRBUF_INIT;
     +-	int i;
     ++	int i, ret;
     + 
     + 	assert(offset <= versions->nr);
     + 	nr = versions->nr - offset;
     +@@ merge-ort.c: static void write_tree(struct object_id *result_oid,
     + 	}
     + 
     + 	/* Write this object file out, and record in result_oid */
     +-	write_object_file(buf.buf, buf.len, OBJ_TREE, result_oid);
     ++	ret = write_object_file(buf.buf, buf.len, OBJ_TREE, result_oid);
     + 	strbuf_release(&buf);
     ++	return ret;
     + }
     + 
     + static void record_entry_for_tree(struct directory_versions *dir_metadata,
     +@@ merge-ort.c: static void record_entry_for_tree(struct directory_versions *dir_metadata,
     + 			   basename)->util = &mi->result;
     + }
     + 
     +-static void write_completed_directory(struct merge_options *opt,
     +-				      const char *new_directory_name,
     +-				      struct directory_versions *info)
     ++static int write_completed_directory(struct merge_options *opt,
     ++				     const char *new_directory_name,
     ++				     struct directory_versions *info)
     + {
     + 	const char *prev_dir;
     + 	struct merged_info *dir_info = NULL;
     +-	unsigned int offset;
     ++	unsigned int offset, ret = 0;
     + 
     + 	/*
     + 	 * Some explanation of info->versions and info->offsets...
     +@@ merge-ort.c: static void write_completed_directory(struct merge_options *opt,
     + 	 * strcmp here.)
     + 	 */
     + 	if (new_directory_name == info->last_directory)
     +-		return;
     ++		return 0;
     + 
     + 	/*
     + 	 * If we are just starting (last_directory is NULL), or last_directory
     +@@ merge-ort.c: static void write_completed_directory(struct merge_options *opt,
     + 		 */
     + 		string_list_append(&info->offsets,
     + 				   info->last_directory)->util = (void*)offset;
     +-		return;
     ++		return 0;
     + 	}
     + 
     + 	/*
     +@@ merge-ort.c: static void write_completed_directory(struct merge_options *opt,
     + 		 */
     + 		dir_info->is_null = 0;
     + 		dir_info->result.mode = S_IFDIR;
     +-		write_tree(&dir_info->result.oid, &info->versions, offset,
     +-			   opt->repo->hash_algo->rawsz);
     ++		if (write_tree(&dir_info->result.oid, &info->versions, offset,
     ++			       opt->repo->hash_algo->rawsz) < 0)
     ++			ret = -1;
     + 	}
     + 
     + 	/*
     +@@ merge-ort.c: static void write_completed_directory(struct merge_options *opt,
     + 	/* And, of course, we need to update last_directory to match. */
     + 	info->last_directory = new_directory_name;
     + 	info->last_directory_len = strlen(info->last_directory);
     ++
     ++	return ret;
     + }
     + 
     + /* Per entry merge function */
     +@@ merge-ort.c: static void prefetch_for_content_merges(struct merge_options *opt,
     + 	oid_array_clear(&to_fetch);
     + }
     + 
     +-static void process_entries(struct merge_options *opt,
     +-			    struct object_id *result_oid)
     ++static int process_entries(struct merge_options *opt,
     ++			   struct object_id *result_oid)
     + {
     + 	struct hashmap_iter iter;
     + 	struct strmap_entry *e;
     +@@ merge-ort.c: static void process_entries(struct merge_options *opt,
     + 	struct directory_versions dir_metadata = { STRING_LIST_INIT_NODUP,
     + 						   STRING_LIST_INIT_NODUP,
     + 						   NULL, 0 };
     ++	int ret;
     + 
     + 	trace2_region_enter("merge", "process_entries setup", opt->repo);
     + 	if (strmap_empty(&opt->priv->paths)) {
     + 		oidcpy(result_oid, opt->repo->hash_algo->empty_tree);
     +-		return;
     ++		return 0;
       	}
     - 	merge_finalize(&opt, &result);
     --	return !result.clean; /* result.clean < 0 handled above */
     -+	return !result.tree || !result.clean; /* result.clean < 0 handled above */
     + 
     + 	/* Hack to pre-allocate plist to the desired size */
     +@@ merge-ort.c: static void process_entries(struct merge_options *opt,
     + 		 */
     + 		struct merged_info *mi = entry->util;
     + 
     +-		write_completed_directory(opt, mi->directory_name,
     +-					  &dir_metadata);
     ++		if (write_completed_directory(opt, mi->directory_name,
     ++					      &dir_metadata) < 0) {
     ++			ret = -1;
     ++			goto cleanup;
     ++		}
     + 		if (mi->clean)
     + 			record_entry_for_tree(&dir_metadata, path, mi);
     + 		else {
     +@@ merge-ort.c: static void process_entries(struct merge_options *opt,
     + 		fflush(stdout);
     + 		BUG("dir_metadata accounting completely off; shouldn't happen");
     + 	}
     +-	write_tree(result_oid, &dir_metadata.versions, 0,
     +-		   opt->repo->hash_algo->rawsz);
     ++	ret = write_tree(result_oid, &dir_metadata.versions, 0,
     ++			 opt->repo->hash_algo->rawsz);
     ++cleanup:
     + 	string_list_clear(&plist, 0);
     + 	string_list_clear(&dir_metadata.versions, 0);
     + 	string_list_clear(&dir_metadata.offsets, 0);
     + 	trace2_region_leave("merge", "process_entries cleanup", opt->repo);
     ++
     ++	return ret;
       }
       
     - int cmd_merge_tree(int argc, const char **argv, const char *prefix)
     + /*** Function Grouping: functions related to merge_switch_to_result() ***/
     +@@ merge-ort.c: redo:
     + 	}
     + 
     + 	trace2_region_enter("merge", "process_entries", opt->repo);
     +-	process_entries(opt, &working_tree_oid);
     ++	if (process_entries(opt, &working_tree_oid) < 0)
     ++		result->clean = -1;
     + 	trace2_region_leave("merge", "process_entries", opt->repo);
     + 
     + 	/* Set return values */
     + 	result->path_messages = &opt->priv->conflicts;
     + 
     +-	result->tree = parse_tree_indirect(&working_tree_oid);
     +-	/* existence of conflicted entries implies unclean */
     +-	result->clean &= strmap_empty(&opt->priv->conflicted);
     ++	if (result->clean >= 0) {
     ++		result->tree = parse_tree_indirect(&working_tree_oid);
     ++		/* existence of conflicted entries implies unclean */
     ++		result->clean &= strmap_empty(&opt->priv->conflicted);
     ++	}
     + 	if (!opt->priv->call_depth) {
     + 		result->priv = opt->priv;
     + 		result->_properly_initialized = RESULT_INITIALIZED;
      
       ## t/t4301-merge-tree-write-tree.sh ##
      @@ t/t4301-merge-tree-write-tree.sh: test_expect_success 'can override merge of unrelated histories' '

 merge-ort.c                      | 64 +++++++++++++++++++-------------
 t/t4301-merge-tree-write-tree.sh |  9 +++++
 2 files changed, 48 insertions(+), 25 deletions(-)

On Thu, Sep 22, 2022 at 12:46 PM Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com> wrote:

:-)

This sent me into a bit of a hunt to try to figure out what the return value of write_object_file() is. I know it's non-zero on failure, but don't know if it's always positive or always negative or possibly a mix and how that maps to the idea of "clean_merge". I gave up after a few indirections, to be honest.

Anyway, regardless of my inability to find the answer to the above question, would this be a bit easier to read if we initialized ret to 0 and then did something like

    if (write_object_file(buf.buf, buf.len, OBJ_TREE, result_oid))
        ret = -1;

?

This makes me wonder about write_tree() again.  What if its call to
write_object_file() returns a value greater than 0?  Is that possible?
 If it is, are those error cases or not?  About half the callers in
the code base that check the return value of write_object_file() check
for a non-zero value, the other half check for a value less than 0.
And I can't find any documentation.  And it seemed like too much time
for me to figure out what its range of return values was.

Nice to see that it didn't take too much work to propagate the -1 value from write_tree() up the hierarchy. Nice work!

I think we've still got some separate problems related to propagating the return value of the other write_object_file() call, from handle_content_merge(). The direct call in that other case is okay, but the higher levels at process_renames() and process_entry() seem to fumble it. Your fix for failed tree writes is still valid without fixing the failed blob writes, and I'm happy to tackle the other half if you'd prefer to hand it off. But, if you'd like to tackle that half too, you might see fewer error messages when it fails to write to the read-only repository, since it'll fail early on the first blob write instead of only failing when it gets around to trying to write a new tree.

Hi Elijah,

On Thu, 22 Sep 2022, Elijah Newren wrote:

You're right, this is a better pattern to follow because it makes it easier to wrap your head around.

No, `write_object_file()` returns either 0 or -1, always. I followed all the code paths.

Here is a tangent (feel free to skip over this lengthy analysis):

These code paths include a _very, very_ misleading one. In `write_loose_object()` (which is called via the call chain `write_object_file()` -> `write_object_file_flags()` -> `write_object_file_flags()`), we call `finalize_object_file()`, see https://github.com/git/git/blob/v2.37.3/object-file.c#L2030). And in that `finalize_object_file()` function, we set `ret = errno;` in two places (https://github.com/git/git/blob/v2.37.3/object-file.c#L1833 and https://github.com/git/git/blob/v2.37.3/object-file.c#L1850).

When I read that, I chased down what the conventions are for `errno` values. Now, while https://pubs.opengroup.org/onlinepubs/007908799/xsh/errno.h.html does not say anything about `errno` values being non-negative, https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/errno.h.html very much says:

	Values for errno are now required to be distinct positive values
	rather than non-zero values. This change is for alignment with the
	ISO/IEC 9899:1999 standard.

Terrible, right? If we set `ret = errno`, that means that we return something positive!

I then even dug up the origins of the `ret = errno` statements. Turns out that yours truly introduced the first of these `ret = errno` statements in 9e48b389990 (Work around missing hard links on FAT formatted media, 2005-10-26). :facepalm:

So why did I do such a thing? Well, back then, the `ret` value _already_ contained an `errno`, thanks to calling `link_temp_to_file()`, which _did_ return an `errno`. In fact, there was no code path in `move_temp_to_file()` (as the function was called back then) that would return -1, it always returned either 0 or an `errno`.

And after coming so far, after one hour of analyzing, I finally, finally realized that there is no `return ret;` in that function. That function has a `ret` variable that is never returned _from_ that function, but it used to be returned _to_ that function, by the `link_temp_to_file()` function that does not even exist any longer.

In my defense, this terribly misleading code was not even introduced by myself. It was introduced in 230f13225df (Create object subdirectories on demand, 2005-10-08).

tl;dr all is good, `finalize_object_file()` returns 0 upon success, -1 upon failure.

And then there is `apply.c` where the return value isn't even checked at all most of the time.

Thank you!

It'll be part of v4 ;-)

Ciao, Dscho

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.

Changes since v3:

 * I now consistently use the pattern int ret = 0; ... if (...) ret = -1.
 * I added a commit to properly propagate write failures through the
   handle_content_merge() call path, even if it is not really critical (it
   just fails sooner, but the merge would have failed just the same without
   this patch).

Changes since v2:

 * Completely changed the approach by no longer touching
   builtin/merge-tree.c but instead fixing the underlying merge-ort layer:
   we no longer ignore the return value of write_tree() and
   write_object_file().

Changes since v1:

 * Using the SANITY prereq now
 * If the merge was aborted due to a write error, exit with a non-zero code
   even if the (potentially partial) merge is clean
 * The test case now also verifies that the git merge-tree command fails in
   a read-only repository even if the merge would have succeeded

Johannes Schindelin (2):
  merge-ort: fix segmentation fault in read-only repositories
  merge-ort: return early when failing to write a blob

 merge-ort.c                      | 94 ++++++++++++++++++++------------
 t/t4301-merge-tree-write-tree.sh |  9 +++
 2 files changed, 69 insertions(+), 34 deletions(-)

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

Range-diff vs v3:

 1:  198ff455f90 ! 1:  ab6df092eba merge-ort: fix segmentation fault in read-only repositories
     @@ merge-ort.c: static int tree_entry_order(const void *a_, const void *b_)
       	unsigned int nr;
       	struct strbuf buf = STRBUF_INIT;
      -	int i;
     -+	int i, ret;
     ++	int i, ret = 0;
       
       	assert(offset <= versions->nr);
       	nr = versions->nr - offset;
     @@ merge-ort.c: static void write_tree(struct object_id *result_oid,
       
       	/* Write this object file out, and record in result_oid */
      -	write_object_file(buf.buf, buf.len, OBJ_TREE, result_oid);
     -+	ret = write_object_file(buf.buf, buf.len, OBJ_TREE, result_oid);
     ++	if (write_object_file(buf.buf, buf.len, OBJ_TREE, result_oid))
     ++		ret = -1;
       	strbuf_release(&buf);
      +	return ret;
       }
     @@ merge-ort.c: static void process_entries(struct merge_options *opt,
       	struct directory_versions dir_metadata = { STRING_LIST_INIT_NODUP,
       						   STRING_LIST_INIT_NODUP,
       						   NULL, 0 };
     -+	int ret;
     ++	int ret = 0;
       
       	trace2_region_enter("merge", "process_entries setup", opt->repo);
       	if (strmap_empty(&opt->priv->paths)) {
     @@ merge-ort.c: static void process_entries(struct merge_options *opt,
       	}
      -	write_tree(result_oid, &dir_metadata.versions, 0,
      -		   opt->repo->hash_algo->rawsz);
     -+	ret = write_tree(result_oid, &dir_metadata.versions, 0,
     -+			 opt->repo->hash_algo->rawsz);
     ++	if (write_tree(result_oid, &dir_metadata.versions, 0,
     ++		       opt->repo->hash_algo->rawsz) < 0)
     ++		ret = -1;
      +cleanup:
       	string_list_clear(&plist, 0);
       	string_list_clear(&dir_metadata.versions, 0);
 -:  ----------- > 2:  087207ae0b0 merge-ort: return early when failing to write a blob

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In the previous commit, we fixed a segmentation fault when a tree object could not be written.

However, before the tree object is written, `merge-ort` wants to write out a blob object (except in cases where the merge results in a blob that already exists in the database). And this can fail, too, but we ignore that write failure so far.

Since we will always write out a new tree object in addition to the blob (and if the blob did not exist in the database yet, we can be certain that the tree object did not exist yet), the merge will _still_ fail at that point, but it does unnecessary work by continuing after the blob could not be written.

Let's pay close attention and error out early if the blob could not be written. This reduces the error output of t4301.25 ("merge-ort fails gracefully in a read-only repository") from:

	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
	error: insufficient permission for adding an object to repository database ./objects
	fatal: failure to merge

to:

	error: insufficient permission for adding an object to repository database ./objects
	error: error: Unable to add numbers to database
	fatal: failure to merge

Note: This patch adjusts two variable declarations from `unsigned` to
`int` because their purpose is to hold the return value of
`handle_content_merge()`, which is of type `int`. The existing users of
those variables are only interested whether that variable is zero or
non-zero, therefore this type change does not affect the existing code.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 merge-ort.c | 28 +++++++++++++++++++---------
 1 file changed, 19 insertions(+), 9 deletions(-)

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:

Nice find.

As long as we create at least one "new" blob the repository has not seen, we have a chance to exit before we attempt to write a tree object (and notice a failure). Checking the return value of process_entry() for failures is a good thing to do.

Will replace.  Thanks.

On Mon, Sep 26, 2022 at 2:55 PM Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com> wrote:

I don't think this is quite true. I've had a number of users come to me with "messed up git repositories" where I eventually discover that users just randomly add "sudo" to some of their commands because when things don't work, sometimes "sudo" fixes it. That means they've created stuff in their .git directory which may be owned by root, perhaps even some of the .git/objects/XX directories. However, they may have other .git/objects/XX directories owned by their normal user.

If just some .git/objects/XX directories are owned by root and not others, then it may be when users run git commands as themselves that some things can be written to the object database but not others. In particular, it could be that writing blob objects fail, but writing a tree object which references those blobs succeeds.

Patch looks good to me, though.

Elijah Newren <newren@gmail.com> writes:

It is a good example scenario that argues even more strongly for this change. We may know the correct object contents and name for the top-level tree and even manage to write it out, after computing all the blobs and the trees contained within, some of which we may know the object name but have failed to write out. Letting such a breakage unnoticed would be, eh, bad.

Yup, thanks for a review.

From: Johannes Schindelin <johannes.schindelin@gmx.de>

If the blob/tree objects cannot be written, we really need the merge operations to fail, and not to continue (and then try to access the tree object which is however still set to `NULL`).

Let's stop ignoring the return value of `write_object_file()` and `write_tree()` and set `clean = -1` in the error case.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 merge-ort.c                      | 66 ++++++++++++++++++++------------
 t/t4301-merge-tree-write-tree.sh |  9 +++++
 2 files changed, 50 insertions(+), 25 deletions(-)

On Mon, Sep 26, 2022 at 2:55 PM Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com> wrote:

I'm a little unsure about the commit message of patch 2, as I commented on, but the code all looks good to me. I don't think the comment in the commit message matters too much; whether your commit message addresses the weird case I mentioned, I feel the code handles it just fine. So I'll leave it up to you to decide whether to tweak the commit message or not. Anyway, after you decide on that, feel free to apply my

Reviewed-by: Elijah Newren <newren@gmail.com>

Thanks for investigating, for tracking down all the return codes and how to propagate them, and getting it all fixed up!

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.

Changes since v4:

 * Corrected the commit message that claimed that the second patch was not
   fixing a real bug but just avoided unnecessary work. As pointed out by
   Elijah, there are scenarios where this second patch fixes a very real
   bug.
 * Added Elijah's "Reviewed-by" footer.

Changes since v3:

 * I now consistently use the pattern int ret = 0; ... if (...) ret = -1.
 * I added a commit to properly propagate write failures through the
   handle_content_merge() call path, even if it is not really critical (it
   just fails sooner, but the merge would have failed just the same without
   this patch).

Changes since v2:

 * Completely changed the approach by no longer touching
   builtin/merge-tree.c but instead fixing the underlying merge-ort layer:
   we no longer ignore the return value of write_tree() and
   write_object_file().

Changes since v1:

 * Using the SANITY prereq now
 * If the merge was aborted due to a write error, exit with a non-zero code
   even if the (potentially partial) merge is clean
 * The test case now also verifies that the git merge-tree command fails in
   a read-only repository even if the merge would have succeeded

Johannes Schindelin (2):
  merge-ort: fix segmentation fault in read-only repositories
  merge-ort: return early when failing to write a blob

 merge-ort.c                      | 94 ++++++++++++++++++++------------
 t/t4301-merge-tree-write-tree.sh |  9 +++
 2 files changed, 69 insertions(+), 34 deletions(-)

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

Range-diff vs v4:

 1:  ab6df092eba ! 1:  63be9f9f717 merge-ort: fix segmentation fault in read-only repositories
     @@ Commit message
          Let's stop ignoring the return value of `write_object_file()` and
          `write_tree()` and set `clean = -1` in the error case.
      
     +    Reviewed-by: Elijah Newren <newren@gmail.com>
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
       ## merge-ort.c ##
 2:  087207ae0b0 ! 2:  bfc71a2d8ad merge-ort: return early when failing to write a blob
     @@ Commit message
          that already exists in the database). And this can fail, too, but we
          ignore that write failure so far.
      
     -    Since we will always write out a new tree object in addition to the blob
     -    (and if the blob did not exist in the database yet, we can be certain
     -    that the tree object did not exist yet), the merge will _still_ fail at
     -    that point, but it does unnecessary work by continuing after the blob
     -    could not be written.
     -
          Let's pay close attention and error out early if the blob could not be
          written. This reduces the error output of t4301.25 ("merge-ort fails
          gracefully in a read-only repository") from:
     @@ Commit message
                  error: error: Unable to add numbers to database
                  fatal: failure to merge
      
     +    This is _not_ just a cosmetic change: Even though one might assume that
     +    the operation would have failed anyway at the point when the new tree
     +    object is written (and the corresponding tree object _will_ be new if it
     +    contains a blob that is new), but that is not so: As pointed out by
     +    Elijah Newren, when Git has previously been allowed to add loose objects
     +    via `sudo` calls, it is very possible that the blob object cannot be
     +    written (because the corresponding `.git/objects/??/` directory may be
     +    owned by `root`) but the tree object can be written (because the
     +    corresponding objects directory is owned by the current user). This
     +    would result in a corrupt repository because it is missing the blob
     +    object, and with this here patch we prevent that.
     +
          Note: This patch adjusts two variable declarations from `unsigned` to
          `int` because their purpose is to hold the return value of
          `handle_content_merge()`, which is of type `int`. The existing users of
          those variables are only interested whether that variable is zero or
          non-zero, therefore this type change does not affect the existing code.
      
     +    Reviewed-by: Elijah Newren <newren@gmail.com>
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
       ## merge-ort.c ##

From: Johannes Schindelin <johannes.schindelin@gmx.de>

If the blob/tree objects cannot be written, we really need the merge operations to fail, and not to continue (and then try to access the tree object which is however still set to `NULL`).

Let's stop ignoring the return value of `write_object_file()` and `write_tree()` and set `clean = -1` in the error case.

Reviewed-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 merge-ort.c                      | 66 ++++++++++++++++++++------------
 t/t4301-merge-tree-write-tree.sh |  9 +++++
 2 files changed, 50 insertions(+), 25 deletions(-)

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In the previous commit, we fixed a segmentation fault when a tree object could not be written.

However, before the tree object is written, `merge-ort` wants to write out a blob object (except in cases where the merge results in a blob that already exists in the database). And this can fail, too, but we ignore that write failure so far.

Let's pay close attention and error out early if the blob could not be written. This reduces the error output of t4301.25 ("merge-ort fails gracefully in a read-only repository") from:

	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
	error: insufficient permission for adding an object to repository database ./objects
	fatal: failure to merge

to:

	error: insufficient permission for adding an object to repository database ./objects
	error: error: Unable to add numbers to database
	fatal: failure to merge

This is _not_ just a cosmetic change: Even though one might assume that the operation would have failed anyway at the point when the new tree object is written (and the corresponding tree object _will_ be new if it contains a blob that is new), but that is not so: As pointed out by Elijah Newren, when Git has previously been allowed to add loose objects via `sudo` calls, it is very possible that the blob object cannot be written (because the corresponding `.git/objects/??/` directory may be owned by `root`) but the tree object can be written (because the corresponding objects directory is owned by the current user). This would result in a corrupt repository because it is missing the blob object, and with this here patch we prevent that.

Note: This patch adjusts two variable declarations from `unsigned` to
`int` because their purpose is to hold the return value of
`handle_content_merge()`, which is of type `int`. The existing users of
those variables are only interested whether that variable is zero or
non-zero, therefore this type change does not affect the existing code.

Reviewed-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 merge-ort.c | 28 +++++++++++++++++++---------
 1 file changed, 19 insertions(+), 9 deletions(-)

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:

Thanks. The first paragraph in the quoted part above is new and not exactly "reviewed" by Elijah yet, so let's ask ;-)

To me the description of the issue looks reasonable to me. Any comments, Elijah?

The code is the same as the one in the previous round and Elijah gave his Reviewed-by, and does look good to me, too.

Thanks, all.  Queued.

On Wed, Sep 28, 2022 at 8:53 AM Junio C Hamano <gitster@pobox.com> wrote:

Looks good to me!

Elijah Newren <newren@gmail.com> writes:

Excellent.  Thanks, both.