{"thread":{"id":"45422","subject":"[PATCH] pickaxe: fix segfault with '-S<...> --pickaxe-regex'","startedAt":"2017-03-18T15:13:00Z","lastAt":"2017-03-21T11:46:58Z","messageCount":6,"participants":["SZEDER Gábor","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"314502","messageId":"20170318151239.17196-1-szeder.dev@gmail.com","threadId":"45422","inReplyTo":null,"subject":"[PATCH] pickaxe: fix segfault with '-S<...> --pickaxe-regex'","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2017-03-18T15:12:39Z","receivedAt":"2017-03-18T15:13:00Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"'git {log,diff,...} -S<...> --pickaxe-regex' can segfault as a result\nof out-of-bounds memory reads.\n\ndiffcore-pickaxe.c:contains() looks for all matches of the given regex\nin a buffer in a loop, advancing the buffer pointer to the end of the\nlast match in each iteration.  When we switched to REG_STARTEND in\nb7d36ffca (regex: use regexec_buf(), 2016-09-21), we started passing\nthe size of that buffer to the regexp engine, too.  Unfortunately,\nthis buffer size is never updated on subsequent iterations, and as the\nbuffer pointer advances on each iteration, this \"bufptr+bufsize\"\npoints past the end of the buffer.  This results in segmentation\nfault, if that memory can't be accessed.  In case of 'git log' it can\nalso result in erroneously listed commits, if the memory past the end\nof buffer is accessible and happens to contain data matching the\nregex.\n\nMake sure that the buffer size is reduced on each iteration as the\nbuffer pointer is advanced, thus maintaining the correct end of buffer\nlocation.\n\nThe new test is flaky, I've never seen it fail on my Linux box, but\nthis is expected according to db5dfa331 (regex: -G<pattern> feeds a\nnon NUL-terminated string to regexec() and fails, 2016-09-21).  And\nbased on that commit message I would expect the new test without the\nfix to fail reliably on Windows.\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n\n diffcore-pickaxe.c      | 5 ++++-\n t/t4062-diff-pickaxe.sh | 5 +++++\n 2 files changed, 9 insertions(+), 1 deletion(-)\n\ndiff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c\nindex 9795ca1c1..03f84b714 100644\n--- a/diffcore-pickaxe.c\n+++ b/diffcore-pickaxe.c\n@@ -85,8 +85,11 @@ static unsigned int contains(mmfile_t *mf, regex_t *regexp, kwset_t kws)\n \t\t       !regexec_buf(regexp, data, sz, 1, &regmatch, flags)) {\n \t\t\tflags |= REG_NOTBOL;\n \t\t\tdata += regmatch.rm_eo;\n-\t\t\tif (*data && regmatch.rm_so == regmatch.rm_eo)\n+\t\t\tsz -= regmatch.rm_eo;\n+\t\t\tif (*data && regmatch.rm_so == regmatch.rm_eo) {\n \t\t\t\tdata++;\n+\t\t\t\tsz--;\n+\t\t\t}\n \t\t\tcnt++;\n \t\t}\n \ndiff --git a/t/t4062-diff-pickaxe.sh b/t/t4062-diff-pickaxe.sh\nindex f0bf50bda..7c4903f49 100755\n--- a/t/t4062-diff-pickaxe.sh\n+++ b/t/t4062-diff-pickaxe.sh\n@@ -19,4 +19,9 @@ test_expect_success '-G matches' '\n \ttest 4096-zeroes.txt = \"$(cat out)\"\n '\n \n+test_expect_success '-S --pickaxe-regex' '\n+\tgit diff --name-only -S0 --pickaxe-regex HEAD^ >out &&\n+\tverbose test 4096-zeroes.txt = \"$(cat out)\"\n+'\n+\n test_done\n-- \n2.12.0.377.g15f6ffe90\n\n"},{"id":"314511","messageId":"CAM0VKjknLpCyQfY+ie3sfGemwhyad3Tk-5fHdeTSz2-WTw7NoQ@mail.gmail.com","threadId":"45422","inReplyTo":"20170318151239.17196-1-szeder.dev@gmail.com","subject":"Re: [PATCH] pickaxe: fix segfault with '-S<...> --pickaxe-regex'","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2017-03-18T17:45:00Z","receivedAt":"2017-03-18T17:45:10Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Sat, Mar 18, 2017 at 4:12 PM, SZEDER Gábor <szeder.dev@gmail.com> wrote:\n> Make sure that the buffer size is reduced on each iteration as the\n> buffer pointer is advanced, thus maintaining the correct end of buffer\n> location.\n>\n> The new test is flaky, I've never seen it fail on my Linux box, but\n> this is expected according to db5dfa331 (regex: -G<pattern> feeds a\n> non NUL-terminated string to regexec() and fails, 2016-09-21).  And\n> based on that commit message I would expect the new test without the\n> fix to fail reliably on Windows.\n>\n> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n> ---\n>\n>  diffcore-pickaxe.c      | 5 ++++-\n>  t/t4062-diff-pickaxe.sh | 5 +++++\n>  2 files changed, 9 insertions(+), 1 deletion(-)\n>\n> diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c\n> index 9795ca1c1..03f84b714 100644\n> --- a/diffcore-pickaxe.c\n> +++ b/diffcore-pickaxe.c\n> @@ -85,8 +85,11 @@ static unsigned int contains(mmfile_t *mf, regex_t *regexp, kwset_t kws)\n>                        !regexec_buf(regexp, data, sz, 1, &regmatch, flags)) {\n>                         flags |= REG_NOTBOL;\n>                         data += regmatch.rm_eo;\n> -                       if (*data && regmatch.rm_so == regmatch.rm_eo)\n> +                       sz -= regmatch.rm_eo;\n> +                       if (*data && regmatch.rm_so == regmatch.rm_eo) {\n>                                 data++;\n> +                               sz--;\n> +                       }\n>                         cnt++;\n>                 }\n>\n> diff --git a/t/t4062-diff-pickaxe.sh b/t/t4062-diff-pickaxe.sh\n> index f0bf50bda..7c4903f49 100755\n> --- a/t/t4062-diff-pickaxe.sh\n> +++ b/t/t4062-diff-pickaxe.sh\n> @@ -19,4 +19,9 @@ test_expect_success '-G matches' '\n>         test 4096-zeroes.txt = \"$(cat out)\"\n>  '\n>\n> +test_expect_success '-S --pickaxe-regex' '\n> +       git diff --name-only -S0 --pickaxe-regex HEAD^ >out &&\n> +       verbose test 4096-zeroes.txt = \"$(cat out)\"\n> +'\n> +\n>  test_done\n\nHang on, this new test does fail because of a segfault _with_ the fix\non Travis 64bit Linux and OSX builds.\n\nOh, well.\n"},{"id":"314519","messageId":"20170318182408.4444-1-szeder.dev@gmail.com","threadId":"45422","inReplyTo":"CAM0VKjknLpCyQfY+ie3sfGemwhyad3Tk-5fHdeTSz2-WTw7NoQ@mail.gmail.com","subject":"[PATCHv2] pickaxe: fix segfault with '-S<...> --pickaxe-regex'","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2017-03-18T18:24:08Z","receivedAt":"2017-03-18T18:24:24Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"'git {log,diff,...} -S<...> --pickaxe-regex' can segfault as a result\nof out-of-bounds memory reads.\n\ndiffcore-pickaxe.c:contains() looks for all matches of the given regex\nin a buffer in a loop, advancing the buffer pointer to the end of the\nlast match in each iteration.  When we switched to REG_STARTEND in\nb7d36ffca (regex: use regexec_buf(), 2016-09-21), we started passing\nthe size of that buffer to the regexp engine, too.  Unfortunately,\nthis buffer size is never updated on subsequent iterations, and as the\nbuffer pointer advances on each iteration, this \"bufptr+bufsize\"\npoints past the end of the buffer.  This results in segmentation\nfault, if that memory can't be accessed.  In case of 'git log' it can\nalso result in erroneously listed commits, if the memory past the end\nof buffer is accessible and happens to contain data matching the\nregex.\n\nReduce the buffer size on each iteration as the buffer pointer is\nadvanced, thus maintaining the correct end of buffer location.\nFurthermore, make sure that the buffer pointer is not dereferenced in\nthe control flow statements when we already reached the end of the\nbuffer.\n\nThe new test is flaky, I've never seen it fail on my Linux box even\nwithout the fix, but this is expected according to db5dfa3 (regex:\n-G<pattern> feeds a non NUL-terminated string to regexec() and fails,\n2016-09-21).  However, it did fail on Travis CI with the first (and\nincomplete) version of the fix, and based on that commit message I\nwould expect the new test without the fix to fail most of the time on\nWindows.\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n\nChanges since v1:\n\n diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c\n index 03f84b714..341529b5a 100644\n --- a/diffcore-pickaxe.c\n +++ b/diffcore-pickaxe.c\n @@ -81,12 +81,12 @@ static unsigned int contains(mmfile_t *mf, regex_t *regexp, kwset_t kws)\n  \t\tregmatch_t regmatch;\n  \t\tint flags = 0;\n  \n -\t\twhile (*data &&\n +\t\twhile (sz && *data &&\n  \t\t       !regexec_buf(regexp, data, sz, 1, &regmatch, flags)) {\n  \t\t\tflags |= REG_NOTBOL;\n  \t\t\tdata += regmatch.rm_eo;\n  \t\t\tsz -= regmatch.rm_eo;\n -\t\t\tif (*data && regmatch.rm_so == regmatch.rm_eo) {\n +\t\t\tif (sz && *data && regmatch.rm_so == regmatch.rm_eo) {\n  \t\t\t\tdata++;\n  \t\t\t\tsz--;\n  \t\t\t}\n\n diffcore-pickaxe.c      | 7 +++++--\n t/t4062-diff-pickaxe.sh | 5 +++++\n 2 files changed, 10 insertions(+), 2 deletions(-)\n\ndiff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c\nindex 9795ca1c1..341529b5a 100644\n--- a/diffcore-pickaxe.c\n+++ b/diffcore-pickaxe.c\n@@ -81,12 +81,15 @@ static unsigned int contains(mmfile_t *mf, regex_t *regexp, kwset_t kws)\n \t\tregmatch_t regmatch;\n \t\tint flags = 0;\n \n-\t\twhile (*data &&\n+\t\twhile (sz && *data &&\n \t\t       !regexec_buf(regexp, data, sz, 1, &regmatch, flags)) {\n \t\t\tflags |= REG_NOTBOL;\n \t\t\tdata += regmatch.rm_eo;\n-\t\t\tif (*data && regmatch.rm_so == regmatch.rm_eo)\n+\t\t\tsz -= regmatch.rm_eo;\n+\t\t\tif (sz && *data && regmatch.rm_so == regmatch.rm_eo) {\n \t\t\t\tdata++;\n+\t\t\t\tsz--;\n+\t\t\t}\n \t\t\tcnt++;\n \t\t}\n \ndiff --git a/t/t4062-diff-pickaxe.sh b/t/t4062-diff-pickaxe.sh\nindex f0bf50bda..7c4903f49 100755\n--- a/t/t4062-diff-pickaxe.sh\n+++ b/t/t4062-diff-pickaxe.sh\n@@ -19,4 +19,9 @@ test_expect_success '-G matches' '\n \ttest 4096-zeroes.txt = \"$(cat out)\"\n '\n \n+test_expect_success '-S --pickaxe-regex' '\n+\tgit diff --name-only -S0 --pickaxe-regex HEAD^ >out &&\n+\tverbose test 4096-zeroes.txt = \"$(cat out)\"\n+'\n+\n test_done\n-- \n2.12.0.377.g15f6ffe90\n"},{"id":"314528","messageId":"xmqqefxu2yzg.fsf@gitster.mtv.corp.google.com","threadId":"45422","inReplyTo":"20170318151239.17196-1-szeder.dev@gmail.com","subject":"Re: [PATCH] pickaxe: fix segfault with '-S<...> --pickaxe-regex'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-18T18:58:43Z","receivedAt":"2017-03-18T18:58:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Interestingly, the new test fails (with the patch) under prove but\nnot when run from the shell (i.e. \"cd t && sh t4062-diff-pickaxe.sh\").\n\n"},{"id":"314534","messageId":"xmqqshma1jjt.fsf@gitster.mtv.corp.google.com","threadId":"45422","inReplyTo":"xmqqefxu2yzg.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] pickaxe: fix segfault with '-S<...> --pickaxe-regex'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-18T19:17:26Z","receivedAt":"2017-03-18T19:17:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Interestingly, the new test fails (with the patch) under prove but\n> not when run from the shell (i.e. \"cd t && sh t4062-diff-pickaxe.sh\").\n\nSorry, false alarm.\n"},{"id":"314789","messageId":"alpine.DEB.2.20.1703211244470.3767@virtualbox","threadId":"45422","inReplyTo":"20170318182408.4444-1-szeder.dev@gmail.com","subject":"Re: [PATCHv2] pickaxe: fix segfault with '-S<...> --pickaxe-regex'","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-03-21T11:46:34Z","receivedAt":"2017-03-21T11:46:58Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Gábor,\n\nOn Sat, 18 Mar 2017, SZEDER Gábor wrote:\n\n> 'git {log,diff,...} -S<...> --pickaxe-regex' can segfault as a result of\n> out-of-bounds memory reads.\n> \n> diffcore-pickaxe.c:contains() looks for all matches of the given regex\n> in a buffer in a loop, advancing the buffer pointer to the end of the\n> last match in each iteration.  When we switched to REG_STARTEND in\n> b7d36ffca (regex: use regexec_buf(), 2016-09-21), we started passing\n> the size of that buffer to the regexp engine, too.  Unfortunately,\n> this buffer size is never updated on subsequent iterations, and as the\n> buffer pointer advances on each iteration, this \"bufptr+bufsize\"\n> points past the end of the buffer.  This results in segmentation\n> fault, if that memory can't be accessed.  In case of 'git log' it can\n> also result in erroneously listed commits, if the memory past the end\n> of buffer is accessible and happens to contain data matching the\n> regex.\n> \n> Reduce the buffer size on each iteration as the buffer pointer is\n> advanced, thus maintaining the correct end of buffer location.\n> Furthermore, make sure that the buffer pointer is not dereferenced in\n> the control flow statements when we already reached the end of the\n> buffer.\n> \n> The new test is flaky, I've never seen it fail on my Linux box even\n> without the fix, but this is expected according to db5dfa3 (regex:\n> -G<pattern> feeds a non NUL-terminated string to regexec() and fails,\n> 2016-09-21).  However, it did fail on Travis CI with the first (and\n> incomplete) version of the fix, and based on that commit message I\n> would expect the new test without the fix to fail most of the time on\n> Windows.\n\nThank you for catching and fixing this. On Windows, I indeed get a\nsegmentation fault for the new test case without the patched code, and the\npatch indeed fixes this.\n\nACK,\nDscho"}]}