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

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

[Johannes Schindelin]

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

[PATCH 1/2] cherry-pick: demonstrate a segmentation fault

[Johannes Schindelin]

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 <johannes.schindelin@gmx.de>
---
 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

[PATCH 2/2] Avoid a segmentation fault with renaming merges

[Johannes Schindelin]

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 <johannes.schindelin@gmx.de>
---
 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

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

[Johannes Schindelin]

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

Re: [PATCH 1/2] cherry-pick: demonstrate a segmentation fault

[Johannes Schindelin]

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

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

[Johannes Schindelin]

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

[PATCH v2 1/2] cherry-pick: demonstrate a segmentation fault

[Johannes Schindelin]

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 <johannes.schindelin@gmx.de>
---
 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

[PATCH v2 2/2] Avoid a segmentation fault with renaming merges

[Johannes Schindelin]

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 <johannes.schindelin@gmx.de>
---
 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

Re: [PATCH v2 2/2] Avoid a segmentation fault with renaming merges

[Johannes Schindelin]

Hi,

On Sat, 26 Nov 2016, Johannes Schindelin wrote:

> 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);
>  	}

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

Re: [PATCH v2 2/2] Avoid a segmentation fault with renaming merges

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Sat, 26 Nov 2016, Johannes Schindelin wrote:
>
>> 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);
>>  	}
>
> 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.

This callsite used to call make_cache_entry() with CE_MATCH_REFRESH,
which creates a new cache entry, calls refresh_cache_ent, and
returns the cache entry"; the log message attempts to explain why we
avoid passing CE_MATCH_REFRESH and instead first add it as a merged
entry and then refresh it (and re-add it if ce got changed).  We
used to leave the old (possibly conflicted) entries for the same
path in the index while refreshing the new cache entry, which has
correctly converted result, and the old entries got in the way,
attempting a wrong eol conversion and declaring that the new entry
out-of-date.  By adding the correctly converted result as a merged
entry, which gets rid of the old entries, the refresh operation will
not be corrupted by them.

> 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.

Looking at the commit you blamed, what happened in this case before
that change was that

 (1) make_cache_entry() would have called refresh_cache_entry() with
     CE_MATCH_REFRESH and returned a NULL;

 (2) merge-recursive.c::add_cacheinfo() noticed NULL and did

     return error(_("addinfo_cache failed for path '%s'"), path)

But the updated code forgot that refresh_cache_entry() could return
NULL.  So 1335d76e45 ("merge: avoid "safer crlf" during recording of
merge results", 2016-07-08) was not a faithful rewrite.

So I agree that your patch is the right fix; using the old message
lost by mistake in 1335d76e45 may have made it more clear that this
is a fix for a misconversion in that commit, though.

In any case, this does not seem like a new regression (1/2 applied
on v2.10.0 dies the same way), so I am inclined to queue these two
but not ship in the upcoming release for now.

Thanks.

Re: [PATCH v2 2/2] Avoid a segmentation fault with renaming merges

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Looking at the commit you blamed, what happened in this case before
> that change was that
>
>  (1) make_cache_entry() would have called refresh_cache_entry() with
>      CE_MATCH_REFRESH and returned a NULL;
>
>  (2) merge-recursive.c::add_cacheinfo() noticed NULL and did
>
>      return error(_("addinfo_cache failed for path '%s'"), path)
>
> But the updated code forgot that refresh_cache_entry() could return
> NULL.  So 1335d76e45 ("merge: avoid "safer crlf" during recording of
> merge results", 2016-07-08) was not a faithful rewrite.

I'd tentatively queue the two patches fro you on top of the
jc/renormalize-merge-kill-safer-crlf topic that ends at 1335d76e45
("merge: avoid "safer crlf" during recording of merge results",
2016-07-08).  The real "fix" became like this with the above
analysis.  Semantic adjustment "error(" -> "err(o," between the old
codebase and the current one will be handled when merging.

Thanks for catching my incorrect refactoring.

-- >8 --
From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Sat, 26 Nov 2016 13:48:06 +0100
Subject: [PATCH] merge-recursive: handle NULL in add_cacheinfo() correctly

1335d76e45 ("merge: avoid "safer crlf" during recording of merge
results", 2016-07-08) tried to split make_cache_entry() call made
with CE_MATCH_REFRESH into a call to make_cache_entry() without one,
followed by a call to add_cache_entry(), refresh_cache() and another
add_cache_entry() as needed.  However the conversion was botched in
that it forgot that refresh_cache() can return NULL, which was handled
correctly in make_cache_entry() but not in the updated code.

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

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 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 de37e5153c..56385d4c01 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -213,6 +213,8 @@ static int add_cacheinfo(unsigned int mode, const unsigned char *sha1,
 		struct cache_entry *nce;
 
 		nce = refresh_cache_entry(ce, CE_MATCH_REFRESH | CE_MATCH_IGNORE_MISSING);
+		if (!nce)
+			return error(_("addinfo_cache failed for path '%s'"), 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 5bef564ff1..22970d2223 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-172-gc8d0e450d3