diff mbox series

[v5,2/2] merge-ort: return early when failing to write a blob

Message ID bfc71a2d8adfbf9b899a47d469fe2343e4703ff7.1664350162.git.gitgitgadget@gmail.com (mailing list archive)
State Accepted
Commit 92481d1b26ab57525f5efe01d01c7a3d337b8df7
Headers show
Series merge-tree: fix segmentation fault in read-only repositories | expand

Commit Message

Johannes Schindelin Sept. 28, 2022, 7:29 a.m. UTC 
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(-)

Comments

Junio C Hamano Sept. 28, 2022, 3:53 p.m. UTC | #1 
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

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

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.

>  merge-ort.c | 28 +++++++++++++++++++---------
>  1 file changed, 19 insertions(+), 9 deletions(-)

Thanks, all.  Queued.
Elijah Newren Sept. 29, 2022, 1:36 a.m. UTC | #2 
On Wed, Sep 28, 2022 at 8:53 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > 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>
> > ---
>
> 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?

Looks good to me!
Junio C Hamano Sept. 29, 2022, 1:49 a.m. UTC | #3 
Elijah Newren <newren@gmail.com> writes:

>> To me the description of the issue looks reasonable to me.  Any
>> comments, Elijah?
>
> Looks good to me!

Excellent.  Thanks, both.
diff mbox series

Patch

diff --git a/merge-ort.c b/merge-ort.c
index f3bdce1041a..e5f41cce481 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -2807,6 +2807,8 @@  static int process_renames(struct merge_options *opt,
 							   pathnames,
 							   1 + 2 * opt->priv->call_depth,
 							   &merged);
+			if (clean_merge < 0)
+				return -1;
 			if (!clean_merge &&
 			    merged.mode == side1->stages[1].mode &&
 			    oideq(&merged.oid, &side1->stages[1].oid))
@@ -2916,7 +2918,7 @@  static int process_renames(struct merge_options *opt,
 			struct version_info merged;
 
 			struct conflict_info *base, *side1, *side2;
-			unsigned clean;
+			int clean;
 
 			pathnames[0] = oldpath;
 			pathnames[other_source_index] = oldpath;
@@ -2937,6 +2939,8 @@  static int process_renames(struct merge_options *opt,
 						     pathnames,
 						     1 + 2 * opt->priv->call_depth,
 						     &merged);
+			if (clean < 0)
+				return -1;
 
 			memcpy(&newinfo->stages[target_index], &merged,
 			       sizeof(merged));
@@ -3806,10 +3810,10 @@  static int write_completed_directory(struct merge_options *opt,
 }
 
 /* Per entry merge function */
-static void process_entry(struct merge_options *opt,
-			  const char *path,
-			  struct conflict_info *ci,
-			  struct directory_versions *dir_metadata)
+static int process_entry(struct merge_options *opt,
+			 const char *path,
+			 struct conflict_info *ci,
+			 struct directory_versions *dir_metadata)
 {
 	int df_file_index = 0;
 
@@ -3823,7 +3827,7 @@  static void process_entry(struct merge_options *opt,
 		record_entry_for_tree(dir_metadata, path, &ci->merged);
 		if (ci->filemask == 0)
 			/* nothing else to handle */
-			return;
+			return 0;
 		assert(ci->df_conflict);
 	}
 
@@ -3870,7 +3874,7 @@  static void process_entry(struct merge_options *opt,
 		 */
 		if (ci->filemask == 1) {
 			ci->filemask = 0;
-			return;
+			return 0;
 		}
 
 		/*
@@ -4065,7 +4069,7 @@  static void process_entry(struct merge_options *opt,
 	} else if (ci->filemask >= 6) {
 		/* Need a two-way or three-way content merge */
 		struct version_info merged_file;
-		unsigned clean_merge;
+		int clean_merge;
 		struct version_info *o = &ci->stages[0];
 		struct version_info *a = &ci->stages[1];
 		struct version_info *b = &ci->stages[2];
@@ -4074,6 +4078,8 @@  static void process_entry(struct merge_options *opt,
 						   ci->pathnames,
 						   opt->priv->call_depth * 2,
 						   &merged_file);
+		if (clean_merge < 0)
+			return -1;
 		ci->merged.clean = clean_merge &&
 				   !ci->df_conflict && !ci->path_conflict;
 		ci->merged.result.mode = merged_file.mode;
@@ -4169,6 +4175,7 @@  static void process_entry(struct merge_options *opt,
 
 	/* Record metadata for ci->merged in dir_metadata */
 	record_entry_for_tree(dir_metadata, path, &ci->merged);
+	return 0;
 }
 
 static void prefetch_for_content_merges(struct merge_options *opt,
@@ -4285,7 +4292,10 @@  static int process_entries(struct merge_options *opt,
 			record_entry_for_tree(&dir_metadata, path, mi);
 		else {
 			struct conflict_info *ci = (struct conflict_info *)mi;
-			process_entry(opt, path, ci, &dir_metadata);
+			if (process_entry(opt, path, ci, &dir_metadata) < 0) {
+				ret = -1;
+				goto cleanup;
+			};
 		}
 	}
 	trace2_region_leave("merge", "processing", opt->repo);