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.

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

index 4b079e4494..599cd9808c 100644
@@ -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.

index 116bd6a70c..e7065df2bb 100644
@@ -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

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.

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

index d0b9b6cdfa..f3139e867c 100644
@@ -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");

index 2a6679c2f5..c8ff50cc30 100755
@@ -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

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

On Wed, Nov 22 2017, Eric Sunshine jotted:

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.

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.

---

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(+)

index 4b079e4494..599cd9808c 100644
@@ -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.

index 116bd6a70c..e7065df2bb 100644
@@ -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

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

---

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(+)

index d0b9b6cdfa..e8ae0b5d8f 100644
@@ -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");

index 2a6679c2f5..c8ff50cc30 100755
@@ -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

Hi,

On Wed, Nov 22 2017, Jonathan Nieder jotted:

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).