[PATCH] pickaxe: fix segfault with '-S<...> --pickaxe-regex'

[PATCH] pickaxe: fix segfault with '-S<...> --pickaxe-regex'

[SZEDER Gábor]

'git {log,diff,...} -S<...> --pickaxe-regex' can segfault as a result
of out-of-bounds memory reads.

diffcore-pickaxe.c:contains() looks for all matches of the given regex
in a buffer in a loop, advancing the buffer pointer to the end of the
last match in each iteration.  When we switched to REG_STARTEND in
b7d36ffca (regex: use regexec_buf(), 2016-09-21), we started passing
the size of that buffer to the regexp engine, too.  Unfortunately,
this buffer size is never updated on subsequent iterations, and as the
buffer pointer advances on each iteration, this "bufptr+bufsize"
points past the end of the buffer.  This results in segmentation
fault, if that memory can't be accessed.  In case of 'git log' it can
also result in erroneously listed commits, if the memory past the end
of buffer is accessible and happens to contain data matching the
regex.

Make sure that the buffer size is reduced on each iteration as the
buffer pointer is advanced, thus maintaining the correct end of buffer
location.

The new test is flaky, I've never seen it fail on my Linux box, but
this is expected according to db5dfa331 (regex: -G<pattern> feeds a
non NUL-terminated string to regexec() and fails, 2016-09-21).  And
based on that commit message I would expect the new test without the
fix to fail reliably on Windows.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---

 diffcore-pickaxe.c      | 5 ++++-
 t/t4062-diff-pickaxe.sh | 5 +++++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
index 9795ca1c1..03f84b714 100644
--- a/diffcore-pickaxe.c
+++ b/diffcore-pickaxe.c
@@ -85,8 +85,11 @@ static unsigned int contains(mmfile_t *mf, regex_t *regexp, kwset_t kws)
 		       !regexec_buf(regexp, data, sz, 1, &regmatch, flags)) {
 			flags |= REG_NOTBOL;
 			data += regmatch.rm_eo;
-			if (*data && regmatch.rm_so == regmatch.rm_eo)
+			sz -= regmatch.rm_eo;
+			if (*data && regmatch.rm_so == regmatch.rm_eo) {
 				data++;
+				sz--;
+			}
 			cnt++;
 		}
 
diff --git a/t/t4062-diff-pickaxe.sh b/t/t4062-diff-pickaxe.sh
index f0bf50bda..7c4903f49 100755
--- a/t/t4062-diff-pickaxe.sh
+++ b/t/t4062-diff-pickaxe.sh
@@ -19,4 +19,9 @@ test_expect_success '-G matches' '
 	test 4096-zeroes.txt = "$(cat out)"
 '
 
+test_expect_success '-S --pickaxe-regex' '
+	git diff --name-only -S0 --pickaxe-regex HEAD^ >out &&
+	verbose test 4096-zeroes.txt = "$(cat out)"
+'
+
 test_done
-- 
2.12.0.377.g15f6ffe90

Re: [PATCH] pickaxe: fix segfault with '-S<...> --pickaxe-regex'

[SZEDER Gábor]

On Sat, Mar 18, 2017 at 4:12 PM, SZEDER Gábor <szeder.dev@gmail.com> wrote:
> Make sure that the buffer size is reduced on each iteration as the
> buffer pointer is advanced, thus maintaining the correct end of buffer
> location.
>
> The new test is flaky, I've never seen it fail on my Linux box, but
> this is expected according to db5dfa331 (regex: -G<pattern> feeds a
> non NUL-terminated string to regexec() and fails, 2016-09-21).  And
> based on that commit message I would expect the new test without the
> fix to fail reliably on Windows.
>
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
>
>  diffcore-pickaxe.c      | 5 ++++-
>  t/t4062-diff-pickaxe.sh | 5 +++++
>  2 files changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
> index 9795ca1c1..03f84b714 100644
> --- a/diffcore-pickaxe.c
> +++ b/diffcore-pickaxe.c
> @@ -85,8 +85,11 @@ static unsigned int contains(mmfile_t *mf, regex_t *regexp, kwset_t kws)
>                        !regexec_buf(regexp, data, sz, 1, &regmatch, flags)) {
>                         flags |= REG_NOTBOL;
>                         data += regmatch.rm_eo;
> -                       if (*data && regmatch.rm_so == regmatch.rm_eo)
> +                       sz -= regmatch.rm_eo;
> +                       if (*data && regmatch.rm_so == regmatch.rm_eo) {
>                                 data++;
> +                               sz--;
> +                       }
>                         cnt++;
>                 }
>
> diff --git a/t/t4062-diff-pickaxe.sh b/t/t4062-diff-pickaxe.sh
> index f0bf50bda..7c4903f49 100755
> --- a/t/t4062-diff-pickaxe.sh
> +++ b/t/t4062-diff-pickaxe.sh
> @@ -19,4 +19,9 @@ test_expect_success '-G matches' '
>         test 4096-zeroes.txt = "$(cat out)"
>  '
>
> +test_expect_success '-S --pickaxe-regex' '
> +       git diff --name-only -S0 --pickaxe-regex HEAD^ >out &&
> +       verbose test 4096-zeroes.txt = "$(cat out)"
> +'
> +
>  test_done

Hang on, this new test does fail because of a segfault _with_ the fix
on Travis 64bit Linux and OSX builds.

Oh, well.

[PATCHv2] pickaxe: fix segfault with '-S<...> --pickaxe-regex'

[SZEDER Gábor]

'git {log,diff,...} -S<...> --pickaxe-regex' can segfault as a result
of out-of-bounds memory reads.

diffcore-pickaxe.c:contains() looks for all matches of the given regex
in a buffer in a loop, advancing the buffer pointer to the end of the
last match in each iteration.  When we switched to REG_STARTEND in
b7d36ffca (regex: use regexec_buf(), 2016-09-21), we started passing
the size of that buffer to the regexp engine, too.  Unfortunately,
this buffer size is never updated on subsequent iterations, and as the
buffer pointer advances on each iteration, this "bufptr+bufsize"
points past the end of the buffer.  This results in segmentation
fault, if that memory can't be accessed.  In case of 'git log' it can
also result in erroneously listed commits, if the memory past the end
of buffer is accessible and happens to contain data matching the
regex.

Reduce the buffer size on each iteration as the buffer pointer is
advanced, thus maintaining the correct end of buffer location.
Furthermore, make sure that the buffer pointer is not dereferenced in
the control flow statements when we already reached the end of the
buffer.

The new test is flaky, I've never seen it fail on my Linux box even
without the fix, but this is expected according to db5dfa3 (regex:
-G<pattern> feeds a non NUL-terminated string to regexec() and fails,
2016-09-21).  However, it did fail on Travis CI with the first (and
incomplete) version of the fix, and based on that commit message I
would expect the new test without the fix to fail most of the time on
Windows.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---

Changes since v1:

 diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
 index 03f84b714..341529b5a 100644
 --- a/diffcore-pickaxe.c
 +++ b/diffcore-pickaxe.c
 @@ -81,12 +81,12 @@ static unsigned int contains(mmfile_t *mf, regex_t *regexp, kwset_t kws)
  		regmatch_t regmatch;
  		int flags = 0;
  
 -		while (*data &&
 +		while (sz && *data &&
  		       !regexec_buf(regexp, data, sz, 1, &regmatch, flags)) {
  			flags |= REG_NOTBOL;
  			data += regmatch.rm_eo;
  			sz -= regmatch.rm_eo;
 -			if (*data && regmatch.rm_so == regmatch.rm_eo) {
 +			if (sz && *data && regmatch.rm_so == regmatch.rm_eo) {
  				data++;
  				sz--;
  			}

 diffcore-pickaxe.c      | 7 +++++--
 t/t4062-diff-pickaxe.sh | 5 +++++
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
index 9795ca1c1..341529b5a 100644
--- a/diffcore-pickaxe.c
+++ b/diffcore-pickaxe.c
@@ -81,12 +81,15 @@ static unsigned int contains(mmfile_t *mf, regex_t *regexp, kwset_t kws)
 		regmatch_t regmatch;
 		int flags = 0;
 
-		while (*data &&
+		while (sz && *data &&
 		       !regexec_buf(regexp, data, sz, 1, &regmatch, flags)) {
 			flags |= REG_NOTBOL;
 			data += regmatch.rm_eo;
-			if (*data && regmatch.rm_so == regmatch.rm_eo)
+			sz -= regmatch.rm_eo;
+			if (sz && *data && regmatch.rm_so == regmatch.rm_eo) {
 				data++;
+				sz--;
+			}
 			cnt++;
 		}
 
diff --git a/t/t4062-diff-pickaxe.sh b/t/t4062-diff-pickaxe.sh
index f0bf50bda..7c4903f49 100755
--- a/t/t4062-diff-pickaxe.sh
+++ b/t/t4062-diff-pickaxe.sh
@@ -19,4 +19,9 @@ test_expect_success '-G matches' '
 	test 4096-zeroes.txt = "$(cat out)"
 '
 
+test_expect_success '-S --pickaxe-regex' '
+	git diff --name-only -S0 --pickaxe-regex HEAD^ >out &&
+	verbose test 4096-zeroes.txt = "$(cat out)"
+'
+
 test_done
-- 
2.12.0.377.g15f6ffe90

Re: [PATCH] pickaxe: fix segfault with '-S<...> --pickaxe-regex'

[Junio C Hamano]

Interestingly, the new test fails (with the patch) under prove but
not when run from the shell (i.e. "cd t && sh t4062-diff-pickaxe.sh").

Re: [PATCH] pickaxe: fix segfault with '-S<...> --pickaxe-regex'

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Interestingly, the new test fails (with the patch) under prove but
> not when run from the shell (i.e. "cd t && sh t4062-diff-pickaxe.sh").

Sorry, false alarm.

Re: [PATCHv2] pickaxe: fix segfault with '-S<...> --pickaxe-regex'

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 1885 bytes --]

Hi Gábor,

On Sat, 18 Mar 2017, SZEDER Gábor wrote:

> 'git {log,diff,...} -S<...> --pickaxe-regex' can segfault as a result of
> out-of-bounds memory reads.
> 
> diffcore-pickaxe.c:contains() looks for all matches of the given regex
> in a buffer in a loop, advancing the buffer pointer to the end of the
> last match in each iteration.  When we switched to REG_STARTEND in
> b7d36ffca (regex: use regexec_buf(), 2016-09-21), we started passing
> the size of that buffer to the regexp engine, too.  Unfortunately,
> this buffer size is never updated on subsequent iterations, and as the
> buffer pointer advances on each iteration, this "bufptr+bufsize"
> points past the end of the buffer.  This results in segmentation
> fault, if that memory can't be accessed.  In case of 'git log' it can
> also result in erroneously listed commits, if the memory past the end
> of buffer is accessible and happens to contain data matching the
> regex.
> 
> Reduce the buffer size on each iteration as the buffer pointer is
> advanced, thus maintaining the correct end of buffer location.
> Furthermore, make sure that the buffer pointer is not dereferenced in
> the control flow statements when we already reached the end of the
> buffer.
> 
> The new test is flaky, I've never seen it fail on my Linux box even
> without the fix, but this is expected according to db5dfa3 (regex:
> -G<pattern> feeds a non NUL-terminated string to regexec() and fails,
> 2016-09-21).  However, it did fail on Travis CI with the first (and
> incomplete) version of the fix, and based on that commit message I
> would expect the new test without the fix to fail most of the time on
> Windows.

Thank you for catching and fixing this. On Windows, I indeed get a
segmentation fault for the new test case without the patched code, and the
patch indeed fixes this.

ACK,
Dscho