Fix two silent-corruption bugs and four conformance gaps - #154
Merged
Merged
Conversation
A pattern that is not valid UTF-8 is tokenized byte by byte, the way PCRE compiles it without /u. Two places did not follow: the quoted run and the comment were read with a regex of their own, and both asked PCRE for UTF-8. On a pattern holding a stray byte those matches fail, and the failure was taken for "nothing left to read": the lexer left the mode, jumped to the end of the pattern, and everything after it disappeared. "/\Q\xFFabc\E]/" — which PCRE accepts — came back as "//". No error, no warning, an empty pattern. Both now read the subject the way the pattern itself is read, and a failure that is a real PCRE error is reported instead of swallowed. The two tests that covered the old behaviour asserted the truncation. They assert the error now, and a new test walks byte-mode patterns through the lexer and back out.
The same mistake as the recursion condition, one node along: the version condition wrote its own "(?(...)" and the conditional wrapped it again, so "(?(VERSION>=10.4)y|n)" came back as "(?((?(VERSION>=10.4))y|n)" — one parenthesis too many and a pattern PCRE refuses. The node returns the condition alone now, as every other kind does, and the conditional writes the parentheses. It went unnoticed because nothing compiled a conditional back out. There is now a case per kind of condition — numbered, named, recursion, version, the four lookarounds, define, and a conditional with no else branch — each checked against the pattern it came from and against PCRE. "(?(VERSION=10.4)...)" is read too, which PCRE accepts and the reader did not.
A quick fix hands the editor the text to put in place of the pattern, and built it by wrapping the new pattern in single quotes. A regex is free to hold one — "/it's/" is a fine pattern — and applying the fix to such a pattern left the file unparseable. The replacement is written as a PHP literal, which escapes what has to be. The binary also stops announcing a version of its own: --version said 1.0.0 while the library was at 1.3.0, the same drift the handshake had.
Encoding a block of code points asked mb_convert_encoding without checking that mbstring is there. Everywhere else the package works without it — CodePointHelper falls back to intl and then to writing the UTF-8 bytes itself — so a process without the extension would have hit a fatal here and nowhere else. The conversion is used where it exists, since it does the whole block at once, and the same encoding is done by hand where it does not. A test walks both across every width change in UTF-8 and around the surrogates.
"J" lets two groups share a name, and like every inline modifier it reaches only to the end of the group that carries it. The parser turned it on and never turned it back off, so a name repeated well outside the reach of the "(?J)" was accepted — "(?J:(?<n>a))(?<n>b)" among them, which PCRE refuses. It is saved and restored the way the extended mode already was, at the two places a group ends. Six cases pin where the modifier reaches, each checked against PCRE's answer for the same pattern before the parser is asked.
The alphabetic verbs carry a sub-pattern — "(*atomic:...)", "(*pla:...)", "(*sr:...)" — and the token pattern that matched them allowed brackets one level deep. "(*atomic:((a)))" is a pattern PCRE takes and this one refused, with an error about a quantifier, since the verb fell apart into pieces the parser then had to make sense of. The body is matched by recursion instead, so it nests as deep as the sub-pattern does. The corpus report is unchanged: nothing in twelve thousand real patterns was reading a verb the old way.
…n with PCRE names a group four ways — "(?<n>", "(?'n'", "(?P<n>", "(?P\"n\"" — and writes a script run two — "(*sr:", "(*script_run:". They mean the same thing, so the tree keeps only the name, and the compiler picked one spelling for everyone. Rewriting a pattern the author did not ask to have rewritten is what the round trip is meant to avoid; it already avoided it for optional escapes and for the whitespace /x ignores. Both are now read back from the source, and only when what is written names the very group being compiled. The fidelity test gains the six spellings. Two cases move with it: the quoted Python spellings come back as they were written, and since PCRE refuses those spellings outright, they leave the list of patterns whose compiled form PCRE is asked to accept.
Rector cannot resolve a PHPUnit assertion called through self:: and stops on the file rather than analysing it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two of these lose or corrupt what the caller gave us, and four are patterns
PCRE accepts that this library did not — or the other way round. Each commit
carries the test that would have caught it.
Silent corruption
\Q...\Erun or a(?#...)comment. Those two were read with a UTF-8 regex, and PCRE's refusalwas taken for the end of the pattern:
/\Q\xFFabc\E]/came back as//, withno error.
(?(VERSION>=10.4)y|n)came back with one parenthesis too many. Nothingcompiled a conditional back out, so it went unseen; there is now a case per
kind of condition.
escaping, so applying one to
/it's/left the file unparseable.Conformance
(*atomic:((a)))and(*pla:((a)b(c)))were refused; a verb wraps as manybrackets as its sub-pattern needs.
Jmodifier reached past the group carrying it, so(?J:(?<n>a))(?<n>b)was accepted where PCRE refuses it.
(?(VERSION=10.4)...)is read; PCRE takes that spelling too.(?'name'x)and(*sr:...)came back rewritten as(?<name>x)and(*script_run:...). The round trip already kept the author's optionalescapes; it keeps these too.
Robustness
package that required it.
Every new case is checked against what PCRE answers for the same pattern.
6670 tests,
task lintgreen, and the report for the 12 929 corpus patterns isbyte-identical.