Re: Null deref in recursive merge in df73af5f667a479764d2b2195cb0cb60b0b89e3d

Josh ben Jore [off-list ref] writes:

I know more now. 

Thanks.  The log was a bit hard to read with linewrapping; here is what I
could glean out of it anyway.

You have something like this in the output

    CONFLICT (rename/add):

    Rename config/conf/target/dev-ubuntu/wpn_rails/appserver.yml
    ->
    config/conf/target/dev/wpn_rails/appserver.yml
    in
    Temporary merge branch 1.
    config/conf/target/dev/wpn_rails/appse2

    Adding as
    config/conf/target/dev/wpn_rails/appserver.yml~Temporary merge branch 2
    instead

which almost matches this part from merge_recursive.c

	} else if (!sha_eq(dst_other.sha1, null_sha1)) {
		const char *new_path;
		clean_merge = 0;
		try_merge = 1;
		output(o, 1, "CONFLICT (rename/add): Rename %s->%s in %s. "
		       "%s added in %s",
		       ren1_src, ren1_dst, branch1,
		       ren1_dst, branch2);
		new_path = unique_path(o, ren1_dst, branch2);
		output(o, 1, "Adding as %s instead", new_path);
		update_file(o, 0, dst_other.sha1, dst_other.mode, new_path);

although I do not see "%s added in %s" part, which means we cannot see what
ren1_dst nor branch2 were.

And the crucial bit is:

      There are unmerged index entries:
      2 config/conf/target/dev/wpn_rails/appserver.yml
      3 config/conf/target/dev/wpn_rails/appserver.yml

So the code did not want to add config/conf/target/dev/wpn_rails/appse2???
and instead tried to add it with suffix.  That is what the "update_file()"
does for a recursive "virtual base" merge --- it is supposed to resolve
everything down to stage#0.

But it forgets to resolve the original path (in dev/ not in dev-ubuntu/
and without the "~Temporary" suffix).

Since I do not have an access to exact details, nor reproducible history,
this is shot in the dark, but I think this may fix it.

The codepath saw that one branch renamed dev-ubuntu/ stuff to dev/ at that
"unmerged" path, while the other branch added something else to the same
path, and decided to add that at an alternative path, and the intent of
that is so that it can safely resolve the "renamed" side to its final
destination.  The added update_file() call is about finishing that
conflict resolution the code forgets to do.

 merge-recursive.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index d415c41..868b383 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c

@@ -955,6 +955,7 @@ static int process_renames(struct merge_options *o,
 				new_path = unique_path(o, ren1_dst, branch2);
 				output(o, 1, "Adding as %s instead", new_path);
 				update_file(o, 0, dst_other.sha1, dst_other.mode, new_path);
+				update_file(o, 0, src_other.sha1, src_other.mode, ren1_dst);
 			} else if ((item = string_list_lookup(ren1_dst, renames2Dst))) {
 				ren2 = item->util;
 				clean_merge = 0;

On 7/30/09 12:03 AM, "Junio C Hamano" [off-list ref] wrote:

Josh ben Jore [off-list ref] writes:

quoted

I know more now.

Thanks.  The log was a bit hard to read with linewrapping; here is what I
could glean out of it anyway.

Sorry about that. I didn't realize it was wrapped. I thought I'd use my
work's Outlook since I was addressing a problem for work.

Since I do not have an access to exact details, nor reproducible history,
this is shot in the dark, but I think this may fix it.

It is a very accurate shot in the dark. It appears to have fixed it when
applied to 0a53e9ddeaddad63ad106860237bbf53411d11a7 GIT 1.6.4. I'll be
trying this against v1.6.0.4 tomorrow.

Thanks very much,
Josh

When a branch moves A to B while the other branch created B (or moved C to
B), the code tried to rename one of them to B~something to preserve both
versions, and failed to register temporary resolution for the original
path B at stage#0 during virtual ancestor computation.  This left the
index in unmerged state and caused a segfault.

A better solution is to merge these two versions of B's in place and use
the (potentially conflicting) result as the intermediate merge result in
the virtual ancestor.

Signed-off-by: Junio C Hamano <redacted>
---

 Junio C Hamano [off-list ref] writes:

 > The codepath saw that one branch renamed dev-ubuntu/ stuff to dev/ at that
 > "unmerged" path, while the other branch added something else to the same
 > path, and decided to add that at an alternative path, and the intent of
 > that is so that it can safely resolve the "renamed" side to its final
 > destination.  The added update_file() call is about finishing that
 > conflict resolution the code forgets to do.
 > ...

 Yesterday's patch squashes the conflicted path down to stage#0 even for
 outermost merge, which is not quite correct.

 This may be a better fix.

 merge-recursive.c                 |   28 ++++++++++++++++--
 t/t6036-recursive-corner-cases.sh |   56 +++++++++++++++++++++++++++++++++++++
 2 files changed, 81 insertions(+), 3 deletions(-)
 create mode 100755 t/t6036-recursive-corner-cases.sh

diff --git a/merge-recursive.c b/merge-recursive.c
index d415c41..10d7913 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c

@@ -952,9 +952,31 @@ static int process_renames(struct merge_options *o,
 				       "%s added in %s",
 				       ren1_src, ren1_dst, branch1,
 				       ren1_dst, branch2);
-				new_path = unique_path(o, ren1_dst, branch2);
-				output(o, 1, "Adding as %s instead", new_path);
-				update_file(o, 0, dst_other.sha1, dst_other.mode, new_path);
+				if (o->call_depth) {
+					struct merge_file_info mfi;
+					struct diff_filespec one, a, b;
+
+					one.path = a.path = b.path =
+						(char *)ren1_dst;
+					hashcpy(one.sha1, null_sha1);
+					one.mode = 0;
+					hashcpy(a.sha1, ren1->pair->two->sha1);
+					a.mode = ren1->pair->two->mode;
+					hashcpy(b.sha1, dst_other.sha1);
+					b.mode = dst_other.mode;
+					mfi = merge_file(o, &one, &a, &b,
+							 branch1,
+							 branch2);
+					output(o, 1, "Adding merged %s", ren1_dst);
+					update_file(o, 0,
+						    mfi.sha,
+						    mfi.mode,
+						    ren1_dst);
+				} else {
+					new_path = unique_path(o, ren1_dst, branch2);
+					output(o, 1, "Adding as %s instead", new_path);
+					update_file(o, 0, dst_other.sha1, dst_other.mode, new_path);
+				}
 			} else if ((item = string_list_lookup(ren1_dst, renames2Dst))) {
 				ren2 = item->util;
 				clean_merge = 0;

diff --git a/t/t6036-recursive-corner-cases.sh b/t/t6036-recursive-corner-cases.sh
new file mode 100755
index 0000000..a9d1613
--- /dev/null
+++ b/t/t6036-recursive-corner-cases.sh

@@ -0,0 +1,56 @@
+#!/bin/sh
+
+test_description='recursive merge corner cases'
+
+. ./test-lib.sh
+
+#
+#  L1  L2
+#   o---o
+#  / \ / \
+# o   X   ?
+#  \ / \ /
+#   o---o
+#  R1  R2
+#
+
+test_expect_success setup '
+	ten="0 1 2 3 4 5 6 7 8 9"
+	for i in $ten
+	do
+		echo line $i in a sample file
+	done >one &&
+	for i in $ten
+	do
+		echo line $i in another sample file
+	done >two &&
+	git add one two &&
+	test_tick && git commit -m initial &&
+
+	git branch L1 &&
+	git checkout -b R1 &&
+	git mv one three &&
+	test_tick && git commit -m R1 &&
+
+	git checkout L1 &&
+	git mv two three &&
+	test_tick && git commit -m L1 &&
+
+	git checkout L1^0 &&
+	test_tick && git merge -s ours R1 &&
+	git tag L2 &&
+
+	git checkout R1^0 &&
+	test_tick && git merge -s ours L1 &&
+	git tag R2
+'
+
+test_expect_success merge '
+	git reset --hard &&
+	git checkout L2^0 &&
+
+	test_must_fail git merge -s recursive R2^0
+'
+
+test_done
+

-- 
1.6.4.13.ge65800