git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] rerere: avoid buffer overrun
@ 2018-09-05 17:56 Elijah Newren
  2018-09-11 18:55 ` [PATCHv2 0/2] Fix rerere segfault with specially crafted merge Elijah Newren
  0 siblings, 1 reply; 5+ messages in thread
From: Elijah Newren @ 2018-09-05 17:56 UTC (permalink / raw)
  To: git; +Cc: gitster, 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


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCHv2 0/2] Fix rerere segfault with specially crafted merge
  2018-09-05 17:56 [PATCH] rerere: avoid buffer overrun Elijah Newren
@ 2018-09-11 18:55 ` Elijah Newren
  2018-09-11 18:55   ` [PATCHv2 1/2] t4200: demonstrate rerere segfault on " Elijah Newren
  2018-09-11 18:55   ` [PATCHv2 2/2] rerere: avoid buffer overrun Elijah Newren
  0 siblings, 2 replies; 5+ messages in thread
From: Elijah Newren @ 2018-09-11 18:55 UTC (permalink / raw)
  To: git; +Cc: gitster, 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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCHv2 1/2] t4200: demonstrate rerere segfault on specially crafted merge
  2018-09-11 18:55 ` [PATCHv2 0/2] Fix rerere segfault with specially crafted merge Elijah Newren
@ 2018-09-11 18:55   ` Elijah Newren
  2018-09-11 18:55   ` [PATCHv2 2/2] rerere: avoid buffer overrun Elijah Newren
  1 sibling, 0 replies; 5+ messages in thread
From: Elijah Newren @ 2018-09-11 18:55 UTC (permalink / raw)
  To: git; +Cc: gitster, 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


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCHv2 2/2] rerere: avoid buffer overrun
  2018-09-11 18:55 ` [PATCHv2 0/2] Fix rerere segfault with specially crafted merge Elijah Newren
  2018-09-11 18:55   ` [PATCHv2 1/2] t4200: demonstrate rerere segfault on " Elijah Newren
@ 2018-09-11 18:55   ` Elijah Newren
  2018-09-11 20:45     ` Junio C Hamano
  1 sibling, 1 reply; 5+ messages in thread
From: Elijah Newren @ 2018-09-11 18:55 UTC (permalink / raw)
  To: git; +Cc: gitster, 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


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCHv2 2/2] rerere: avoid buffer overrun
  2018-09-11 18:55   ` [PATCHv2 2/2] rerere: avoid buffer overrun Elijah Newren
@ 2018-09-11 20:45     ` Junio C Hamano
  0 siblings, 0 replies; 5+ messages in thread
From: Junio C Hamano @ 2018-09-11 20:45 UTC (permalink / raw)
  To: Elijah Newren; +Cc: git

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2018-09-11 20:45 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-05 17:56 [PATCH] rerere: avoid buffer overrun Elijah Newren
2018-09-11 18:55 ` [PATCHv2 0/2] Fix rerere segfault with specially crafted merge Elijah Newren
2018-09-11 18:55   ` [PATCHv2 1/2] t4200: demonstrate rerere segfault on " Elijah Newren
2018-09-11 18:55   ` [PATCHv2 2/2] rerere: avoid buffer overrun Elijah Newren
2018-09-11 20:45     ` Junio C Hamano

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).