#20411 fix Yaml parsing for very long quoted strings #21523

RichardBradley · 2017-02-03T16:39:20Z

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	no

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_ERROR error 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_ERROR error.

RichardBradley · 2017-02-03T16:41:36Z

src/Symfony/Component/Yaml/Parser.php

@@ -92,13 +92,13 @@ public function parse($value, $exceptionOnInvalidType = false, $objectSupport =
            }

            $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)) {


See comment on line 127 below

RichardBradley · 2017-02-03T16:41:47Z

src/Symfony/Component/Yaml/Parser.php

@@ -124,7 +124,10 @@ public function parse($value, $exceptionOnInvalidType = false, $objectSupport =
                if ($isRef) {
                    $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)


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.

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)

RichardBradley · 2017-02-03T16:42:01Z

src/Symfony/Component/Yaml/Parser.php

@@ -108,7 +108,7 @@ public function parse($value, $exceptionOnInvalidType = false, $objectSupport =
                    $data[] = $this->parseBlock($this->getRealCurrentLineNb() + 1, $this->getNextEmbedBlock(null, true), $exceptionOnInvalidType, $objectSupport, $objectForMap);
                } 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)


see comment on line 127

RichardBradley · 2017-02-03T16:45:40Z

src/Symfony/Component/Yaml/Parser.php

+                    $error = 'Error.';
+            }
+
+            throw new ParseException($error);


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.

linaori · 2017-02-03T17:04:34Z

src/Symfony/Component/Yaml/Parser.php

+     * @throws ParseException on a PCRE internal error
+     * @see preg_last_error()
+     */
+    static function preg_match($pattern, $subject, &$matches = null, $flags = 0, $offset = 0)


Should probably be called pregMatch if you want to call it like this. Perhaps match would be a better name

I deliberately named it "preg_match" so that it was a drop-in replacement for the builtin preg_match. Is that disallowed?

stof · 2017-02-03T17:21:12Z

src/Symfony/Component/Yaml/Parser.php

+     *
+     * This avoids us needing to check for "false" every time PCRE is used
+     * in the YAML engine
+     *


must be @internal as we clearly don't want to support BC on it

ok, will do

stof · 2017-02-03T17:22:45Z

src/Symfony/Component/Yaml/Parser.php

+                    $error = 'Error.';
+            }
+
+            throw new ParseException($error);


this misses the location of the error in the ParseException

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?

You could pass the needed context to the new method. Might not look nice, but will do what it should.

stof · 2017-02-03T17:24:07Z

src/Symfony/Component/Yaml/Parser.php

@@ -61,7 +61,7 @@ public function __construct($offset = 0, $totalNumberOfLines = null, array $skip
     */
    public function parse($value, $exceptionOnInvalidType = false, $objectSupport = false, $objectForMap = false)
    {
-        if (!preg_match('//u', $value)) {
+        if (!self::preg_match('//u', $value)) {


I would not replace this, as it would throw an exception saying Malformed UTF-8 data. instead of the expected one

Good point. I'll add a test to cover this

fabpot · 2017-02-12T19:26:49Z

fabbot failures must be fixed.

xabbuh · 2017-02-27T17:00:18Z

@RichardBradley Do you have time to finish here? :)

RichardBradley · 2017-02-28T11:37:19Z

Sorry, I have been busy. I should be able to look over the next couple of days, yes.

RichardBradley · 2017-03-01T14:17:23Z

I have pushed an update which addresses the review comments above and fixes the "fabbot" style checks.

xabbuh · 2017-03-02T07:57:56Z

src/Symfony/Component/Yaml/Parser.php

-            } 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('"', "'")))) {


The way the if 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('"', "'"))) ) {

I have pushed a new version with your preferred indentation

xabbuh · 2017-03-02T07:58:40Z

src/Symfony/Component/Yaml/Parser.php

-                }
-
-                throw new ParseException($error, $this->getRealCurrentLineNb() + 1, $this->currentLine);
+                throw new ParseException('Unable to parse', $this->getRealCurrentLineNb() + 1, $this->currentLine);


Will this now ever be reached anymore?

Yes, this line is covered by 3 tests:

ParserTest::testUnindentedCollectionException

ParserTest::testShortcutKeyUnindentedCollectionException

ParserTest::testScalarInSequence

RichardBradley · 2017-03-10T11:46:35Z

I have pushed a new version which I believe addresses all the review issues raised

xabbuh

👍

Status: Reviewed

fabpot · 2017-03-17T16:20:31Z

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 | no 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_ERROR` error 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_ERROR` error. Commits ------- c9a1c09 #20411 fix Yaml parsing for very long quoted strings

symfony#20411 fix Yaml parsing for very long quoted strings

6e001c8

carsonbot added Status: Needs Review Bug labels Feb 3, 2017

RichardBradley commented Feb 3, 2017

View reviewed changes

linaori reviewed Feb 3, 2017

View reviewed changes

stof reviewed Feb 3, 2017

View reviewed changes

xabbuh added Status: Needs Work and removed Status: Needs Review labels Feb 3, 2017

nicolas-grekas added this to the 2.7 milestone Feb 6, 2017

symfony#20411 fix Yaml parsing for very long quoted strings

0152ad9

RichardBradley force-pushed the ticket_20411 branch from ec7f68e to 0152ad9 Compare March 1, 2017 14:16

xabbuh reviewed Mar 2, 2017

View reviewed changes

symfony#20411 reformat conditional as requested

757364d

xabbuh approved these changes Mar 17, 2017

View reviewed changes

xabbuh added Status: Reviewed Yaml and removed Status: Needs Work labels Mar 17, 2017

fabpot closed this Mar 17, 2017

fabpot mentioned this pull request Apr 4, 2017

Release v2.7.26 #22263

Merged

This was referenced Apr 5, 2017

Release v2.8.19 #22284

Merged

Release v3.2.7 #22293

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#20411 fix Yaml parsing for very long quoted strings #21523

#20411 fix Yaml parsing for very long quoted strings #21523

RichardBradley commented Feb 3, 2017

RichardBradley Feb 3, 2017

RichardBradley Feb 3, 2017

RichardBradley Feb 3, 2017 •

edited

Loading

RichardBradley Feb 3, 2017

RichardBradley Feb 3, 2017

linaori Feb 3, 2017

RichardBradley Feb 3, 2017

stof Feb 3, 2017

RichardBradley Feb 3, 2017

stof Feb 3, 2017

RichardBradley Feb 3, 2017 •

edited

Loading

xabbuh Mar 2, 2017

stof Feb 3, 2017

RichardBradley Feb 3, 2017

fabpot commented Feb 12, 2017

xabbuh commented Feb 27, 2017

RichardBradley commented Feb 28, 2017

RichardBradley commented Mar 1, 2017

xabbuh Mar 2, 2017

RichardBradley Mar 10, 2017

xabbuh Mar 2, 2017

RichardBradley Mar 10, 2017

RichardBradley commented Mar 10, 2017

xabbuh left a comment

fabpot commented Mar 17, 2017

#20411 fix Yaml parsing for very long quoted strings #21523

#20411 fix Yaml parsing for very long quoted strings #21523

Conversation

RichardBradley commented Feb 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RichardBradley Feb 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RichardBradley Feb 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Feb 12, 2017

xabbuh commented Feb 27, 2017

RichardBradley commented Feb 28, 2017

RichardBradley commented Mar 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RichardBradley commented Mar 10, 2017

xabbuh left a comment

Choose a reason for hiding this comment

fabpot commented Mar 17, 2017

RichardBradley Feb 3, 2017 •

edited

Loading

RichardBradley Feb 3, 2017 •

edited

Loading