[PATCH] rerere: avoid buffer overrun

[PATCH] rerere: avoid buffer overrun

[Elijah Newren]

check_one_conflict() compares `i` to `active_nr` in two places to avoid
buffer overruns, but left out an important third location.

Note that this bug probably cannot be triggered in the current codebase.
Existing merge strategies have tended not to create entries at stage #1
that do not have a corresponding entry at either stage #2 or stage #3.
(The bug was found while exploring some ideas for modifying conflict
resolution.)  However, it is technically possible that an external merge
strategy could create such entries, so add a check to avoid segfaults.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
Originally submitted here:
  https://public-inbox.org/git/20180806224745.8681-2-newren@gmail.com/
While I want to push that RFC series more and will get back to it, this
patch is independently good so I'm submitting separately.

For some history about how the current code got to where it is today, see:
  fb70a06da2f1 ("rerere: fix an off-by-one non-bug", 2015-06-28)
  5eda906b2873 ("rerere: handle conflicts with multiple stage #1 entries", 2015-07-24)

 rerere.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/rerere.c b/rerere.c
index c7787aa07f..783d4dae2a 100644
--- a/rerere.c
+++ b/rerere.c
@@ -533,7 +533,7 @@ static int check_one_conflict(int i, int *type)
 	}
 
 	*type = PUNTED;
-	while (ce_stage(active_cache[i]) == 1)
+	while (i < active_nr && ce_stage(active_cache[i]) == 1)
 		i++;
 
 	/* Only handle regular files with both stages #2 and #3 */
-- 
2.19.0.rc2

[PATCHv2 0/2] Fix rerere segfault with specially crafted merge

[Elijah Newren]

This series documents and fixes a rerere segfault (dating back to
git-2.7.0) when a merge strategy makes the last entry in the index be
at stage 1.

Changes since last version:
  - In my last submission I only had the code fix and no testcase; I
    even commented in the commit message that the bug "cannot be
    triggered in the current codebase" and mentioned that I was fixing
    it just because an exotic external merge strategy could trigger
    the bug.  I realized later that it is triggerable without any
    exotic external merge strategies; built-in ones can do it.

Elijah Newren (2):
  t4200: demonstrate rerere segfault on specially crafted merge
  rerere: avoid buffer overrun

 rerere.c          |  2 +-
 t/t4200-rerere.sh | 29 +++++++++++++++++++++++++++++
 2 files changed, 30 insertions(+), 1 deletion(-)

-- 
2.19.0.2.gdbd064c81f

[PATCHv2 1/2] t4200: demonstrate rerere segfault on specially crafted merge

[Elijah Newren]

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 t/t4200-rerere.sh | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh
index 65da74c766..f9294b7677 100755
--- a/t/t4200-rerere.sh
+++ b/t/t4200-rerere.sh
@@ -577,4 +577,33 @@ test_expect_success 'multiple identical conflicts' '
 	count_pre_post 0 0
 '
 
+test_expect_success 'setup simple stage 1 handling' '
+	test_create_repo stage_1_handling &&
+	(
+		cd stage_1_handling &&
+
+		test_seq 1 10 >original &&
+		git add original &&
+		git commit -m original &&
+
+		git checkout -b A master &&
+		git mv original A &&
+		git commit -m "rename to A" &&
+
+		git checkout -b B master &&
+		git mv original B &&
+		git commit -m "rename to B"
+	)
+'
+
+test_expect_failure 'test simple stage 1 handling' '
+	(
+		cd stage_1_handling &&
+
+		git config rerere.enabled true &&
+		git checkout A^0 &&
+		test_must_fail git merge B^0
+	)
+'
+
 test_done
-- 
2.19.0.2.gdbd064c81f

[PATCHv2 2/2] rerere: avoid buffer overrun

[Elijah Newren]

check_one_conflict() compares `i` to `active_nr` in two places to avoid
buffer overruns, but left out an important third location.

The code did used to have a check here comparing i to active_nr, back
before commit fb70a06da2f1 ("rerere: fix an off-by-one non-bug",
2015-06-28), however the code at the time used an 'if' rather than a
'while' meaning back then that this loop could not have read past the
end of the array, making the check unnecessary and it was removed.
Unfortunately, in commit 5eda906b2873 ("rerere: handle conflicts with
multiple stage #1 entries", 2015-07-24), the 'if' was changed to a
'while' and the check comparing i and active_nr was not re-instated,
leading to this problem.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 rerere.c          | 2 +-
 t/t4200-rerere.sh | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/rerere.c b/rerere.c
index c7787aa07f..783d4dae2a 100644
--- a/rerere.c
+++ b/rerere.c
@@ -533,7 +533,7 @@ static int check_one_conflict(int i, int *type)
 	}
 
 	*type = PUNTED;
-	while (ce_stage(active_cache[i]) == 1)
+	while (i < active_nr && ce_stage(active_cache[i]) == 1)
 		i++;
 
 	/* Only handle regular files with both stages #2 and #3 */
diff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh
index f9294b7677..313222d0d6 100755
--- a/t/t4200-rerere.sh
+++ b/t/t4200-rerere.sh
@@ -596,7 +596,7 @@ test_expect_success 'setup simple stage 1 handling' '
 	)
 '
 
-test_expect_failure 'test simple stage 1 handling' '
+test_expect_success 'test simple stage 1 handling' '
 	(
 		cd stage_1_handling &&
 
-- 
2.19.0.2.gdbd064c81f

Re: [PATCHv2 2/2] rerere: avoid buffer overrun

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

> check_one_conflict() compares `i` to `active_nr` in two places to avoid
> buffer overruns, but left out an important third location.
>
> The code did used to have a check here comparing i to active_nr, back
> before commit fb70a06da2f1 ("rerere: fix an off-by-one non-bug",
> 2015-06-28), however the code at the time used an 'if' rather than a
> 'while' meaning back then that this loop could not have read past the
> end of the array, making the check unnecessary and it was removed.
> Unfortunately, in commit 5eda906b2873 ("rerere: handle conflicts with
> multiple stage #1 entries", 2015-07-24), the 'if' was changed to a
> 'while' and the check comparing i and active_nr was not re-instated,
> leading to this problem.
>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---

Thanks.  Looks good to me.

>  rerere.c          | 2 +-
>  t/t4200-rerere.sh | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/rerere.c b/rerere.c
> index c7787aa07f..783d4dae2a 100644
> --- a/rerere.c
> +++ b/rerere.c
> @@ -533,7 +533,7 @@ static int check_one_conflict(int i, int *type)
>  	}
>  
>  	*type = PUNTED;
> -	while (ce_stage(active_cache[i]) == 1)
> +	while (i < active_nr && ce_stage(active_cache[i]) == 1)
>  		i++;
>  
>  	/* Only handle regular files with both stages #2 and #3 */
> diff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh
> index f9294b7677..313222d0d6 100755
> --- a/t/t4200-rerere.sh
> +++ b/t/t4200-rerere.sh
> @@ -596,7 +596,7 @@ test_expect_success 'setup simple stage 1 handling' '
>  	)
>  '
>  
> -test_expect_failure 'test simple stage 1 handling' '
> +test_expect_success 'test simple stage 1 handling' '
>  	(
>  		cd stage_1_handling &&