* [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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.