threads / patch / 47296

patch, 2 partstest-lib: add LIBPCRE1 & LIBPCRE2 prerequisites

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

## tl;dr

9 messages between Nov 22, 2017 and Nov 23, 2017. Diffs are folded; open one to read it.

replies: 8people: 4as markdown or json

Ævar Arnfjörð Bjarmason· Nov 22, 2017, 13:36 UTC · lore

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(+)
Show changes to 2 files +14 −0

t/README, t/test-lib.sh

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
Ævar Arnfjörð Bjarmason· Nov 22, 2017, 13:36 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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(+)
Show changes to 2 files +31 −0

grep.c, t/t7810-grep.sh

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
Eric Sunshine· Nov 22, 2017, 18:18 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

On Wed, Nov 22, 2017 at 8:36 AM, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 9 quoted lines
> 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,/

Show 10 quoted lines
> 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>
Ævar Arnfjörð Bjarmason· Nov 23, 2017, 09:15 UTC · re: Eric Sunshine · lore

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

On Wed, Nov 22 2017, Eric Sunshine jotted:
Show 14 quoted lines
> 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.

Show 10 quoted lines
>> 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>
Simon Ruderich· Nov 23, 2017, 09:10 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

On Wed, Nov 22, 2017 at 01:36:30PM +0000, Ævar Arnfjörð Bjarmason wrote:
Show 11 quoted lines
> +		 *
> +		 * 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
Ævar Arnfjörð Bjarmason· Nov 23, 2017, 14:16 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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(+)
Show changes to 2 files +14 −0

t/README, t/test-lib.sh

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
Ævar Arnfjörð Bjarmason· Nov 23, 2017, 14:16 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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(+)
Show changes to 2 files +32 −0

grep.c, t/t7810-grep.sh

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
Jonathan Nieder· Nov 22, 2017, 20:22 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

Hi,
Ævar Arnfjörð Bjarmason wrote:
Show 27 quoted lines
> 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

Ævar Arnfjörð Bjarmason· Nov 23, 2017, 09:11 UTC · re: Jonathan Nieder · lore

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

On Wed, Nov 22 2017, Jonathan Nieder jotted:
Show 38 quoted lines
> 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).

← back to recent threads