[PATCHv2,2/2] rerere: avoid buffer overrun
diff mbox series

Message ID 20180911185546.10449-3-newren@gmail.com
State New
Headers show
Series
  • Fix rerere segfault with specially crafted merge
Related show

Commit Message

Elijah Newren Sept. 11, 2018, 6:55 p.m. UTC
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(-)

Comments

Junio C Hamano Sept. 11, 2018, 8:45 p.m. UTC | #1
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 &&

Patch
diff mbox series

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