[PATCH 1/2] test-lib: add LIBPCRE1 & LIBPCRE2 prerequisites

[PATCH 1/2] test-lib: add LIBPCRE1 & LIBPCRE2 prerequisites

[Ævar Arnfjörð Bjarmason]

Add LIBPCRE1 and LIBPCRE2 prerequisites which are true when git is
compiled with USE_LIBPCRE1=YesPlease or USE_LIBPCRE2=YesPlease,
respectively.

The syntax of PCRE1 and PCRE2 isn't the same in all cases (see
pcresyntax(3) and pcre2syntax(3)). If test are added that test for
those they'll need to be guarded by these new prerequisites.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/README      | 12 ++++++++++++
 t/test-lib.sh |  2 ++
 2 files changed, 14 insertions(+)

diff --git a/t/README b/t/README
index 4b079e4494..599cd9808c 100644
--- a/t/README
+++ b/t/README
@@ -808,6 +808,18 @@ use these, and "test_set_prereq" for how to define your own.
    Git was compiled with support for PCRE. Wrap any tests
    that use git-grep --perl-regexp or git-grep -P in these.
 
+ - LIBPCRE1
+
+   Git was compiled with PCRE v1 support via
+   USE_LIBPCRE1=YesPlease. Wrap any PCRE using tests that for some
+   reason need v1 of the PCRE library instead of v2 in these.
+
+ - LIBPCRE2
+
+   Git was compiled with PCRE v2 support via
+   USE_LIBPCRE2=YesPlease. Wrap any PCRE using tests that for some
+   reason need v2 of the PCRE library instead of v1 in these.
+
  - CASE_INSENSITIVE_FS
 
    Test is run on a case insensitive file system.
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 116bd6a70c..e7065df2bb 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1028,6 +1028,8 @@ test -z "$NO_PERL" && test_set_prereq PERL
 test -z "$NO_PTHREADS" && test_set_prereq PTHREADS
 test -z "$NO_PYTHON" && test_set_prereq PYTHON
 test -n "$USE_LIBPCRE1$USE_LIBPCRE2" && test_set_prereq PCRE
+test -n "$USE_LIBPCRE1" && test_set_prereq LIBPCRE1
+test -n "$USE_LIBPCRE2" && test_set_prereq LIBPCRE2
 test -z "$NO_GETTEXT" && test_set_prereq GETTEXT
 
 # Can we rely on git's output in the C locale?
-- 
2.15.0.403.gc27cc4dac6

[PATCH 2/2] grep: fix segfault under -P + PCRE2 + (*NO_JIT)

[Ævar Arnfjörð Bjarmason]

Fix a bug in the compilation of PCRE2 patterns under JIT (the most
common runtime configuration), any pattern with a (*NO_JIT) verb would
segfault. This bug dates back to my 94da9193a6 ("grep: add support for
PCRE v2", 2017-06-01):

    $ git grep -P '(*NO_JIT)hi.*there'
    Segmentation fault

As explained ad more length in the comment being added here it isn't
sufficient to just check pcre2_config() to see whether the JIT should
be used, pcre2_pattern_info() also has to be asked.

This is something I discovered myself when fiddling around with PCRE2
verbs in patterns passed to git. I don't expect that any user of git
has encountered this given the obscurity of passing PCRE2 verbs
through to the library, along with the relative obscurity of (*NO_JIT)
itself.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 grep.c          | 25 +++++++++++++++++++++++++
 t/t7810-grep.sh |  6 ++++++
 2 files changed, 31 insertions(+)

diff --git a/grep.c b/grep.c
index d0b9b6cdfa..f3139e867c 100644
--- a/grep.c
+++ b/grep.c
@@ -477,6 +477,8 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
 	int options = PCRE2_MULTILINE;
 	const uint8_t *character_tables = NULL;
 	int jitret;
+	int patinforet;
+	size_t jitsizearg;
 
 	assert(opt->pcre2);
 
@@ -511,6 +513,29 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
 		jitret = pcre2_jit_compile(p->pcre2_pattern, PCRE2_JIT_COMPLETE);
 		if (jitret)
 			die("Couldn't JIT the PCRE2 pattern '%s', got '%d'\n", p->pattern, jitret);
+
+		/*
+		 * The pcre2_config(PCRE2_CONFIG_JIT, ...) call just
+		 * tells us whether the library itself supports JIT,
+		 * but to see whether we're going to be actually using
+		 * JIT we need to extract PCRE2_INFO_JITSIZE from the
+		 * pattern *after* we do pcre2_jit_compile() above.
+		 *
+		 * This is because if the pattern contains the
+		 * (*NO_JIT) verb (see pcre2syntax(3))
+		 * pcre2_jit_compile() will exit early with 0. If we
+		 * then proceed to call pcre2_jit_match() further down
+		 * the line instead of pcre2_match() we'll segfault.
+		 */
+		patinforet = pcre2_pattern_info(p->pcre2_pattern, PCRE2_INFO_JITSIZE, &jitsizearg);
+		if (patinforet)
+			die("BUG: The patinforet variable should be 0 after the pcre2_pattern_info() call, not %d",
+			    patinforet);
+		if (jitsizearg == 0) {
+			p->pcre2_jit_on = 0;
+			return;
+		}
+
 		p->pcre2_jit_stack = pcre2_jit_stack_create(1, 1024 * 1024, NULL);
 		if (!p->pcre2_jit_stack)
 			die("Couldn't allocate PCRE2 JIT stack");
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 2a6679c2f5..c8ff50cc30 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -1110,6 +1110,12 @@ test_expect_success PCRE 'grep -P pattern' '
 	test_cmp expected actual
 '
 
+test_expect_success LIBPCRE2 "grep -P with (*NO_JIT) doesn't error out" '
+	git grep -P "(*NO_JIT)\p{Ps}.*?\p{Pe}" hello.c >actual &&
+	test_cmp expected actual
+
+'
+
 test_expect_success !PCRE 'grep -P pattern errors without PCRE' '
 	test_must_fail git grep -P "foo.*bar"
 '
-- 
2.15.0.403.gc27cc4dac6

Re: [PATCH 2/2] grep: fix segfault under -P + PCRE2 + (*NO_JIT)

[Eric Sunshine]

On Wed, Nov 22, 2017 at 8:36 AM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> Fix a bug in the compilation of PCRE2 patterns under JIT (the most
> common runtime configuration), any pattern with a (*NO_JIT) verb would
> segfault. This bug dates back to my 94da9193a6 ("grep: add support for
> PCRE v2", 2017-06-01):
>
>     $ git grep -P '(*NO_JIT)hi.*there'
>     Segmentation fault
>
> As explained ad more length in the comment being added here it isn't

s/ad/at/
s/here/here,/

> sufficient to just check pcre2_config() to see whether the JIT should
> be used, pcre2_pattern_info() also has to be asked.
>
> This is something I discovered myself when fiddling around with PCRE2
> verbs in patterns passed to git. I don't expect that any user of git
> has encountered this given the obscurity of passing PCRE2 verbs
> through to the library, along with the relative obscurity of (*NO_JIT)
> itself.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

Re: [PATCH 2/2] grep: fix segfault under -P + PCRE2 + (*NO_JIT)

[Ævar Arnfjörð Bjarmason]

On Wed, Nov 22 2017, Eric Sunshine jotted:

> On Wed, Nov 22, 2017 at 8:36 AM, Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>> Fix a bug in the compilation of PCRE2 patterns under JIT (the most
>> common runtime configuration), any pattern with a (*NO_JIT) verb would
>> segfault. This bug dates back to my 94da9193a6 ("grep: add support for
>> PCRE v2", 2017-06-01):
>>
>>     $ git grep -P '(*NO_JIT)hi.*there'
>>     Segmentation fault
>>
>> As explained ad more length in the comment being added here it isn't
>
> s/ad/at/
> s/here/here,/

Thanks. I'll let this sit for a bit and submit a v2 soon. There's also
an upstream fix in pcre2 to prevent the segfault that'll be in future
versions & I'm going to note in the amended commit message.

>> sufficient to just check pcre2_config() to see whether the JIT should
>> be used, pcre2_pattern_info() also has to be asked.
>>
>> This is something I discovered myself when fiddling around with PCRE2
>> verbs in patterns passed to git. I don't expect that any user of git
>> has encountered this given the obscurity of passing PCRE2 verbs
>> through to the library, along with the relative obscurity of (*NO_JIT)
>> itself.
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

Re: [PATCH 2/2] grep: fix segfault under -P + PCRE2 + (*NO_JIT)

[Simon Ruderich]

On Wed, Nov 22, 2017 at 01:36:30PM +0000, Ævar Arnfjörð Bjarmason wrote:
> +		 *
> +		 * This is because if the pattern contains the
> +		 * (*NO_JIT) verb (see pcre2syntax(3))
> +		 * pcre2_jit_compile() will exit early with 0. If we
> +		 * then proceed to call pcre2_jit_match() further down
> +		 * the line instead of pcre2_match() we'll segfault.
> +		 */
> +		patinforet = pcre2_pattern_info(p->pcre2_pattern, PCRE2_INFO_JITSIZE, &jitsizearg);
> +		if (patinforet)
> +			die("BUG: The patinforet variable should be 0 after the pcre2_pattern_info() call, not %d",
> +			    patinforet);

I think BUG() should be used here, and maybe shorten the error
message:

    BUG("pcre2_pattern_info() failed: %d", patinforet);

Regards
Simon
-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9

[PATCH v2 1/2] test-lib: add LIBPCRE1 & LIBPCRE2 prerequisites

[Ævar Arnfjörð Bjarmason]

Add LIBPCRE1 and LIBPCRE2 prerequisites which are true when git is
compiled with USE_LIBPCRE1=YesPlease or USE_LIBPCRE2=YesPlease,
respectively.

The syntax of PCRE1 and PCRE2 isn't the same in all cases (see
pcresyntax(3) and pcre2syntax(3)). If test are added that test for
those they'll need to be guarded by these new prerequisites.

The subsequent patch will make use of LIBPCRE2, so LIBPCRE1 isn't
strictly needed for now, but let's add it for consistency and so that
checking for it doesn't have to be done with the less obvious "PCRE,
!LIBPCRE2", which while semantically the same is more confusing, and
would lead to bugs if PCRE v3 is ever released as the tests would mean
v1, not any non-v2 version.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

As noted on-list I changed nothing here in the code, but noted in the
commit message why I'm keeping LIBPCRE1.

 t/README      | 12 ++++++++++++
 t/test-lib.sh |  2 ++
 2 files changed, 14 insertions(+)

diff --git a/t/README b/t/README
index 4b079e4494..599cd9808c 100644
--- a/t/README
+++ b/t/README
@@ -808,6 +808,18 @@ use these, and "test_set_prereq" for how to define your own.
    Git was compiled with support for PCRE. Wrap any tests
    that use git-grep --perl-regexp or git-grep -P in these.
 
+ - LIBPCRE1
+
+   Git was compiled with PCRE v1 support via
+   USE_LIBPCRE1=YesPlease. Wrap any PCRE using tests that for some
+   reason need v1 of the PCRE library instead of v2 in these.
+
+ - LIBPCRE2
+
+   Git was compiled with PCRE v2 support via
+   USE_LIBPCRE2=YesPlease. Wrap any PCRE using tests that for some
+   reason need v2 of the PCRE library instead of v1 in these.
+
  - CASE_INSENSITIVE_FS
 
    Test is run on a case insensitive file system.
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 116bd6a70c..e7065df2bb 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1028,6 +1028,8 @@ test -z "$NO_PERL" && test_set_prereq PERL
 test -z "$NO_PTHREADS" && test_set_prereq PTHREADS
 test -z "$NO_PYTHON" && test_set_prereq PYTHON
 test -n "$USE_LIBPCRE1$USE_LIBPCRE2" && test_set_prereq PCRE
+test -n "$USE_LIBPCRE1" && test_set_prereq LIBPCRE1
+test -n "$USE_LIBPCRE2" && test_set_prereq LIBPCRE2
 test -z "$NO_GETTEXT" && test_set_prereq GETTEXT
 
 # Can we rely on git's output in the C locale?
-- 
2.15.0.403.gc27cc4dac6

[PATCH v2 2/2] grep: fix segfault under -P + PCRE2 <=10.30 + (*NO_JIT)

[Ævar Arnfjörð Bjarmason]

Fix a bug in the compilation of PCRE2 patterns under JIT (the most
common runtime configuration). Any pattern with a (*NO_JIT) verb would
segfault in any currently released PCRE2 version:

    $ git grep -P '(*NO_JIT)hi.*there'
    Segmentation fault

That this segfaulted was a bug in PCRE2 itself, after reporting it[1]
on pcre-dev it's been fixed in a yet-to-be-released version of
PCRE (presumably released first as 10.31). Now it'll die with:

    $ git grep -P '(*NO_JIT)hi.*there'
    fatal: pcre2_jit_match failed with error code -45: bad JIT option

But the cause of the bug is in our own code dating back to my
94da9193a6 ("grep: add support for PCRE v2", 2017-06-01).

As explained at more length in the comment being added here, it isn't
sufficient to just check pcre2_config() to see whether the JIT should
be used, pcre2_pattern_info() also has to be asked.

This is something I discovered myself when fiddling around with PCRE2
verbs in patterns passed to git. I don't expect that any user of git
has encountered this given the obscurity of passing PCRE2 verbs
through to the library, along with the relative obscurity of (*NO_JIT)
itself.

1. "How am I supposed to use PCRE2 JIT in the face of (*NO_JIT) ?"
   (<CACBZZX5mMqDuWuFmi7sRBp3wH6CFyd-ghACukd=v0NN=rBMnJg@mail.gmail.com> &
    https://lists.exim.org/lurker/thread/20171123.101502.7f0d38ca.en.html)
   on the pcre-dev mailing list

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

Incorporates feedback from Eric & Simon. Thanks both. I also amended
the commit message / comment to note that this was also a bug in PCRE2
upstream, which has been fixed after I reported it.

 grep.c          | 26 ++++++++++++++++++++++++++
 t/t7810-grep.sh |  6 ++++++
 2 files changed, 32 insertions(+)

diff --git a/grep.c b/grep.c
index d0b9b6cdfa..e8ae0b5d8f 100644
--- a/grep.c
+++ b/grep.c
@@ -477,6 +477,8 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
 	int options = PCRE2_MULTILINE;
 	const uint8_t *character_tables = NULL;
 	int jitret;
+	int patinforet;
+	size_t jitsizearg;
 
 	assert(opt->pcre2);
 
@@ -511,6 +513,30 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
 		jitret = pcre2_jit_compile(p->pcre2_pattern, PCRE2_JIT_COMPLETE);
 		if (jitret)
 			die("Couldn't JIT the PCRE2 pattern '%s', got '%d'\n", p->pattern, jitret);
+
+		/*
+		 * The pcre2_config(PCRE2_CONFIG_JIT, ...) call just
+		 * tells us whether the library itself supports JIT,
+		 * but to see whether we're going to be actually using
+		 * JIT we need to extract PCRE2_INFO_JITSIZE from the
+		 * pattern *after* we do pcre2_jit_compile() above.
+		 *
+		 * This is because if the pattern contains the
+		 * (*NO_JIT) verb (see pcre2syntax(3))
+		 * pcre2_jit_compile() will exit early with 0. If we
+		 * then proceed to call pcre2_jit_match() further down
+		 * the line instead of pcre2_match() we'll either
+		 * segfault (pre PCRE 10.31) or run into a fatal error
+		 * (post PCRE2 10.31)
+		 */
+		patinforet = pcre2_pattern_info(p->pcre2_pattern, PCRE2_INFO_JITSIZE, &jitsizearg);
+		if (patinforet)
+			BUG("pcre2_pattern_info() failed: %d", patinforet);
+		if (jitsizearg == 0) {
+			p->pcre2_jit_on = 0;
+			return;
+		}
+
 		p->pcre2_jit_stack = pcre2_jit_stack_create(1, 1024 * 1024, NULL);
 		if (!p->pcre2_jit_stack)
 			die("Couldn't allocate PCRE2 JIT stack");
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 2a6679c2f5..c8ff50cc30 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -1110,6 +1110,12 @@ test_expect_success PCRE 'grep -P pattern' '
 	test_cmp expected actual
 '
 
+test_expect_success LIBPCRE2 "grep -P with (*NO_JIT) doesn't error out" '
+	git grep -P "(*NO_JIT)\p{Ps}.*?\p{Pe}" hello.c >actual &&
+	test_cmp expected actual
+
+'
+
 test_expect_success !PCRE 'grep -P pattern errors without PCRE' '
 	test_must_fail git grep -P "foo.*bar"
 '
-- 
2.15.0.403.gc27cc4dac6

Re: [PATCH 1/2] test-lib: add LIBPCRE1 & LIBPCRE2 prerequisites

[Jonathan Nieder]

Hi,

Ævar Arnfjörð Bjarmason wrote:

> Add LIBPCRE1 and LIBPCRE2 prerequisites which are true when git is
> compiled with USE_LIBPCRE1=YesPlease or USE_LIBPCRE2=YesPlease,
> respectively.
>
> The syntax of PCRE1 and PCRE2 isn't the same in all cases (see
> pcresyntax(3) and pcre2syntax(3)). If test are added that test for
> those they'll need to be guarded by these new prerequisites.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  t/README      | 12 ++++++++++++
>  t/test-lib.sh |  2 ++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/t/README b/t/README
> index 4b079e4494..599cd9808c 100644
> --- a/t/README
> +++ b/t/README
> @@ -808,6 +808,18 @@ use these, and "test_set_prereq" for how to define your own.
>     Git was compiled with support for PCRE. Wrap any tests
>     that use git-grep --perl-regexp or git-grep -P in these.
>  
> + - LIBPCRE1
> +
> +   Git was compiled with PCRE v1 support via
> +   USE_LIBPCRE1=YesPlease. Wrap any PCRE using tests that for some
> +   reason need v1 of the PCRE library instead of v2 in these.

Are there plans to use the LIBPCRE1 prereq?  It might be simpler to
only have LIBPCRE2, and LIBPCRE1 can still be expressed as

	PCRE,!LIBPCRE2

which I think is clearer about the intent.

Thanks,
Jonathan

Re: [PATCH 1/2] test-lib: add LIBPCRE1 & LIBPCRE2 prerequisites

[Ævar Arnfjörð Bjarmason]

On Wed, Nov 22 2017, Jonathan Nieder jotted:

> Hi,
>
> Ævar Arnfjörð Bjarmason wrote:
>
>> Add LIBPCRE1 and LIBPCRE2 prerequisites which are true when git is
>> compiled with USE_LIBPCRE1=YesPlease or USE_LIBPCRE2=YesPlease,
>> respectively.
>>
>> The syntax of PCRE1 and PCRE2 isn't the same in all cases (see
>> pcresyntax(3) and pcre2syntax(3)). If test are added that test for
>> those they'll need to be guarded by these new prerequisites.
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>  t/README      | 12 ++++++++++++
>>  t/test-lib.sh |  2 ++
>>  2 files changed, 14 insertions(+)
>>
>> diff --git a/t/README b/t/README
>> index 4b079e4494..599cd9808c 100644
>> --- a/t/README
>> +++ b/t/README
>> @@ -808,6 +808,18 @@ use these, and "test_set_prereq" for how to define your own.
>>     Git was compiled with support for PCRE. Wrap any tests
>>     that use git-grep --perl-regexp or git-grep -P in these.
>>
>> + - LIBPCRE1
>> +
>> +   Git was compiled with PCRE v1 support via
>> +   USE_LIBPCRE1=YesPlease. Wrap any PCRE using tests that for some
>> +   reason need v1 of the PCRE library instead of v2 in these.
>
> Are there plans to use the LIBPCRE1 prereq?  It might be simpler to
> only have LIBPCRE2, and LIBPCRE1 can still be expressed as
>
> 	PCRE,!LIBPCRE2
>
> which I think is clearer about the intent.

I prefer to keep it as it is. It's more obvious to me to have a 1=1
mapping between the ${USE,NO}_* variables and the prerequisites, and
it's future-proof if there's ever a PCRE v3, since tests that use this
will mean v1 specifically, not just any non-v2 version (although now v1
is the only one).