rebase -i segmentation fault and another problem

rebase -i segmentation fault and another problem

[Zbyszek Szmek]

Hi,
I get a core dump when trying to remove a commit from the tip
of a branch which was merged with a branch with a different
root. Sorry if this is unclear - picture might help:

 *-*-m-r
    /
 z-*

(m is merge point, and r is the commit to be removed,
 z is initial commit on the other branch).

A simple 'git reset --hard HEAD^' works, I know. But:

$ git rebase --interactive HEAD~2
Segmentation fault (core dumped)

Could not apply 532e7e0... z

The core files comes from 'git cherry-pick ...',
and indeed it's enough to run this command directly:

$ git-cherry-pick --ff 532e7e0
[2]    9582 segmentation fault (core dumped)  git-cherry-pick --ff 532e7e0

gdb:
Program terminated with signal 11, Segmentation fault.
#0  do_pick_commit (argc=3, argv=<value optimized out>) at builtin/revert.c:445
445 		   if (allow_ff && !hashcmp(parent->object.sha1, head))

(gdb) bt
#0  do_pick_commit (argc=3, argv=<value optimized out>) at builtin/revert.c:445
#1  revert_or_cherry_pick (argc=3, argv=<value optimized out>) at builtin/revert.c:576
#2  0x000000000040499a in run_builtin (argc=3, argv=0x7fff86b20ba8) at git.c:275
#3  handle_internal_command (argc=3, argv=0x7fff86b20ba8) at git.c:431
#4  0x0000000000404f9f in main (argc=3, argv=0x7fff86b20ba8) at git.c:516

Repo which shows this behaviour can be created with:
git init repo1; cd repo1
touch a; git add a; git commit -m a
cd ..
git init repo2; cd repo2
touch b; git add b; git commit -m b
cd ../repo1
git pull ../repo2
touch c; git add c; git commit -m c
git rebase --interactive HEAD~2
 (just save without doing anything -> segmentation fault)


$ git --version
git version 1.7.3 (compiled from git/master) 

--------------------------------------------------------

Another small problem with git rebase --interactive: in the same repo,
running git rebase HEAD~1 and removing the only commit on the list
doesn't do anything (repo is unchanged). The expected behaviour
would be to remove the tip of branch...

Best,
Zbyszek

Re: rebase -i segmentation fault and another problem

[Brandon Casey]

On 09/27/2010 09:23 AM, Zbyszek Szmek wrote:
> Hi,
> I get a core dump when trying to remove a commit from the tip
> of a branch which was merged with a branch with a different
> root. Sorry if this is unclear - picture might help:
> 
>  *-*-m-r
>     /
>  z-*
> 
> (m is merge point, and r is the commit to be removed,
>  z is initial commit on the other branch).
> 
> A simple 'git reset --hard HEAD^' works, I know. But:
> 
> $ git rebase --interactive HEAD~2
> Segmentation fault (core dumped)
> 
> Could not apply 532e7e0... z
> 
> The core files comes from 'git cherry-pick ...',
> and indeed it's enough to run this command directly:
> 
> $ git-cherry-pick --ff 532e7e0
> [2]    9582 segmentation fault (core dumped)  git-cherry-pick --ff 532e7e0
> 
> gdb:
> Program terminated with signal 11, Segmentation fault.
> #0  do_pick_commit (argc=3, argv=<value optimized out>) at builtin/revert.c:445
> 445 		   if (allow_ff && !hashcmp(parent->object.sha1, head))
                                            ^^^^^^
Looks like gdb points us to exactly where the problem is.
"parent" should be checked for NULL as it is in the 'if' statement
following this one at line 445.

Since your 'b' commit is a root commit, it has no parents.  In the cascading
'if' statement preceding line 445, parent was set to NULL for this case.
Then here at line 445, we try to dereference parent to get object.sha1. fail.

The fix seems to be this (copy/paste):


diff --git a/builtin/revert.c b/builtin/revert.c
index 4b47ace..57b51e4 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -442,7 +442,7 @@ static int do_pick_commit(void)
        else
                parent = commit->parents->item;
 
-       if (allow_ff && !hashcmp(parent->object.sha1, head))
+       if (allow_ff && parent && !hashcmp(parent->object.sha1, head))
                return fast_forward_to(commit->object.sha1, head);
 
        if (parent && parse_commit(parent) < 0)



> (gdb) bt
> #0  do_pick_commit (argc=3, argv=<value optimized out>) at builtin/revert.c:445
> #1  revert_or_cherry_pick (argc=3, argv=<value optimized out>) at builtin/revert.c:576
> #2  0x000000000040499a in run_builtin (argc=3, argv=0x7fff86b20ba8) at git.c:275
> #3  handle_internal_command (argc=3, argv=0x7fff86b20ba8) at git.c:431
> #4  0x0000000000404f9f in main (argc=3, argv=0x7fff86b20ba8) at git.c:516
> 
> Repo which shows this behaviour can be created with:
> git init repo1; cd repo1
> touch a; git add a; git commit -m a
> cd ..
> git init repo2; cd repo2
> touch b; git add b; git commit -m b
> cd ../repo1
> git pull ../repo2
> touch c; git add c; git commit -m c
> git rebase --interactive HEAD~2
>  (just save without doing anything -> segmentation fault)
> 
> 
> $ git --version
> git version 1.7.3 (compiled from git/master) 


And a stacktrace...  You included all of the information needed to
investigate and reproduce this issue.  Great bug report!  Thanks.

> --------------------------------------------------------
> 
> Another small problem with git rebase --interactive: in the same repo,
> running git rebase HEAD~1 and removing the only commit on the list
> doesn't do anything (repo is unchanged). The expected behaviour
> would be to remove the tip of branch...

This is actually documented behavior.

Read the last comment in the buffer that rebase --interactive gives
you (after the list of commits).  It says:

   # If you remove a line here THAT COMMIT WILL BE LOST.
   # However, if you remove everything, the rebase will be aborted.

-Brandon

[PATCH] builtin/revert.c: don't dereference a NULL pointer

[Brandon Casey]

From: Brandon Casey <drafnel@gmail.com>

cherry-pick will segfault when transplanting a root commit if the --ff
option is used.  This happens because the "parent" pointer is set to NULL
when the commit being cherry-picked has no parents.  Later, when "parent"
is dereferenced, the cherry-pick segfaults.

Fix this by checking whether "parent" is NULL before dereferencing it and
add a test for this case of cherry-picking a root commit with --ff.

Reported-by: Zbyszek Szmek <zbyszek@in.waw.pl>
Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
---
 builtin/revert.c          |    2 +-
 t/t3506-cherry-pick-ff.sh |   10 ++++++++++
 2 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 4b47ace..57b51e4 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -442,7 +442,7 @@ static int do_pick_commit(void)
 	else
 		parent = commit->parents->item;
 
-	if (allow_ff && !hashcmp(parent->object.sha1, head))
+	if (allow_ff && parent && !hashcmp(parent->object.sha1, head))
 		return fast_forward_to(commit->object.sha1, head);
 
 	if (parent && parse_commit(parent) < 0)
diff --git a/t/t3506-cherry-pick-ff.sh b/t/t3506-cherry-pick-ff.sh
index e17ae71..51ca391 100755
--- a/t/t3506-cherry-pick-ff.sh
+++ b/t/t3506-cherry-pick-ff.sh
@@ -95,4 +95,14 @@ test_expect_success 'cherry pick a merge relative to nonexistent parent with --f
 	test_must_fail git cherry-pick --ff -m 3 C
 '
 
+test_expect_success 'cherry pick a root commit with --ff' '
+	git reset --hard first -- &&
+	git rm file1 &&
+	echo first >file2 &&
+	git add file2 &&
+	git commit --amend -m "file2" &&
+	git cherry-pick --ff first &&
+	test "$(git rev-parse --verify HEAD)" = "1df192cd8bc58a2b275d842cede4d221ad9000d1"
+'
+
 test_done
-- 
1.7.3

Re: [PATCH] builtin/revert.c: don't dereference a NULL pointer

[Zbyszek Szmek]

Works as advertised. Tests also pass.

Thanks,
Zbyszek

On Mon, Sep 27, 2010 at 12:29:45PM -0500, Brandon Casey wrote:
> From: Brandon Casey <drafnel@gmail.com>
> 
> cherry-pick will segfault when transplanting a root commit if the --ff
> option is used.  This happens because the "parent" pointer is set to NULL
> when the commit being cherry-picked has no parents.  Later, when "parent"
> is dereferenced, the cherry-pick segfaults.
> 
> Fix this by checking whether "parent" is NULL before dereferencing it and
> add a test for this case of cherry-picking a root commit with --ff.
> 
> Reported-by: Zbyszek Szmek <zbyszek@in.waw.pl>
> Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
> ---
>  builtin/revert.c          |    2 +-
>  t/t3506-cherry-pick-ff.sh |   10 ++++++++++
>  2 files changed, 11 insertions(+), 1 deletions(-)
> 
> diff --git a/builtin/revert.c b/builtin/revert.c
> index 4b47ace..57b51e4 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -442,7 +442,7 @@ static int do_pick_commit(void)
>  	else
>  		parent = commit->parents->item;
>  
> -	if (allow_ff && !hashcmp(parent->object.sha1, head))
> +	if (allow_ff && parent && !hashcmp(parent->object.sha1, head))
>  		return fast_forward_to(commit->object.sha1, head);
>  
>  	if (parent && parse_commit(parent) < 0)
> diff --git a/t/t3506-cherry-pick-ff.sh b/t/t3506-cherry-pick-ff.sh
> index e17ae71..51ca391 100755
> --- a/t/t3506-cherry-pick-ff.sh
> +++ b/t/t3506-cherry-pick-ff.sh
> @@ -95,4 +95,14 @@ test_expect_success 'cherry pick a merge relative to nonexistent parent with --f
>  	test_must_fail git cherry-pick --ff -m 3 C
>  '
>  
> +test_expect_success 'cherry pick a root commit with --ff' '
> +	git reset --hard first -- &&
> +	git rm file1 &&
> +	echo first >file2 &&
> +	git add file2 &&
> +	git commit --amend -m "file2" &&
> +	git cherry-pick --ff first &&
> +	test "$(git rev-parse --verify HEAD)" = "1df192cd8bc58a2b275d842cede4d221ad9000d1"
> +'
> +
>  test_done
> -- 
> 1.7.3