Fix the JIT prefix scan of negated classes at 16 and 32 bits - #980
Conversation
| bytes = (sljit_u8*) (cc + 1); | ||
| /* Code units above the bitmap are accepted when its highest bit is set. | ||
| A negated class accepts all of them regardless, since the bitmap only | ||
| covers the first 256. */ |
There was a problem hiding this comment.
Isn't the type (class/nclass) represents the acceptance of chars > 255? If class can accept characters > 255 then nclass is meaningless.
There was a problem hiding this comment.
Yes, you're right. The opcode alone decides it, as it does in the
interpreter, where a code unit above 255 fails an OP_CLASS and passes an
OP_NCLASS without the bitmap being looked at. Bit 255 was never the right
test: the fix now sets accept_above from the opcode at 16 and 32 bits, and
to FALSE at 8 bits, where there is nothing above the bitmap.
That also covers the other direction. A positive class which includes \xff,
such as [A\xff], used to make its position count as any character, which was
only a missed optimization, and it now fast-forwards to its two characters.
I added it to testinput11 next to the negated case, with \x{1ff} in the
subject to check that only the code unit 0xff itself is taken.
In the 16-bit and 32-bit libraries a negated class accepts every code unit
above 255 and a positive class none of them, whatever the bitmap says about
255 itself. scan_prefix() instead took a class to accept those code units
when bit 255 of its bitmap was set, so a negated class which leaves out
\xff, such as [^\x00-\x40\x42-\xff], was taken to accept 'A' alone. The
JIT then fast-forwarded to the next 'A' and missed a \x{100} before it,
which the interpreter matches.
Decide on the opcode alone, as the interpreter does. The 8-bit library has
no code units above 255, so there the bitmap always describes the class on
its own, and a class which includes \xff no longer makes the scan give up
on its position. 10.47 is affected as well.
16da9a5 to
52957bb
Compare
|
I thought that bit 255 has no special meaning in a class. It does in some of the 'study' code. But OP_CLASS and OP_NCLASS should be treating all the bits in the bitmap exactly the same way. Only class/nclass distinction should matter. I may be misremembering... I haven't verified that. |
|
Yes, we are talking about the same thing. |
|
This is a fairly serious match correctness bug - we'll need to see if it's a regression, and how far back it goes in history. Anything that could case a 'reasonable' regex (like Matt's example) to return incorrect matches is bad enough that I'll triage it for backporting to all the old releases. |
|
@mattst88 Just to let you know - after I have merged all open PRs, I plan to re-indent the whole codebase! Any branches you have will need to be reformatted and de-conflicted. Would it be possible for you to pause opening PRs? (Well, you can open them, but any created after today will be held until I'm done.) I really apologise for this disruption. |
|
No problem -- sounds good to me. I don't have anything else in the queue at the moment. |
|
I confirm affected versions: 10.48 I will fix in 10.49-DEV, and produce a patch for backport. EDIT: Matt has fixed in 10.49-DEV. Many thanks! |
3 participants
In the 16-bit and 32-bit libraries a negated class accepts every code unit above 255, whatever its bitmap says about 255 itself.
scan_prefix()only took the class to accept those code units when bit 255 was set, so a negated class which leaves out\xff, such as[^\x00-\x40\x42-\xff], was taken to acceptAalone. The JIT then fast-forwarded to the nextAand missed a\x{100}before it, which the interpreter matches:This treats a negated class as accepting everything above the bitmap at those widths, as
pcre2_study()already does when it builds the start bitmap. 10.47 is affected as well, so it may be worth a backport.I found this while extending #941 to 16 and 32 bits, and the next revision of that PR builds on this change. The new tests in testinput11 fail under the JIT without the fix. RunTest and pcre2_jit_test pass at all three widths.