Re: [PATCH] grep: skip UTF8 checks explicitally

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jul 22, 2019, 19:42 UTC
Message-ID: <87muh57t5r.fsf@evledraar.gmail.com>
In-Reply-To: <nycvar.QRO.7.76.6.1907221340320.47@tvgsbejvaqbjf.bet>

On Mon, Jul 22 2019, Johannes Schindelin wrote:

Show 22 quoted lines

> Hi Carlo,
>
> On Sun, 21 Jul 2019, Carlo Marcelo Arenas Belón wrote:
>
>> Usually PCRE is compiled with JIT support, and therefore the code
>> path used includes calling pcre2_jit_match (for PCRE2), that ignores
>> invalid UTF-8 in the corpus.
>>
>> Make that option explicit so it can be also used when JIT is not
>> enabled and pcre2_match is called instead, preventing `git grep`
>> to abort when hitting the first binary blob in a fixed match
>> after ed0479ce3d ("Merge branch 'ab/no-kwset' into next", 2019-07-15)
>
> Good idea.
>
> The flag has been in PCRE1 since at least March 5, 2007, when the
> pcre.h.in file was first recorded in their Subversion repository:
> https://vcs.pcre.org/pcre/code/trunk/pcre.h.in?view=log
>
> It also was part of PCRE2 from the first revision (rev 4, in fact, where
> pcre2.h.in was added):
> https://vcs.pcre.org/pcre2/code/trunk/src/pcre2.h.in?view=log

Thanks for digging, that portability indeed sounds just fine.

> So I am fine with this patch.

I'm not, not because I dislike the approach. I haven't made up my mind yet.

I stopped paying attention to this error-with-not-JIT discussion when I heard that some other series going into next for Windows fixed that issue[1]

But now we have it again in some form? My ab/no-kwset has a lot of tests for encodings & locales combined with grep, don't some of those trigger this? If so we should make any such failure a test & part of this patch.

Right now we don't have the info of whether we're really using the JIT or not, but that would be easy to add to grep's --debug mode for use in a test prereq.

As noted in [2] I'd be inclined to go the other way, if we indeed have some cases where PCRE skips its own checks does not dying actually give us anything useful? I'd think not, so just ignoring the issue seems like the wrong thing to do.

Surely we're not producing useful grep results at that point, so just not dying and mysteriously returning either nothing or garbage isn't going to help much...

1. https://public-inbox.org/git/xmqq4l3wxk8j.fsf@gitster-ct.c.googlers.com/
2. https://public-inbox.org/git/87pnms7kv0.fsf@evledraar.gmail.com/

Show 33 quoted lines

> Thanks,
> Dscho
>
>> ---
>>  grep.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/grep.c b/grep.c
>> index fc0ed73ef3..146093f590 100644
>> --- a/grep.c
>> +++ b/grep.c
>> @@ -409,7 +409,7 @@ static void compile_pcre1_regexp(struct grep_pat *p, const struct grep_opt *opt)
>>  static int pcre1match(struct grep_pat *p, const char *line, const char *eol,
>>  		regmatch_t *match, int eflags)
>>  {
>> -	int ovector[30], ret, flags = 0;
>> +	int ovector[30], ret, flags = PCRE_NO_UTF8_CHECK;
>>
>>  	if (eflags & REG_NOTBOL)
>>  		flags |= PCRE_NOTBOL;
>> @@ -554,7 +554,7 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
>>  static int pcre2match(struct grep_pat *p, const char *line, const char *eol,
>>  		regmatch_t *match, int eflags)
>>  {
>> -	int ret, flags = 0;
>> +	int ret, flags = PCRE2_NO_UTF_CHECK;
>>  	PCRE2_SIZE *ovector;
>>  	PCRE2_UCHAR errbuf[256];
>>
>> --
>> 2.22.0
>>
>>