Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
#20411 fix Yaml parsing for very long quoted strings#21523
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| $isRef =$mergeNode =false; | ||
| if (preg_match('#^\-((?P<leadspaces>\s+)(?P<value>.+?))?\s*$#u',$this->currentLine,$values)) { | ||
| if (self::preg_match('#^\-((?P<leadspaces>\s+)(?P<value>.+))?$#u',rtrim($this->currentLine),$values)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
See comment on line 127 below
| $this->refs[$isRef] =end($data); | ||
| } | ||
| }elseif (preg_match('#^(?P<key>'.Inline::REGEX_QUOTED_STRING.'|[^\'"\[\{].*?) *\:(\s+(?P<value>.+?))?\s*$#u',$this->currentLine,$values) && (false ===strpos($values['key'],' #') ||in_array($values['key'][0],array('"',"'")))) { | ||
| }elseif (self::preg_match('#^(?P<key>'.Inline::REGEX_QUOTED_STRING.'|[^\'"\[\{].*?) *\:(\s+(?P<value>.+))?$#u',rtrim($this->currentLine),$values) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Here, as well as wrapping "preg_match", I have fixed the regex to avoid large numbers of pcre backtracks by moving the trailing whitespace trimming behaviour out of the regex pattern and into a "rtrim" in the argument list.
This may potentially be less performant in some cases (but I expect more performant in most cases, actually), but I have not measured this.
It demonstrably fixes a bug, as can be seen by the unit test I have added to this commit, which fails without this change and passes with it.
RichardBradleyFeb 3, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The specific problem with the regex here was the ".+?" followed by a "\s*$", which leads to a great deal of backtracking behaviour in long strings. I could not find a simpler fix than the one I propose here (the possessive quantifier fix used in#21279 would not work here)
| }else { | ||
| if (isset($values['leadspaces']) | ||
| &&preg_match('#^(?P<key>'.Inline::REGEX_QUOTED_STRING.'|[^\'"\{\[].*?) *\:(\s+(?P<value>.+?))?\s*$#u',$values['value'],$matches) | ||
| &&self::preg_match('#^(?P<key>'.Inline::REGEX_QUOTED_STRING.'|[^\'"\{\[].*?) *\:(\s+(?P<value>.+))?$#u',rtrim($values['value']),$matches) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
see comment on line 127
| $error ='Error.'; | ||
| } | ||
| thrownewParseException($error); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This should not be reachable, but should hopefully mean that any further undetected "backtrack_limit" bugs, or any similar bugs added in the future, will result in an exception rather than an incorrect result.
| * @throws ParseException on a PCRE internal error | ||
| * @see preg_last_error() | ||
| */ | ||
| staticfunctionpreg_match($pattern,$subject, &$matches =null,$flags =0,$offset =0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should probably be calledpregMatch if you want to call it like this. Perhapsmatch would be a better name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I deliberately named it "preg_match" so that it was a drop-in replacement for the builtinpreg_match. Is that disallowed?
| * | ||
| * This avoids us needing to check for "false" every time PCRE is used | ||
| * in the YAML engine | ||
| * |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
must be@internal as we clearly don't want to support BC on it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
ok, will do
| $error ='Error.'; | ||
| } | ||
| thrownewParseException($error); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this misses the location of the error in the ParseException
RichardBradleyFeb 3, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The location is not always available, unless I make two wrappers -- one static, for Inline and other callers, and one non-static, for use during the parse. Do you think that's worthwhile?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You could pass the needed context to the new method. Might not look nice, but will do what it should.
| publicfunctionparse($value,$exceptionOnInvalidType =false,$objectSupport =false,$objectForMap =false) | ||
| { | ||
| if (!preg_match('//u',$value)) { | ||
| if (!self::preg_match('//u',$value)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would not replace this, as it would throw an exception sayingMalformed UTF-8 data. instead of the expected one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Good point. I'll add a test to cover this
fabpot commentedFeb 12, 2017
fabbot failures must be fixed. |
xabbuh commentedFeb 27, 2017
@RichardBradley Do you have time to finish here? :) |
RichardBradley commentedFeb 28, 2017
Sorry, I have been busy. I should be able to look over the next couple of days, yes. |
RichardBradley commentedMar 1, 2017
I have pushed an update which addresses the review comments above and fixes the "fabbot" style checks. |
| }elseif (preg_match('#^(?P<key>'.Inline::REGEX_QUOTED_STRING.'|[^\'"\[\{].*?) *\:(\s+(?P<value>.+?))?\s*$#u',$this->currentLine,$values) && (false ===strpos($values['key'],' #') ||in_array($values['key'][0],array('"',"'")))) { | ||
| }elseif (self::preg_match('#^(?P<key>'.Inline::REGEX_QUOTED_STRING.'|[^\'"\[\{].*?) *\:(\s+(?P<value>.+))?$#u',rtrim($this->currentLine),$values) | ||
| && (false ===strpos($values['key'],' #') | ||
| ||in_array($values['key'][0],array('"',"'")))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The way theif condition is wrapped now IMO doesn't make it more readable. Maybe reformat it to something like this:
}elseif (self::preg_match('#^(?P<key>'.Inline::REGEX_QUOTED_STRING.'|[^\'"\[\{].*?) *\:(\s+(?P<value>.+))?$#u',rtrim($this->currentLine),$values) && (false ===strpos($values['key'],' #') ||in_array($values['key'][0],array('"',"'")))) {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I have pushed a new version with your preferred indentation
| } | ||
| thrownewParseException($error,$this->getRealCurrentLineNb() +1,$this->currentLine); | ||
| thrownewParseException('Unable to parse',$this->getRealCurrentLineNb() +1,$this->currentLine); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Will this now ever be reached anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes, this line is covered by 3 tests:
ParserTest::testUnindentedCollectionExceptionParserTest::testShortcutKeyUnindentedCollectionExceptionParserTest::testScalarInSequence
RichardBradley commentedMar 10, 2017
I have pushed a new version which I believe addresses all the review issues raised |
xabbuh left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍
Status: Reviewed
fabpot commentedMar 17, 2017
Thank you@RichardBradley. |
…ardBradley)This PR was squashed before being merged into the 2.7 branch (closes#21523).Discussion----------#20411 fix Yaml parsing for very long quoted strings| Q | A| ------------- | ---| Branch? | 2.7| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#20411| License | MIT| Doc PR | noThis is a second fix for the issue discussed in#20411. My first PR (#21279) didn't fix the bug in all cases, sorry.If a YAML string has too many spaces in the value, it can trigger a `PREG_BACKTRACK_LIMIT_ERROR` error in the Yaml parser.There should be no behavioural change other than the bug fixI have included a test which fails before this fix and passes after this fix.I have also added checks that detect other PCRE internal errors and throw a more descriptive exception. Before this patch, the YAML engine would often give incorrect results, rather than throwing, on a PCRE `PREG_BACKTRACK_LIMIT_ERROR` error.Commits-------c9a1c09#20411 fix Yaml parsing for very long quoted strings
This is a second fix for the issue discussed in#20411. My first PR (#21279) didn't fix the bug in all cases, sorry.
If a YAML string has too many spaces in the value, it can trigger a
PREG_BACKTRACK_LIMIT_ERRORerror in the Yaml parser.There should be no behavioural change other than the bug fix
I have included a test which fails before this fix and passes after this fix.
I have also added checks that detect other PCRE internal errors and throw a more descriptive exception. Before this patch, the YAML engine would often give incorrect results, rather than throwing, on a PCRE
PREG_BACKTRACK_LIMIT_ERRORerror.