Segfault with merge-tree on multiple Git versions

Segfault with merge-tree on multiple Git versions

[Charlie Smurthwaite]

I am experiencing a segmentation fault in various versions of Git using
different repositories. Specifically, I have reproduced it using a
public repo and the latest stable Git version. Other repos trigger the
error on different versions.

Full info can be found below. Thanks,

Charlie


Test repository:
https://github.com/atech/mail

Test Command
git merge-tree 26bb22a052fef9f74063afd4fc6fc11fe200b19f
8d6bdf012941d876b2279994e02f1bb0d5c26e7d
d5ef97ac407d945f231cd7c8fb1cfe48b3a12083

Environment:
Linux codebase-staging 2.6.32-41-server #91-Ubuntu SMP Wed Jun 13
11:58:56 UTC 2012 x86_64 GNU/Linux

Git:
git version 1.8.2

Output:
charlie@codebase-staging:~/mail$ git merge-tree
26bb22a052fef9f74063afd4fc6fc11fe200b19f
8d6bdf012941d876b2279994e02f1bb0d5c26e7d
d5ef97ac407d945f231cd7c8fb1cfe48b3a12083
Segmentation fault



Charlie Smurthwaite aTech Media

tel. 01202 901 222 (ext. 603) email. charlie@atechmedia.com<mailto:charlie@atechmedia.com> web. atechmedia.com<http://atechmedia.com>

aTech Media Limited is a registered company in England and Wales. Registration Number 5523199. Registered Office: Unit 9 Winchester Place, North Street, Poole, Dorset, BH15 1NX. VAT Registration Number: GB 868 861 560. This e-mail is confidential and for the intended recipient only. If you are not the intended recipient, be advised that you have received this e-mail in error and that any use, dissemination, forwarding, printing, or copying of this e-mail is prohibited. If you have received this e-mail in error, please notify the sender.

Re: Segfault with merge-tree on multiple Git versions

[thomas]

Charlie Smurthwaite <charlie@atechmedia.com> writes:

> I am experiencing a segmentation fault in various versions of Git using
> different repositories. Specifically, I have reproduced it using a
> public repo and the latest stable Git version. Other repos trigger the
> error on different versions.
>
> Full info can be found below. Thanks,
>
> Charlie
>
>
> Test repository:
> https://github.com/atech/mail
>
> Test Command
> git merge-tree 26bb22a052fef9f74063afd4fc6fc11fe200b19f
> 8d6bdf012941d876b2279994e02f1bb0d5c26e7d
> d5ef97ac407d945f231cd7c8fb1cfe48b3a12083

I happened to walk past on IRC and found I could easily reproduce it, so
I bisected:

  35ffe7583108ab236dcf81226690388491d9962f is the first bad commit
  commit 35ffe7583108ab236dcf81226690388491d9962f
  Author: Junio C Hamano <gitster@pobox.com>
  Date:   Thu Dec 13 15:51:29 2012 -0800

      merge-tree: fix d/f conflicts
      
      The previous commit documented two known breakages revolving around
      a case where one side flips a tree into a blob (or vice versa),
      where the original code simply gets confused and feeds a mixture of
      trees and blobs into either the recursive merge-tree (and recursing
      into the blob will fail) or three-way merge (and merging tree contents
      together with blobs will fail).
      
      Fix it by feeding trees (and only trees) into the recursive
      merge-tree machinery and blobs (and only blobs) into the three-way
      content level merge machinery separately; when this happens, the
      entire merge has to be marked as conflicting at the structure level.
      
      Signed-off-by: Junio C Hamano <gitster@pobox.com>

It seems to be a vanilla null dereference:

  Program received signal SIGSEGV, Segmentation fault.
  0x0000000000453bf9 in add_merge_entry (entry=0x0) at builtin/merge-tree.c:24
  24              *merge_result_end = entry;
  (gdb) bt
  #0  0x0000000000453bf9 in add_merge_entry (entry=0x0) at builtin/merge-tree.c:24
  #1  0x00000000004545f4 in unresolved (info=0x7fffffffce90, n=0x7ff7f0) at builtin/merge-tree.c:265
  #2  0x0000000000454741 in threeway_callback (n=3, mask=7, dirmask=7, entry=0x7ff7f0, 
      info=0x7fffffffce90) at builtin/merge-tree.c:330
  #3  0x00000000005233f3 in traverse_trees (n=3, t=0x7fffffffcf10, info=0x7fffffffce90)
      at tree-walk.c:407
  #4  0x0000000000454792 in merge_trees_recursive (t=0x7fffffffcf10, base=0x800530 "lib/mail", 
      df_conflict=1) at builtin/merge-tree.c:341
  #5  0x0000000000454382 in unresolved_directory (info=0x7fffffffd120, n=0x800420, df_conflict=1)
      at builtin/merge-tree.c:216
  #6  0x0000000000454507 in unresolved (info=0x7fffffffd120, n=0x800420) at builtin/merge-tree.c:253
  #7  0x0000000000454741 in threeway_callback (n=3, mask=7, dirmask=7, entry=0x800420, 
      info=0x7fffffffd120) at builtin/merge-tree.c:330
  #8  0x00000000005233f3 in traverse_trees (n=3, t=0x7fffffffd1a0, info=0x7fffffffd120)
      at tree-walk.c:407
  #9  0x0000000000454792 in merge_trees_recursive (t=0x7fffffffd1a0, base=0x7fd170 "lib", df_conflict=1)
      at builtin/merge-tree.c:341
  #10 0x0000000000454382 in unresolved_directory (info=0x7fffffffd3b0, n=0x8069f0, df_conflict=1)
      at builtin/merge-tree.c:216
  #11 0x0000000000454507 in unresolved (info=0x7fffffffd3b0, n=0x8069f0) at builtin/merge-tree.c:253
  #12 0x0000000000454741 in threeway_callback (n=3, mask=7, dirmask=7, entry=0x8069f0, 
      info=0x7fffffffd3b0) at builtin/merge-tree.c:330
  #13 0x00000000005233f3 in traverse_trees (n=3, t=0x7fffffffd450, info=0x7fffffffd3b0)
      at tree-walk.c:407
  #14 0x0000000000454792 in merge_trees_recursive (t=0x7fffffffd450, base=0x5510fc "", df_conflict=0)
      at builtin/merge-tree.c:341
  #15 0x00000000004547bc in merge_trees (t=0x7fffffffd450, base=0x5510fc "") at builtin/merge-tree.c:346
  #16 0x00000000004548ef in cmd_merge_tree (argc=4, argv=0x7fffffffd728, prefix=0x0)
      at builtin/merge-tree.c:373
  #17 0x00000000004056ec in run_builtin (p=0x7a1c88 <commands.20888+1416>, argc=4, argv=0x7fffffffd728)
      at git.c:273
  #18 0x000000000040587f in handle_internal_command (argc=4, argv=0x7fffffffd728) at git.c:434
  #19 0x0000000000405a4b in main (argc=4, argv=0x7fffffffd728) at git.c:523

Unfortunately I'm not familiar with the merge code, but if you can't
reproduce at your end let me know.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: Segfault with merge-tree on multiple Git versions

[John Keeping]

On Wed, Mar 27, 2013 at 04:53:27PM +0100, thomas wrote:
> Charlie Smurthwaite <charlie@atechmedia.com> writes:
> 
> > I am experiencing a segmentation fault in various versions of Git using
> > different repositories. Specifically, I have reproduced it using a
> > public repo and the latest stable Git version. Other repos trigger the
> > error on different versions.
> >
> > Full info can be found below. Thanks,
> >
> > Charlie
> >
> >
> > Test repository:
> > https://github.com/atech/mail
> >
> > Test Command
> > git merge-tree 26bb22a052fef9f74063afd4fc6fc11fe200b19f
> > 8d6bdf012941d876b2279994e02f1bb0d5c26e7d
> > d5ef97ac407d945f231cd7c8fb1cfe48b3a12083
> 
> I happened to walk past on IRC and found I could easily reproduce it, so
> I bisected:
> 
>   35ffe7583108ab236dcf81226690388491d9962f is the first bad commit
>   commit 35ffe7583108ab236dcf81226690388491d9962f
>   Author: Junio C Hamano <gitster@pobox.com>
>   Date:   Thu Dec 13 15:51:29 2012 -0800
> 
>       merge-tree: fix d/f conflicts
>       
>       The previous commit documented two known breakages revolving around
>       a case where one side flips a tree into a blob (or vice versa),
>       where the original code simply gets confused and feeds a mixture of
>       trees and blobs into either the recursive merge-tree (and recursing
>       into the blob will fail) or three-way merge (and merging tree contents
>       together with blobs will fail).
>       
>       Fix it by feeding trees (and only trees) into the recursive
>       merge-tree machinery and blobs (and only blobs) into the three-way
>       content level merge machinery separately; when this happens, the
>       entire merge has to be marked as conflicting at the structure level.
>       
>       Signed-off-by: Junio C Hamano <gitster@pobox.com>

Looks like a simple typo in merge-tree.c::unresolved:

-- >8 --
merge-tree: fix typo in merge-tree.c::unresolved

When calculating whether there is a d/f conflict, the calculation of
whether both sides are directories generates an incorrect references
mask because it does not use the loop index to set the correct bit.
Fix this typo.

Signed-off-by: John Keeping <john@keeping.me.uk>

diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
index e0d0b7d..bc912e3 100644
--- a/builtin/merge-tree.c
+++ b/builtin/merge-tree.c
@@ -245,7 +245,7 @@ static void unresolved(const struct traverse_info *info, struct name_entry n[3])
 	unsigned dirmask = 0, mask = 0;
 
 	for (i = 0; i < 3; i++) {
-		mask |= (1 << 1);
+		mask |= (1 << i);
 		if (n[i].mode && S_ISDIR(n[i].mode))
 			dirmask |= (1 << i);
 	}

Re: Segfault with merge-tree on multiple Git versions

[Thomas Rast]

John Keeping <john@keeping.me.uk> writes:

> merge-tree: fix typo in merge-tree.c::unresolved
>
> When calculating whether there is a d/f conflict, the calculation of
> whether both sides are directories generates an incorrect references
> mask because it does not use the loop index to set the correct bit.
> Fix this typo.
>
> Signed-off-by: John Keeping <john@keeping.me.uk>
>
> diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
> index e0d0b7d..bc912e3 100644
> --- a/builtin/merge-tree.c
> +++ b/builtin/merge-tree.c
> @@ -245,7 +245,7 @@ static void unresolved(const struct traverse_info *info, struct name_entry n[3])
>  	unsigned dirmask = 0, mask = 0;
>  
>  	for (i = 0; i < 3; i++) {
> -		mask |= (1 << 1);
> +		mask |= (1 << i);
>  		if (n[i].mode && S_ISDIR(n[i].mode))
>  			dirmask |= (1 << i);
>  	}

Indeed, that fixes it.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: Segfault with merge-tree on multiple Git versions

[Junio C Hamano]

John Keeping <john@keeping.me.uk> writes:

> Looks like a simple typo in merge-tree.c::unresolved:

Thanks.

>
> -- >8 --
> merge-tree: fix typo in merge-tree.c::unresolved
>
> When calculating whether there is a d/f conflict, the calculation of
> whether both sides are directories generates an incorrect references
> mask because it does not use the loop index to set the correct bit.
> Fix this typo.
>
> Signed-off-by: John Keeping <john@keeping.me.uk>
>
> diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
> index e0d0b7d..bc912e3 100644
> --- a/builtin/merge-tree.c
> +++ b/builtin/merge-tree.c
> @@ -245,7 +245,7 @@ static void unresolved(const struct traverse_info *info, struct name_entry n[3])
>  	unsigned dirmask = 0, mask = 0;
>  
>  	for (i = 0; i < 3; i++) {
> -		mask |= (1 << 1);
> +		mask |= (1 << i);
>  		if (n[i].mode && S_ISDIR(n[i].mode))
>  			dirmask |= (1 << i);
>  	}

Re: Segfault with merge-tree on multiple Git versions

[Junio C Hamano]

Charlie Smurthwaite <charlie@atechmedia.com> writes:

> I am experiencing a segmentation fault in various versions of Git using
> different repositories.
> ...
> Test Command
> git merge-tree 26bb22a052fef9f74063afd4fc6fc11fe200b19f
> 8d6bdf012941d876b2279994e02f1bb0d5c26e7d
> d5ef97ac407d945f231cd7c8fb1cfe48b3a12083

Thanks for a report (and thanks to John and Thomas for finding the
typo).

Nobody I know uses merge-tree; the last real change we did was back
from July 2010, and the only reason I was looking at it recently was
because I was planning to write a new merge strategy using it.

Mind if I ask what you are using it for?

Re: Segfault with merge-tree on multiple Git versions

[Charlie Smurthwaite]

On 27/03/13 17:06, Junio C Hamano wrote:
> Charlie Smurthwaite <charlie@atechmedia.com> writes:
>
>> I am experiencing a segmentation fault in various versions of Git using
>> different repositories.
>> ...
>> Test Command
>> git merge-tree 26bb22a052fef9f74063afd4fc6fc11fe200b19f
>> 8d6bdf012941d876b2279994e02f1bb0d5c26e7d
>> d5ef97ac407d945f231cd7c8fb1cfe48b3a12083
> Thanks for a report (and thanks to John and Thomas for finding the
> typo).
>
> Nobody I know uses merge-tree; the last real change we did was back
> from July 2010, and the only reason I was looking at it recently was
> because I was planning to write a new merge strategy using it.
>
> Mind if I ask what you are using it for?
Thank you everybody for investigating this and creating a patch. Can I 
assume that this fix will reach somebody who can apply it to master?

With regard our use, we run an SCM hosting service http://codebasehq.com 
and are in the process of deploying a merge-request feature. We use 
git-merge-tree to determine whether a Git merge can be completed 
automatically (without manual conflict resolution), and if so offer the 
user a button to execute an actual merge. If there is a better way to do 
this, I'd be happy to consider it.

Charlie

Re: Segfault with merge-tree on multiple Git versions

[Charlie Smurthwaite]

On 27/03/13 17:06, Junio C Hamano wrote:
> Charlie Smurthwaite <charlie@atechmedia.com> writes:
>
>> I am experiencing a segmentation fault in various versions of Git using
>> different repositories.
>> ...
>> Test Command
>> git merge-tree 26bb22a052fef9f74063afd4fc6fc11fe200b19f
>> 8d6bdf012941d876b2279994e02f1bb0d5c26e7d
>> d5ef97ac407d945f231cd7c8fb1cfe48b3a12083
> Thanks for a report (and thanks to John and Thomas for finding the
> typo).
>
> Nobody I know uses merge-tree; the last real change we did was back
> from July 2010, and the only reason I was looking at it recently was
> because I was planning to write a new merge strategy using it.
>
> Mind if I ask what you are using it for?

I am also using this to obtain a diff that would be applied if a merge 
were to be run. Is there a better way to obtain this information that is 
more commonly used?

Re: Segfault with merge-tree on multiple Git versions

[Jed Brown]

Charlie Smurthwaite <charlie@atechmedia.com> writes:

> I am also using this to obtain a diff that would be applied if a merge 
> were to be run. Is there a better way to obtain this information that is 
> more commonly used?

You can do an actual merge using detached HEAD:

  $ git checkout --detach upstream-branch
  $ git merge topic-branch

This has the benefit that if there are conflicts, you can resolve them
here and commit the result so that rerere can auto-resolve them later.

Are you looking for something that can be run in a bare repo?

Re: Segfault with merge-tree on multiple Git versions

[Charlie Smurthwaite]

On 27/03/13 18:06, Jed Brown wrote:
> Charlie Smurthwaite <charlie@atechmedia.com> writes:
>
>> I am also using this to obtain a diff that would be applied if a merge
>> were to be run. Is there a better way to obtain this information that is
>> more commonly used?
> You can do an actual merge using detached HEAD:
>
>    $ git checkout --detach upstream-branch
>    $ git merge topic-branch
>
> This has the benefit that if there are conflicts, you can resolve them
> here and commit the result so that rerere can auto-resolve them later.
>
> Are you looking for something that can be run in a bare repo?

Yes, I would need to be able to do this on a bare repo for my use case. 
Thanks!

Re: Segfault with merge-tree on multiple Git versions

[Jed Brown]

Charlie Smurthwaite <charlie@atechmedia.com> writes:

> Yes, I would need to be able to do this on a bare repo for my use case. 

And if it's on the server, you don't want this to be observable, so
you don't want HEAD to move around. I don't know a better way than:

  $ git clone --shared -b upstream-branch bare-repo.git /tmp/merge-repo
  $ cd /tmp/merge-repo
  $ git pull URL incoming-branch

Cloning with --shared just writes a path into .git/objects/info/alternatives
and it doesn't need to be on the same file system (unlike --local).

Since 'git merge-tree' just works with trees, it has less information
than 'git merge'.

Re: Segfault with merge-tree on multiple Git versions

[John Keeping]

On Wed, Mar 27, 2013 at 02:16:24PM -0500, Jed Brown wrote:
> Charlie Smurthwaite <charlie@atechmedia.com> writes:
> 
> > Yes, I would need to be able to do this on a bare repo for my use case. 
> 
> And if it's on the server, you don't want this to be observable, so
> you don't want HEAD to move around. I don't know a better way than:
> 
>   $ git clone --shared -b upstream-branch bare-repo.git /tmp/merge-repo
>   $ cd /tmp/merge-repo
>   $ git pull URL incoming-branch
> 
> Cloning with --shared just writes a path into .git/objects/info/alternatives
> and it doesn't need to be on the same file system (unlike --local).
> 
> Since 'git merge-tree' just works with trees, it has less information
> than 'git merge'.

You could use a temporary index and do something like:

	rm -f TMP_INDEX
	GIT_INDEX_FILE=TMP_INDEX
	export GIT_INDEX_FILE
	git read-tree -m $base $ours $theirs &&
	git merge-index git-merge-one-file -a

then inspect that with "git diff-index --cached $ours".

Note that this will fail if there are conflicts and I don't know what
git-merge-tree will do in that case.

Re: Segfault with merge-tree on multiple Git versions

[Jeff King]

On Wed, Mar 27, 2013 at 07:45:21PM +0000, John Keeping wrote:

> On Wed, Mar 27, 2013 at 02:16:24PM -0500, Jed Brown wrote:
> > Charlie Smurthwaite <charlie@atechmedia.com> writes:
> > 
> > > Yes, I would need to be able to do this on a bare repo for my use case. 
> > 
> > And if it's on the server, you don't want this to be observable, so
> > you don't want HEAD to move around. I don't know a better way than:
> > 
> >   $ git clone --shared -b upstream-branch bare-repo.git /tmp/merge-repo
> >   $ cd /tmp/merge-repo
> >   $ git pull URL incoming-branch
> > 
> > Cloning with --shared just writes a path into .git/objects/info/alternatives
> > and it doesn't need to be on the same file system (unlike --local).
> > 
> > Since 'git merge-tree' just works with trees, it has less information
> > than 'git merge'.
> 
> You could use a temporary index and do something like:
> 
> 	rm -f TMP_INDEX
> 	GIT_INDEX_FILE=TMP_INDEX
> 	export GIT_INDEX_FILE
> 	git read-tree -m $base $ours $theirs &&
> 	git merge-index git-merge-one-file -a
> 
> then inspect that with "git diff-index --cached $ours".

That is precisely how we do it at GitHub. You probably want to add in
"--aggressive" to your read-tree to cover a few more simple cases. If
there are conflicts, we just bail and say "this can't be merged", and
expect the user to do it themselves using git.

-Peff

Re: Segfault with merge-tree on multiple Git versions

[Junio C Hamano]

John Keeping <john@keeping.me.uk> writes:

> You could use a temporary index and do something like:
>
> 	rm -f TMP_INDEX
> 	GIT_INDEX_FILE=TMP_INDEX
> 	export GIT_INDEX_FILE
> 	git read-tree -m $base $ours $theirs &&
> 	git merge-index git-merge-one-file -a
>
> then inspect that with "git diff-index --cached $ours".

Good.

> Note that this will fail if there are conflicts and I don't know what
> git-merge-tree will do in that case.

I _think_ Charlies's use case is to detect trivial merges to tell
the requestee that a merge request can be done on site, so failing
is fine when there are conflicts.  merge-tree should report
conflicts as well.

Re: Segfault with merge-tree on multiple Git versions

[Charlie Smurthwaite]

On 27/03/13 20:01, Jeff King wrote:
> On Wed, Mar 27, 2013 at 07:45:21PM +0000, John Keeping wrote:
>
>> On Wed, Mar 27, 2013 at 02:16:24PM -0500, Jed Brown wrote:
>>> Charlie Smurthwaite <charlie@atechmedia.com> writes:
>>>
>>>> Yes, I would need to be able to do this on a bare repo for my use case.
>>> And if it's on the server, you don't want this to be observable, so
>>> you don't want HEAD to move around. I don't know a better way than:
>>>
>>>    $ git clone --shared -b upstream-branch bare-repo.git /tmp/merge-repo
>>>    $ cd /tmp/merge-repo
>>>    $ git pull URL incoming-branch
>>>
>>> Cloning with --shared just writes a path into .git/objects/info/alternatives
>>> and it doesn't need to be on the same file system (unlike --local).
>>>
>>> Since 'git merge-tree' just works with trees, it has less information
>>> than 'git merge'.
>> You could use a temporary index and do something like:
>>
>> 	rm -f TMP_INDEX
>> 	GIT_INDEX_FILE=TMP_INDEX
>> 	export GIT_INDEX_FILE
>> 	git read-tree -m $base $ours $theirs &&
>> 	git merge-index git-merge-one-file -a
>>
>> then inspect that with "git diff-index --cached $ours".
> That is precisely how we do it at GitHub. You probably want to add in
> "--aggressive" to your read-tree to cover a few more simple cases. If
> there are conflicts, we just bail and say "this can't be merged", and
> expect the user to do it themselves using git.
>
> -Peff

This may be ideal. I will compare it with merge-tree to see which will 
suit best. Thank you everyone for your help here.

Charlie