[PATCH 0/2] Fix segmentation fault with cherry-pick

The culprit is actually not cherry-pick, but a special code path that
expects refresh_cache_entry() not to return NULL. And the fix is to
teach it to handle NULL there.

This bug was brought to my attention by Markus Klein via
https://github.com/git-for-windows/git/issues/952.


Johannes Schindelin (2):
  cherry-pick: demonstrate a segmentation fault
  Avoid a segmentation fault with renaming merges

 merge-recursive.c             |  2 ++
 t/t3501-revert-cherry-pick.sh | 12 ++++++++++++
 2 files changed, 14 insertions(+)


base-commit: e2b2d6a172b76d44cb7b1ddb12ea5bfac9613a44
Published-As: https://github.com/dscho/git/releases/tag/cherry-pick-segfault-v1
Fetch-It-Via: git fetch https://github.com/dscho/git cherry-pick-segfault-v1

-- 
2.11.0.rc3.windows.1

In https://github.com/git-for-windows/git/issues/952, a complicated
scenario was described that leads to a segmentation fault in
cherry-pick.

It boils down to a certain code path involving a renamed file that is
dirty, for which `refresh_cache_entry()` returns `NULL`, and that
`NULL` not being handled properly.

Signed-off-by: Johannes Schindelin <redacted>
---
 t/t3501-revert-cherry-pick.sh | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
index 394f0005a1..d96d391af3 100755
--- a/t/t3501-revert-cherry-pick.sh
+++ b/t/t3501-revert-cherry-pick.sh

@@ -141,4 +141,16 @@ test_expect_success 'cherry-pick "-" works with arguments' '
 	test_cmp expect actual
 '
 
+test_expect_failure 'cherry-pick fails gracefully with dirty renamed file' '
+	test_commit to-rename &&
+	git checkout -b unrelated &&
+	test_commit unrelated &&
+	git checkout @{-1} &&
+	git mv to-rename.t renamed &&
+	test_tick &&
+	git commit -m renamed &&
+	echo modified >renamed &&
+	git cherry-pick unrelated
+'
+
 test_done

-- 
2.11.0.rc3.windows.1

Under very particular circumstances, merge-recursive's `add_cacheinfo()`
function gets a `NULL` returned from `refresh_cache_entry()` without
expecting it, and subsequently passes it to `add_cache_entry()` which
consequently crashes.

Let's not crash.

This fixes https://github.com/git-for-windows/git/issues/952

Signed-off-by: Johannes Schindelin <redacted>
---
 merge-recursive.c             | 2 ++
 t/t3501-revert-cherry-pick.sh | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 9041c2f149..609061f58a 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c

@@ -235,6 +235,8 @@ static int add_cacheinfo(struct merge_options *o,
 		struct cache_entry *nce;
 
 		nce = refresh_cache_entry(ce, CE_MATCH_REFRESH | CE_MATCH_IGNORE_MISSING);
+		if (!nce)
+			return err(o, _("addinfo: '%s' is not up-to-date"), path);
 		if (nce != ce)
 			ret = add_cache_entry(nce, options);
 	}

diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
index d96d391af3..8e21840f11 100755
--- a/t/t3501-revert-cherry-pick.sh
+++ b/t/t3501-revert-cherry-pick.sh

@@ -141,7 +141,7 @@ test_expect_success 'cherry-pick "-" works with arguments' '
 	test_cmp expect actual
 '
 
-test_expect_failure 'cherry-pick fails gracefully with dirty renamed file' '
+test_expect_success 'cherry-pick fails gracefully with dirty renamed file' '
 	test_commit to-rename &&
 	git checkout -b unrelated &&
 	test_commit unrelated &&

-- 
2.11.0.rc3.windows.1

Hi,

On Fri, 25 Nov 2016, Johannes Schindelin wrote:

The culprit is actually not cherry-pick, but a special code path that
expects refresh_cache_entry() not to return NULL. And the fix is to
teach it to handle NULL there.

This bug was brought to my attention by Markus Klein via
https://github.com/git-for-windows/git/issues/952.

For the record, I looked at other callers of `refresh_cache_entry()`:
there is only `make_cache_entry()`, whose callers all handle NULL return
values except in resolve-undo.c. But that latter caller is okay because it
specifically does not allow refreshing (by passing 0 as options), so
refresh_cache_entry() cannot return NULL.

Ciao,
Dscho

Hi,

On Fri, 25 Nov 2016, Johannes Schindelin wrote:

+test_expect_failure 'cherry-pick fails gracefully with dirty renamed file' '

Woops. This title is wrong. It should say instead: 'cherry-pick succeeds
with unrelated renamed, dirty file'.

+	test_commit to-rename &&
+	git checkout -b unrelated &&
+	test_commit unrelated &&
+	git checkout @{-1} &&
+	git mv to-rename.t renamed &&
+	test_tick &&
+	git commit -m renamed &&
+	echo modified >renamed &&
+	git cherry-pick unrelated

And this actually warns about an ambiguous refname.

Will send out v2 in a moment.

Ciao,
Dscho

The culprit is actually not cherry-pick, but a special code path that
expects refresh_cache_entry() not to return NULL. And the fix is to
teach it to handle NULL there.

This bug was brought to my attention by Markus Klein via
https://github.com/git-for-windows/git/issues/952.

Changes since v1:

- changed test title

- avoided ambiguous refname in test


Johannes Schindelin (2):
  cherry-pick: demonstrate a segmentation fault
  Avoid a segmentation fault with renaming merges

 merge-recursive.c             |  2 ++
 t/t3501-revert-cherry-pick.sh | 12 ++++++++++++
 2 files changed, 14 insertions(+)


base-commit: e2b2d6a172b76d44cb7b1ddb12ea5bfac9613a44
Published-As: https://github.com/dscho/git/releases/tag/cherry-pick-segfault-v2
Fetch-It-Via: git fetch https://github.com/dscho/git cherry-pick-segfault-v2

Interdiff vs v1:

 diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
 index 8e21840f11..4f2a263b63 100755
 --- a/t/t3501-revert-cherry-pick.sh
 +++ b/t/t3501-revert-cherry-pick.sh
 @@ -141,7 +141,7 @@ test_expect_success 'cherry-pick "-" works with arguments' '
  	test_cmp expect actual
  '
  
 -test_expect_success 'cherry-pick fails gracefully with dirty renamed file' '
 +test_expect_success 'cherry-pick works with dirty renamed file' '
  	test_commit to-rename &&
  	git checkout -b unrelated &&
  	test_commit unrelated &&
 @@ -150,7 +150,7 @@ test_expect_success 'cherry-pick fails gracefully with dirty renamed file' '
  	test_tick &&
  	git commit -m renamed &&
  	echo modified >renamed &&
 -	git cherry-pick unrelated
 +	git cherry-pick refs/heads/unrelated
  '
  
  test_done

-- 
2.11.0.rc3.windows.1

In https://github.com/git-for-windows/git/issues/952, a complicated
scenario was described that leads to a segmentation fault in
cherry-pick.

It boils down to a certain code path involving a renamed file that is
dirty, for which `refresh_cache_entry()` returns `NULL`, and that
`NULL` not being handled properly.

Signed-off-by: Johannes Schindelin <redacted>
---
 t/t3501-revert-cherry-pick.sh | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
index 394f0005a1..d7b4251234 100755
--- a/t/t3501-revert-cherry-pick.sh
+++ b/t/t3501-revert-cherry-pick.sh

@@ -141,4 +141,16 @@ test_expect_success 'cherry-pick "-" works with arguments' '
 	test_cmp expect actual
 '
 
+test_expect_failure 'cherry-pick works with dirty renamed file' '
+	test_commit to-rename &&
+	git checkout -b unrelated &&
+	test_commit unrelated &&
+	git checkout @{-1} &&
+	git mv to-rename.t renamed &&
+	test_tick &&
+	git commit -m renamed &&
+	echo modified >renamed &&
+	git cherry-pick refs/heads/unrelated
+'
+
 test_done

-- 
2.11.0.rc3.windows.1

Under very particular circumstances, merge-recursive's `add_cacheinfo()`
function gets a `NULL` returned from `refresh_cache_entry()` without
expecting it, and subsequently passes it to `add_cache_entry()` which
consequently crashes.

Let's not crash.

This fixes https://github.com/git-for-windows/git/issues/952

Signed-off-by: Johannes Schindelin <redacted>
---
 merge-recursive.c             | 2 ++
 t/t3501-revert-cherry-pick.sh | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 9041c2f149..609061f58a 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c

@@ -235,6 +235,8 @@ static int add_cacheinfo(struct merge_options *o,
 		struct cache_entry *nce;
 
 		nce = refresh_cache_entry(ce, CE_MATCH_REFRESH | CE_MATCH_IGNORE_MISSING);
+		if (!nce)
+			return err(o, _("addinfo: '%s' is not up-to-date"), path);
 		if (nce != ce)
 			ret = add_cache_entry(nce, options);
 	}

diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
index d7b4251234..4f2a263b63 100755
--- a/t/t3501-revert-cherry-pick.sh
+++ b/t/t3501-revert-cherry-pick.sh

@@ -141,7 +141,7 @@ test_expect_success 'cherry-pick "-" works with arguments' '
 	test_cmp expect actual
 '
 
-test_expect_failure 'cherry-pick works with dirty renamed file' '
+test_expect_success 'cherry-pick works with dirty renamed file' '
 	test_commit to-rename &&
 	git checkout -b unrelated &&
 	test_commit unrelated &&

-- 
2.11.0.rc3.windows.1

Hi,

On Sat, 26 Nov 2016, Johannes Schindelin wrote:

BTW I was not quite sure why we need to refresh the cache entry here, and
1335d76e45 (merge: avoid "safer crlf" during recording of merge results,
2016-07-08) has a commit message for which I need some time to wrap my
head around.

Also, an error here may be overkill. Maybe we should simply change the "if
(nce != ce)" to an "if (nce && nce != ce)" here, as a locally-modified
file will give a nicer message later, anyway.

Dunno,
Dscho