| Message ID | 20180911185546.10449-3-newren@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix rerere segfault with specially crafted merge | expand |
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 &&
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 &&
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(-)