{"thread":{"id":"49284","subject":"[PATCH] rerere: avoid buffer overrun","startedAt":"2018-09-05T18:18:18Z","lastAt":"2018-09-11T20:45:05Z","messageCount":5,"participants":["Elijah Newren","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"357461","messageId":"20180905175605.12341-1-newren@gmail.com","threadId":"49284","inReplyTo":null,"subject":"[PATCH] rerere: avoid buffer overrun","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-09-05T17:56:05Z","receivedAt":"2018-09-05T18:18:18Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"check_one_conflict() compares `i` to `active_nr` in two places to avoid\nbuffer overruns, but left out an important third location.\n\nNote that this bug probably cannot be triggered in the current codebase.\nExisting merge strategies have tended not to create entries at stage #1\nthat do not have a corresponding entry at either stage #2 or stage #3.\n(The bug was found while exploring some ideas for modifying conflict\nresolution.)  However, it is technically possible that an external merge\nstrategy could create such entries, so add a check to avoid segfaults.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\nOriginally submitted here:\n  https://public-inbox.org/git/20180806224745.8681-2-newren@gmail.com/\nWhile I want to push that RFC series more and will get back to it, this\npatch is independently good so I'm submitting separately.\n\nFor some history about how the current code got to where it is today, see:\n  fb70a06da2f1 (\"rerere: fix an off-by-one non-bug\", 2015-06-28)\n  5eda906b2873 (\"rerere: handle conflicts with multiple stage #1 entries\", 2015-07-24)\n\n rerere.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/rerere.c b/rerere.c\nindex c7787aa07f..783d4dae2a 100644\n--- a/rerere.c\n+++ b/rerere.c\n@@ -533,7 +533,7 @@ static int check_one_conflict(int i, int *type)\n \t}\n \n \t*type = PUNTED;\n-\twhile (ce_stage(active_cache[i]) == 1)\n+\twhile (i < active_nr && ce_stage(active_cache[i]) == 1)\n \t\ti++;\n \n \t/* Only handle regular files with both stages #2 and #3 */\n-- \n2.19.0.rc2\n\n"},{"id":"357892","messageId":"20180911185546.10449-1-newren@gmail.com","threadId":"49284","inReplyTo":"20180905175605.12341-1-newren@gmail.com","subject":"[PATCHv2 0/2] Fix rerere segfault with specially crafted merge","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-09-11T18:55:44Z","receivedAt":"2018-09-11T18:55:53Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"This series documents and fixes a rerere segfault (dating back to\ngit-2.7.0) when a merge strategy makes the last entry in the index be\nat stage 1.\n\nChanges since last version:\n  - In my last submission I only had the code fix and no testcase; I\n    even commented in the commit message that the bug \"cannot be\n    triggered in the current codebase\" and mentioned that I was fixing\n    it just because an exotic external merge strategy could trigger\n    the bug.  I realized later that it is triggerable without any\n    exotic external merge strategies; built-in ones can do it.\n\nElijah Newren (2):\n  t4200: demonstrate rerere segfault on specially crafted merge\n  rerere: avoid buffer overrun\n\n rerere.c          |  2 +-\n t/t4200-rerere.sh | 29 +++++++++++++++++++++++++++++\n 2 files changed, 30 insertions(+), 1 deletion(-)\n\n-- \n2.19.0.2.gdbd064c81f\n\n"},{"id":"357893","messageId":"20180911185546.10449-3-newren@gmail.com","threadId":"49284","inReplyTo":"20180911185546.10449-1-newren@gmail.com","subject":"[PATCHv2 2/2] rerere: avoid buffer overrun","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-09-11T18:55:46Z","receivedAt":"2018-09-11T18:55:54Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"check_one_conflict() compares `i` to `active_nr` in two places to avoid\nbuffer overruns, but left out an important third location.\n\nThe code did used to have a check here comparing i to active_nr, back\nbefore commit fb70a06da2f1 (\"rerere: fix an off-by-one non-bug\",\n2015-06-28), however the code at the time used an 'if' rather than a\n'while' meaning back then that this loop could not have read past the\nend of the array, making the check unnecessary and it was removed.\nUnfortunately, in commit 5eda906b2873 (\"rerere: handle conflicts with\nmultiple stage #1 entries\", 2015-07-24), the 'if' was changed to a\n'while' and the check comparing i and active_nr was not re-instated,\nleading to this problem.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n rerere.c          | 2 +-\n t/t4200-rerere.sh | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/rerere.c b/rerere.c\nindex c7787aa07f..783d4dae2a 100644\n--- a/rerere.c\n+++ b/rerere.c\n@@ -533,7 +533,7 @@ static int check_one_conflict(int i, int *type)\n \t}\n \n \t*type = PUNTED;\n-\twhile (ce_stage(active_cache[i]) == 1)\n+\twhile (i < active_nr && ce_stage(active_cache[i]) == 1)\n \t\ti++;\n \n \t/* Only handle regular files with both stages #2 and #3 */\ndiff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh\nindex f9294b7677..313222d0d6 100755\n--- a/t/t4200-rerere.sh\n+++ b/t/t4200-rerere.sh\n@@ -596,7 +596,7 @@ test_expect_success 'setup simple stage 1 handling' '\n \t)\n '\n \n-test_expect_failure 'test simple stage 1 handling' '\n+test_expect_success 'test simple stage 1 handling' '\n \t(\n \t\tcd stage_1_handling &&\n \n-- \n2.19.0.2.gdbd064c81f\n\n"},{"id":"357894","messageId":"20180911185546.10449-2-newren@gmail.com","threadId":"49284","inReplyTo":"20180911185546.10449-1-newren@gmail.com","subject":"[PATCHv2 1/2] t4200: demonstrate rerere segfault on specially crafted merge","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-09-11T18:55:45Z","receivedAt":"2018-09-11T18:55:55Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Signed-off-by: Elijah Newren <newren@gmail.com>\n---\n t/t4200-rerere.sh | 29 +++++++++++++++++++++++++++++\n 1 file changed, 29 insertions(+)\n\ndiff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh\nindex 65da74c766..f9294b7677 100755\n--- a/t/t4200-rerere.sh\n+++ b/t/t4200-rerere.sh\n@@ -577,4 +577,33 @@ test_expect_success 'multiple identical conflicts' '\n \tcount_pre_post 0 0\n '\n \n+test_expect_success 'setup simple stage 1 handling' '\n+\ttest_create_repo stage_1_handling &&\n+\t(\n+\t\tcd stage_1_handling &&\n+\n+\t\ttest_seq 1 10 >original &&\n+\t\tgit add original &&\n+\t\tgit commit -m original &&\n+\n+\t\tgit checkout -b A master &&\n+\t\tgit mv original A &&\n+\t\tgit commit -m \"rename to A\" &&\n+\n+\t\tgit checkout -b B master &&\n+\t\tgit mv original B &&\n+\t\tgit commit -m \"rename to B\"\n+\t)\n+'\n+\n+test_expect_failure 'test simple stage 1 handling' '\n+\t(\n+\t\tcd stage_1_handling &&\n+\n+\t\tgit config rerere.enabled true &&\n+\t\tgit checkout A^0 &&\n+\t\ttest_must_fail git merge B^0\n+\t)\n+'\n+\n test_done\n-- \n2.19.0.2.gdbd064c81f\n\n"},{"id":"357912","messageId":"xmqqd0tjojar.fsf@gitster-ct.c.googlers.com","threadId":"49284","inReplyTo":"20180911185546.10449-3-newren@gmail.com","subject":"Re: [PATCHv2 2/2] rerere: avoid buffer overrun","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-11T20:45:00Z","receivedAt":"2018-09-11T20:45:05Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren <newren@gmail.com> writes:\n\n> check_one_conflict() compares `i` to `active_nr` in two places to avoid\n> buffer overruns, but left out an important third location.\n>\n> The code did used to have a check here comparing i to active_nr, back\n> before commit fb70a06da2f1 (\"rerere: fix an off-by-one non-bug\",\n> 2015-06-28), however the code at the time used an 'if' rather than a\n> 'while' meaning back then that this loop could not have read past the\n> end of the array, making the check unnecessary and it was removed.\n> Unfortunately, in commit 5eda906b2873 (\"rerere: handle conflicts with\n> multiple stage #1 entries\", 2015-07-24), the 'if' was changed to a\n> 'while' and the check comparing i and active_nr was not re-instated,\n> leading to this problem.\n>\n> Signed-off-by: Elijah Newren <newren@gmail.com>\n> ---\n\nThanks.  Looks good to me.\n\n>  rerere.c          | 2 +-\n>  t/t4200-rerere.sh | 2 +-\n>  2 files changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/rerere.c b/rerere.c\n> index c7787aa07f..783d4dae2a 100644\n> --- a/rerere.c\n> +++ b/rerere.c\n> @@ -533,7 +533,7 @@ static int check_one_conflict(int i, int *type)\n>  \t}\n>  \n>  \t*type = PUNTED;\n> -\twhile (ce_stage(active_cache[i]) == 1)\n> +\twhile (i < active_nr && ce_stage(active_cache[i]) == 1)\n>  \t\ti++;\n>  \n>  \t/* Only handle regular files with both stages #2 and #3 */\n> diff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh\n> index f9294b7677..313222d0d6 100755\n> --- a/t/t4200-rerere.sh\n> +++ b/t/t4200-rerere.sh\n> @@ -596,7 +596,7 @@ test_expect_success 'setup simple stage 1 handling' '\n>  \t)\n>  '\n>  \n> -test_expect_failure 'test simple stage 1 handling' '\n> +test_expect_success 'test simple stage 1 handling' '\n>  \t(\n>  \t\tcd stage_1_handling &&\n"}]}