[PATCH] Fix NO_LIBPCRE1_JIT to fully disable JIT

[PATCH] Fix NO_LIBPCRE1_JIT to fully disable JIT

[Charles Bailey]

From: Charles Bailey <cbailey32@bloomberg.net>

If you have a pcre1 library which is compiled with JIT enabled then
PCRE_STUDY_JIT_COMPILE will be defined whether or not the
NO_LIBPCRE1_JIT configuration is set.

This means that we enable JIT functionality when calling pcre_study
even if NO_LIBPCRE1_JIT has been explicitly set and we just use plain
pcre_exec later.

Fix this by using own macro (GIT_PCRE_STUDY_JIT_COMPILE) which we set to
PCRE_STUDY_JIT_COMPILE only if NO_LIBPCRE1_JIT is not set and define to
0 otherwise, as before.
---

I was bisecting an issue with the PCRE support that was causing a test
suite failure on our Solaris builds and reached fbaceaac47 ("grep: add
support for the PCRE v1 JIT API"). It appeared to be a misaligned memory
access somewhere inside the libpcre code. I tried disabling the use of
JIT with NO_LIBPCRE1_JIT but it turned out that even with this set we
were still triggering the JIT code path in the call to pcre_study.

Yes, we probably should fix our PCRE1 library build on Solaris or move
to PCRE2, but really NO_LIBPCRE1_JIT should have prevented us from
triggering this crash.

 grep.c | 2 +-
 grep.h | 5 +++--
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/grep.c b/grep.c
index ce6a48e..d0b9b6c 100644
--- a/grep.c
+++ b/grep.c
@@ -387,7 +387,7 @@ static void compile_pcre1_regexp(struct grep_pat *p, const struct grep_opt *opt)
 	if (!p->pcre1_regexp)
 		compile_regexp_failed(p, error);
 
-	p->pcre1_extra_info = pcre_study(p->pcre1_regexp, PCRE_STUDY_JIT_COMPILE, &error);
+	p->pcre1_extra_info = pcre_study(p->pcre1_regexp, GIT_PCRE_STUDY_JIT_COMPILE, &error);
 	if (!p->pcre1_extra_info && error)
 		die("%s", error);
 
diff --git a/grep.h b/grep.h
index 52aecfa..399381c 100644
--- a/grep.h
+++ b/grep.h
@@ -7,11 +7,12 @@
 #if PCRE_MAJOR >= 8 && PCRE_MINOR >= 32
 #ifndef NO_LIBPCRE1_JIT
 #define GIT_PCRE1_USE_JIT
+#define GIT_PCRE_STUDY_JIT_COMPILE PCRE_STUDY_JIT_COMPILE
 #endif
 #endif
 #endif
-#ifndef PCRE_STUDY_JIT_COMPILE
-#define PCRE_STUDY_JIT_COMPILE 0
+#ifndef GIT_PCRE_STUDY_JIT_COMPILE
+#define GIT_PCRE_STUDY_JIT_COMPILE 0
 #endif
 #if PCRE_MAJOR <= 8 && PCRE_MINOR < 20
 typedef int pcre_jit_stack;
-- 
2.10.2

Re: [PATCH] Fix NO_LIBPCRE1_JIT to fully disable JIT

[Ævar Arnfjörð Bjarmason]

On Sun, Nov 12 2017, Charles Bailey jotted:

> From: Charles Bailey <cbailey32@bloomberg.net>
>
> If you have a pcre1 library which is compiled with JIT enabled then
> PCRE_STUDY_JIT_COMPILE will be defined whether or not the
> NO_LIBPCRE1_JIT configuration is set.
>
> This means that we enable JIT functionality when calling pcre_study
> even if NO_LIBPCRE1_JIT has been explicitly set and we just use plain
> pcre_exec later.
>
> Fix this by using own macro (GIT_PCRE_STUDY_JIT_COMPILE) which we set to
> PCRE_STUDY_JIT_COMPILE only if NO_LIBPCRE1_JIT is not set and define to
> 0 otherwise, as before.
> ---
>
> I was bisecting an issue with the PCRE support that was causing a test
> suite failure on our Solaris builds and reached fbaceaac47 ("grep: add
> support for the PCRE v1 JIT API"). It appeared to be a misaligned memory
> access somewhere inside the libpcre code. I tried disabling the use of
> JIT with NO_LIBPCRE1_JIT but it turned out that even with this set we
> were still triggering the JIT code path in the call to pcre_study.
>
> Yes, we probably should fix our PCRE1 library build on Solaris or move
> to PCRE2, but really NO_LIBPCRE1_JIT should have prevented us from
> triggering this crash.
>
>  grep.c | 2 +-
>  grep.h | 5 +++--
>  2 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/grep.c b/grep.c
> index ce6a48e..d0b9b6c 100644
> --- a/grep.c
> +++ b/grep.c
> @@ -387,7 +387,7 @@ static void compile_pcre1_regexp(struct grep_pat *p, const struct grep_opt *opt)
>  	if (!p->pcre1_regexp)
>  		compile_regexp_failed(p, error);
>
> -	p->pcre1_extra_info = pcre_study(p->pcre1_regexp, PCRE_STUDY_JIT_COMPILE, &error);
> +	p->pcre1_extra_info = pcre_study(p->pcre1_regexp, GIT_PCRE_STUDY_JIT_COMPILE, &error);
>  	if (!p->pcre1_extra_info && error)
>  		die("%s", error);
>
> diff --git a/grep.h b/grep.h
> index 52aecfa..399381c 100644
> --- a/grep.h
> +++ b/grep.h
> @@ -7,11 +7,12 @@
>  #if PCRE_MAJOR >= 8 && PCRE_MINOR >= 32
>  #ifndef NO_LIBPCRE1_JIT
>  #define GIT_PCRE1_USE_JIT
> +#define GIT_PCRE_STUDY_JIT_COMPILE PCRE_STUDY_JIT_COMPILE
>  #endif
>  #endif
>  #endif
> -#ifndef PCRE_STUDY_JIT_COMPILE
> -#define PCRE_STUDY_JIT_COMPILE 0
> +#ifndef GIT_PCRE_STUDY_JIT_COMPILE
> +#define GIT_PCRE_STUDY_JIT_COMPILE 0
>  #endif
>  #if PCRE_MAJOR <= 8 && PCRE_MINOR < 20
>  typedef int pcre_jit_stack;

[CC-ing Junio]

Thanks a lot. This patch looks good to me.

I could have sworn I was handling this already, but looking at this now
I wasn't really.

However, as a bit of extra info I *did* test this, and it works just
fine for me, i.e. if I compile PCRE 8.32 now (as I did at the time)
--without-jit it'll error with just USE_LIBPCRE=YesPlease as expected,
but add NO_LIBPCRE1_JIT=UnfortunatelyYes and it works just fine without
your patch.

However, as your patch shows (and as I've independently verified)
PCRE_STUDY_JIT_COMPILE will still be defined in that case, since PCRE
will be exposing the same headers. This is the logic error in my initial
patch.

*But* for some reason you still get away with that on Linux. I don't
know why, but I assume the compiler toolchain is more lax for some
reason than on Solaris.

All of which is a roundabout way of saying that we should apply this
patch, but that I still have no idea why this worked on Linux before ,
but it does.

But that we should take it anyway regardless of that since it'll *also*
work on Linux with your patch, and this logic makes some sense whereas
the other one clearly didn't and just worked by pure accident of some
toolchain semantics that I haven't figured out yet.

Re: [PATCH] Fix NO_LIBPCRE1_JIT to fully disable JIT

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> On Sun, Nov 12 2017, Charles Bailey jotted:
>
>> From: Charles Bailey <cbailey32@bloomberg.net>
>>
>> If you have a pcre1 library which is compiled with JIT enabled then
>> PCRE_STUDY_JIT_COMPILE will be defined whether or not the
>> NO_LIBPCRE1_JIT configuration is set.
>>
>> This means that we enable JIT functionality when calling pcre_study
>> even if NO_LIBPCRE1_JIT has been explicitly set and we just use plain
>> pcre_exec later.
>>
>> Fix this by using own macro (GIT_PCRE_STUDY_JIT_COMPILE) which we set to
>> PCRE_STUDY_JIT_COMPILE only if NO_LIBPCRE1_JIT is not set and define to
>> 0 otherwise, as before.
>> ---
>>
>> I was bisecting an issue with the PCRE support that was causing a test
>> ...
>
> [CC-ing Junio]
>
> Thanks a lot. This patch looks good to me.

Thanks.  This patch needs a sign-off, by the way.

> But that we should take it anyway regardless of that since it'll *also*
> work on Linux with your patch, and this logic makes some sense whereas
> the other one clearly didn't and just worked by pure accident of some
> toolchain semantics that I haven't figured out yet.

That is curious and would be nice to know the answer to.

Re: [PATCH] Fix NO_LIBPCRE1_JIT to fully disable JIT

[Charles Bailey]

On Mon, Nov 13, 2017 at 12:53:15PM +0900, Junio C Hamano wrote:
> 
> Thanks.  This patch needs a sign-off, by the way.

Signed-off-by: cbailey32@bloomberg.net

(I can resend the full patch if required or if anyone requests futher
changes.

> > But that we should take it anyway regardless of that since it'll *also*
> > work on Linux with your patch, and this logic makes some sense whereas
> > the other one clearly didn't and just worked by pure accident of some
> > toolchain semantics that I haven't figured out yet.
> 
> That is curious and would be nice to know the answer to.

The error that I was getting - if I remember the details of the very
brief debugging session that I performed - was an unaligned memory
access causing a SIGBUS in PCRE code whose function name contained 'jit'
and which was being called indirectly from pcre_study.

My guess is that we are just exposing a pre-existing bug in our Solaris
build of libpcre. Unaligned memory accesses on x86 / x86_64 "only" cause
performance issues rather than fatal signals so even if the same bug
exists on Linux it probably has no noticeable effect (or at least no
noticed effect).

Charles.

Re: [PATCH] Fix NO_LIBPCRE1_JIT to fully disable JIT

[Junio C Hamano]

Charles Bailey <charles@hashpling.org> writes:

>> > But that we should take it anyway regardless of that since it'll *also*
>> > work on Linux with your patch, and this logic makes some sense whereas
>> > the other one clearly didn't and just worked by pure accident of some
>> > toolchain semantics that I haven't figured out yet.
>> 
>> That is curious and would be nice to know the answer to.
>
> The error that I was getting ...
> My guess is that we are just exposing a pre-existing bug in our Solaris
> build of libpcre.

Sorry, my question was not clear.  I think you already mentioned the
above in the thread.  What I was curious about was why Ævar was
seeing that JIT disabled with NO_LIBPCRE1_JIT alone on his Linux
setup, i.e. namely this part from his message:

    *But* for some reason you still get away with that on Linux. I
    don't know why, but I assume the compiler toolchain is more lax
    for some reason than on Solaris.n

In any case, thanks for a fix; queued.