Re: Null deref in recursive merge in df73af5f667a479764d2b2195cb0cb60b0b89e3d

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 30, 2009, 07:03 UTC
Message-ID: <7vtz0uk5z3.fsf@alter.siamese.dyndns.org>
In-Reply-To: <C695A6A4.61CD9%jbenjore@whitepages.com>

Josh ben Jore <jbenjore@whitepages.com> 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;