[PATCH] Fixing segmentation fault when merging FETCH_HEAD

[PATCH] Fixing segmentation fault when merging FETCH_HEAD

[Jose Ivan B. Vilarouca Filho]

From: "Jose Ivan B. Vilarouca Filho" <joseivan@lavid.ufpb.br>

A segmentaion fault is raised when trying to merge FETCH_HEAD
formed only by "not-for-merge" refs.

Ex:
    git init .
    git remote add origin ...
    git fetch origin
    git merge FETCH_HEAD

Signed-off-by: Jose Ivan B. Vilarouca Filho <joseivan@lavid.ufpb.br>
---
 builtin/merge.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index 101ffef..7e419dc 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1270,9 +1270,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 			    "an empty head"));
 		remoteheads = collect_parents(head_commit, &head_subsumed,
 					      argc, argv, NULL);
-		remote_head = remoteheads->item;
-		if (!remote_head)
+		if ((!remoteheads) || (!remoteheads->item))
 			die(_("%s - not something we can merge"), argv[0]);
+		remote_head = remoteheads->item;
 		if (remoteheads->next)
 			die(_("Can merge only exactly one commit into empty head"));
 		read_empty(remote_head->object.oid.hash, 0);
-- 
1.7.10.4

Re: [PATCH] Fixing segmentation fault when merging FETCH_HEAD

[Eric Sunshine]

On Sat, Mar 19, 2016 at 2:17 PM, Jose Ivan B. Vilarouca Filho
<joseivan@lavid.ufpb.br> wrote:
> From: "Jose Ivan B. Vilarouca Filho" <joseivan@lavid.ufpb.br>

You can drop this line since it is the same as the From: line in the
email envelope.

> Fixing segmentation fault when merging FETCH_HEAD

Alternate:

    merge: don't dereference NULL pointer

> A segmentaion fault is raised when trying to merge FETCH_HEAD
> formed only by "not-for-merge" refs.
>
> Ex:
>     git init .
>     git remote add origin ...
>     git fetch origin
>     git merge FETCH_HEAD

Can you add a test to ensure that some future change doesn't regress
this fix? The above recipe would make a good basis for the new test.

> Signed-off-by: Jose Ivan B. Vilarouca Filho <joseivan@lavid.ufpb.br>
> ---
> diff --git a/builtin/merge.c b/builtin/merge.c
> @@ -1270,9 +1270,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>                             "an empty head"));
>                 remoteheads = collect_parents(head_commit, &head_subsumed,
>                                               argc, argv, NULL);
> -               remote_head = remoteheads->item;
> -               if (!remote_head)
> +               if ((!remoteheads) || (!remoteheads->item))

Style: drop unnecessary parantheses

    if (!remoteheads || !remoteheads->item)

>                         die(_("%s - not something we can merge"), argv[0]);
> +               remote_head = remoteheads->item;
>                 if (remoteheads->next)
>                         die(_("Can merge only exactly one commit into empty head"));
>                 read_empty(remote_head->object.oid.hash, 0);
> --
> 1.7.10.4

Re: [PATCH] Fixing segmentation fault when merging FETCH_HEAD

[Jose Ivan B. Vilarouca Filho]

From: "Jose Ivan B. Vilarouca Filho" <joseivan@lavid.ufpb.br>

Hello, Eric.

Thanks for suggestions. I've added a test in commit replacing git fetch origin by a fake FETCH_HEAD content.

merge: don't dereference NULL pointer

A segmentaion fault is raised when trying to merge FETCH_HEAD
formed only by "not-for-merge" refs.

Ex:
    git init .
    git remote add origin ...
    git fetch origin
    git merge FETCH_HEAD

Signed-off-by: Jose Ivan B. Vilarouca Filho <joseivan@lavid.ufpb.br>
---
 builtin/merge.c         |    4 ++--
 test-merge-fetchhead.sh |   23 +++++++++++++++++++++++
 2 files changed, 25 insertions(+), 2 deletions(-)
 create mode 100755 test-merge-fetchhead.sh

diff --git a/builtin/merge.c b/builtin/merge.c
index 101ffef..098aa8a 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1270,9 +1270,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 			    "an empty head"));
 		remoteheads = collect_parents(head_commit, &head_subsumed,
 					      argc, argv, NULL);
-		remote_head = remoteheads->item;
-		if (!remote_head)
+		if (!remoteheads || !remoteheads->item)
 			die(_("%s - not something we can merge"), argv[0]);
+		remote_head = remoteheads->item;
 		if (remoteheads->next)
 			die(_("Can merge only exactly one commit into empty head"));
 		read_empty(remote_head->object.oid.hash, 0);
diff --git a/test-merge-fetchhead.sh b/test-merge-fetchhead.sh
new file mode 100755
index 0000000..383415a
--- /dev/null
+++ b/test-merge-fetchhead.sh
@@ -0,0 +1,23 @@
+#!/bin/bash
+GIT=$(pwd)/git
+REPO_DIR=./test-fetch-head-repo
+
+if [ ! -x "${GIT}" ]; then
+    echo "Missing git binary"
+    exit 1
+fi
+
+${GIT} init ${REPO_DIR} || rm -rf ${REPO_DIR} || exit 1
+pushd ${REPO_DIR} || rm -rf ${REPO_DIR} || exit 1
+#Let's fake a FETCH_HEAD only formed by not-for-merge (git fetch origin)
+echo -ne "f954fc9919c9ec33179e11aa1af562384677e174\tnot-for-merge\tbranch 'master' of https://github.com/git/git.git" > .git/FETCH_HEAD
+${GIT} merge FETCH_HEAD
+GRET=$?
+popd
+if [ ${GRET} -eq 139 ]; then
+    rm -rf ${REPO_DIR}
+    exit 1
+fi
+
+rm -rf ${REPO_DIR}
+exit 0
-- 
1.7.10.4

Re: [PATCH] Fixing segmentation fault when merging FETCH_HEAD

[Eric Sunshine]

On Sun, Mar 20, 2016 at 8:11 PM, Jose Ivan B. Vilarouca Filho
<joseivan@lavid.ufpb.br> wrote:
> Hello, Eric.
>
> Thanks for suggestions. I've added a test in commit replacing git fetch origin by a fake FETCH_HEAD content.

Thanks for the re-roll. To be "git am"-friendly, you should either
place a "-- >8 --" scissor line after the above commentary, or use
"git send-email" to send the patch, and place the commentary just
below the "---" line following your sign-off.

More below...

> merge: don't dereference NULL pointer
>
> A segmentaion fault is raised when trying to merge FETCH_HEAD
> formed only by "not-for-merge" refs.
>
> Ex:
>     git init .
>     git remote add origin ...
>     git fetch origin
>     git merge FETCH_HEAD
>
> Signed-off-by: Jose Ivan B. Vilarouca Filho <joseivan@lavid.ufpb.br>
> ---
> diff --git a/test-merge-fetchhead.sh b/test-merge-fetchhead.sh

As you're new around here, I probably should have been more specific
in my previous review and explained that you'd want your new test to
be incorporated into the existing test suite so that it actually gets
exercised regularly. A good place for the new test might be at the
bottom of t/t7600-merge.sh.

Writing a new test is pretty simple, especially when you already have
a recipe for reproduction. Take a look at existing tests in that file
to get a feel for how it should be done. Rather than all the "|| exit
1"'s, chain your test statements together with &&. And, because you'll
be incorporating the new test into an existing script, you can drop a
lot of the boilerplate below and just focus on the recipe.

Some style comments below (most of which won't matter after you drop
the boilerplate but are generally good to know)...

> @@ -0,0 +1,23 @@
> +#!/bin/bash
> +GIT=$(pwd)/git
> +REPO_DIR=./test-fetch-head-repo
> +
> +if [ ! -x "${GIT}" ]; then

Use 'test' rather than '['. Drop the semicolon. Place 'then' on its own line.

> +    echo "Missing git binary"
> +    exit 1
> +fi
> +
> +${GIT} init ${REPO_DIR} || rm -rf ${REPO_DIR} || exit 1
> +pushd ${REPO_DIR} || rm -rf ${REPO_DIR} || exit 1

In test scripts you need to take extra care when switching directories
since failure of a command following 'pushd' will prevent the
subsequent 'popd' from executing, and then tests later in the script
will be invoked in the wrong directory. The typical way of dealing
with this is to use a subhsell since 'cd' within a subshell doesn't
affect the parent, and the parent continues at its own working
directory when the subshell exits. For example:

    some-command &&
    (
        cd somewhere &&
        command-which-might-fail &&
        another-possible-failure
    )

> +#Let's fake a FETCH_HEAD only formed by not-for-merge (git fetch origin)
> +echo -ne "f954fc9919c9ec33179e11aa1af562384677e174\tnot-for-merge\tbranch 'master' of https://github.com/git/git.git" > .git/FETCH_HEAD

Non-portable 'echo' options. Use 'printf' instead. And, you'll need to
take extra care with the single-quotes since the test body itself will
be within single-quotes.

> +${GIT} merge FETCH_HEAD
> +GRET=$?
> +popd
> +if [ ${GRET} -eq 139 ]; then

So, both before and after this patch, this git-merge fails, and you
want the test to detect that it's failing in a controlled way (via
die()) rather than the previous uncontrolled way (via segfault). You
could use test_expect_code() for this (from t/test-lib-functions.sh),
or you could capture stderr and verify that it has the expected
failure message from die(). The latter is probably preferable, and may
look something like this:

test_expect_success 'my test title' '
    ...setup stuff... &&
    test_must_fail git merge FETCH_HEAD 2>err &&
    test_i18ngrep "not something we can merge" err
'

> +    rm -rf ${REPO_DIR}
> +    exit 1
> +fi
> +
> +rm -rf ${REPO_DIR}
> +exit 0

You typically don't need to cleanup after your test, so this
boilerplate can go away.

merge: fix NULL pointer dereference when merging nothing into void

[Junio C Hamano]

When we are on an unborn branch and merging only one foreign parent,
we allow "git merge" to fast-forward to that foreign parent commit.

This codepath incorrectly attempted to dereference the list of
parents that the merge is going to record even when the list is
empty.  It must refuse to operate instead when there is no parent.

All other codepaths make sure the list is not empty before they
dereference it, and are safe.

Reported by Jose Ivan B. Vilarouca Filho
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * So here is what I came up with as a suggestion.  The original
   check to see if remote_head is empty is simply bogus (an empty
   list would to have a single element whose item is NULL), so I
   rewrote it to clarify what is going on in this codepath.

 builtin/merge.c  | 10 +++++-----
 t/t7600-merge.sh | 10 ++++++++++
 2 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index 101ffef..bf2f261 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1257,12 +1257,12 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 			builtin_merge_options);
 
 	if (!head_commit) {
-		struct commit *remote_head;
 		/*
 		 * If the merged head is a valid one there is no reason
 		 * to forbid "git merge" into a branch yet to be born.
 		 * We do the same for "git pull".
 		 */
+		unsigned char *remote_head_sha1;
 		if (squash)
 			die(_("Squash commit into empty head not supported yet"));
 		if (fast_forward == FF_NO)
@@ -1270,13 +1270,13 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 			    "an empty head"));
 		remoteheads = collect_parents(head_commit, &head_subsumed,
 					      argc, argv, NULL);
-		remote_head = remoteheads->item;
-		if (!remote_head)
+		if (!remoteheads)
 			die(_("%s - not something we can merge"), argv[0]);
 		if (remoteheads->next)
 			die(_("Can merge only exactly one commit into empty head"));
-		read_empty(remote_head->object.oid.hash, 0);
-		update_ref("initial pull", "HEAD", remote_head->object.oid.hash,
+		remote_head_sha1 = remoteheads->item->object.oid.hash;
+		read_empty(remote_head_sha1, 0);
+		update_ref("initial pull", "HEAD", remote_head_sha1,
 			   NULL, 0, UPDATE_REFS_DIE_ON_ERR);
 		goto done;
 	}
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index 302e238..9d7952f 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -725,4 +725,14 @@ test_expect_success 'merge detects mod-256 conflicts (resolve)' '
 	test_must_fail git merge -s resolve master
 '
 
+test_expect_success 'merge nothing into void' '
+	git init void &&
+	(
+		cd void &&
+		git remote add up .. &&
+		git fetch up &&
+		test_must_fail git merge FETCH_HEAD
+	)
+'
+
 test_done

Re: merge: fix NULL pointer dereference when merging nothing into void

[Eric Sunshine]

On Mon, Mar 21, 2016 at 3:01 PM, Junio C Hamano <gitster@pobox.com> wrote:
> When we are on an unborn branch and merging only one foreign parent,
> we allow "git merge" to fast-forward to that foreign parent commit.
>
> This codepath incorrectly attempted to dereference the list of
> parents that the merge is going to record even when the list is
> empty.  It must refuse to operate instead when there is no parent.
>
> All other codepaths make sure the list is not empty before they
> dereference it, and are safe.
>
> Reported by Jose Ivan B. Vilarouca Filho
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
> @@ -725,4 +725,14 @@ test_expect_success 'merge detects mod-256 conflicts (resolve)' '
> +test_expect_success 'merge nothing into void' '
> +       git init void &&
> +       (
> +               cd void &&
> +               git remote add up .. &&
> +               git fetch up &&
> +               test_must_fail git merge FETCH_HEAD

Ah, nice. I either didn't know or had forgotten that test_must_fail is
smart enough to detect unexpected failures (such as segfault), so my
advice to Jose about capturing stderr[1] was misdirected. Thanks.

[1]: http://article.gmane.org/gmane.comp.version-control.git/289405

> +       )
> +'
> +
>  test_done