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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

500 Error when PREG Backtrack Limit Exhausted #993

Closed
christianc1 opened this Issue Mar 8, 2018 · 12 comments

Comments

Projects
None yet
3 participants
@christianc1
Copy link
Contributor

christianc1 commented Mar 8, 2018

PHP versions 5.2.x have a default value of 100000 on pcre.backtrack_limit. We stumbled on the issue in 0.7-beta with the work supporting amp-bind.

We are using amp-iframe as a container for a table with a lot of rows and some javascript to help filter that table. We're injecting the iframe source through the srcdoc attribute. The regex matching tag attributes eventually will hit backtrack limit, fail with a PREG_BACKTRACK_LIMIT_ERROR silently, and AMP_Dom_Utils::convert_amp_bind_attributes will return null.

Further along the stack, this null value will be fed to DOMDocument which will cause a fatal error.
I'm opening up a PR shortly with some failing tests. I've made an attempt at modifying the regex, but I can't get all the tests to pass with my changes so I'm opening up this issue to the broader community.

@christianc1

This comment has been minimized.

Copy link
Contributor

christianc1 commented Mar 8, 2018

I added failing tests in #994.

## PHPUnit tests
Installing WP from https://develop.svn.wordpress.org/tags/4.7/ to /tmp/wordpress
mysqladmin: DROP DATABASE wordpress_test failed;
error: 'Can't drop database 'wordpress_test'; database doesn't exist'
wordpress_test does not exist yet
DB wordpress_test created
WP and unit tests installed
Location: /tmp/wordpress/src/wp-content/plugins/amp-wp
Installing REST API...
Installing...
Running as single site... To run multisite, use -c tests/phpunit/multisite.xml
Not running ajax tests. To execute these, use --group ajax.
Not running ms-files tests. To execute these, use --group ms-files.
Not running external-http tests. To execute these, use --group external-http.
PHPUnit 5.7.27 by Sebastian Bergmann and contributors.
...............................................................  63 / 728 (  8%)
............................................................... 126 / 728 ( 17%)
............................................................... 189 / 728 ( 25%)
............................................................... 252 / 728 ( 34%)
.....................FF........................................ 315 / 728 ( 43%)
.............S................................................. 378 / 728 ( 51%)
............................................................... 441 / 728 ( 60%)
............................................................... 504 / 728 ( 69%)
............................................................... 567 / 728 ( 77%)
............................................................... 630 / 728 ( 86%)
............................................................... 693 / 728 ( 95%)
...................................                             728 / 728 (100%)
Time: 10.91 seconds, Memory: 73.25MB
There were 2 failures:
1) AMP_DOM_Utils_Test::test_attribute_conversion_on_long_iframe_srcdocs with data set #3 (1000)
Failed when backtrack limit was exhausted.
/tmp/wordpress/src/wp-content/plugins/amp-wp/tests/test-class-amp-dom-utils.php:199
2) AMP_DOM_Utils_Test::test_attribute_conversion_on_long_iframe_srcdocs with data set #4 (10000)
Failed when backtrack limit was exhausted.
/tmp/wordpress/src/wp-content/plugins/amp-wp/tests/test-class-amp-dom-utils.php:199
FAILURES!
Tests: 728, Assertions: 1341, Failures: 2, Skipped: 1.
@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented Mar 8, 2018

@christianc1 the 0.7 version of the plugin bumps the minimum PHP version to 5.3. So this is not just a 5.2 issue?

@christianc1

This comment has been minimized.

Copy link
Contributor

christianc1 commented Mar 8, 2018

@westonruter no, this issue will occur on PHP versions > 5.2. Independent of PHP version, this may affect any environment with pcre.backtrack_limit ini value of 100,000, which is the default on 5.2+.

@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented Mar 9, 2018

Strange I can't get the issue to reproduce on my system (VVV). I tried doing ini_set( 'pcre.backtrack_limit', 100000 ); at the start of the method but it doesn't cause the issue to manifest.

At issue is this regular expression?

'#^\s+(?P<name>\[?[a-zA-Z0-9_\-]+\]?)(?P<value>=(?:"[^"]*"|\'[^\']*\'|[^\'"\s]+))?#'
@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented Mar 9, 2018

Or is it?

'#<(?P<name>[a-zA-Z0-9_\-]+)(?P<attrs>\s+[^>]+\]=[^>]+)\s*>#'

@westonruter westonruter added this to the v0.7 milestone Mar 9, 2018

@adamsilverstein

This comment has been minimized.

Copy link
Contributor

adamsilverstein commented Mar 9, 2018

@westonruter Do you get the error if you run the tests in #994? I believe the regex that causes the issue is the second one: '#<(?P<name>[a-zA-Z0-9_\-]+)(?P<attrs>\s+[^>]+\]=[^>]+)\s*>#'

@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented Mar 9, 2018

@adamsilverstein no, I get no errors on that fix/993-preg-backtrack-limit-exhausted branch in VVV:

$ phpunit --filter test_attribute_conversion_on_long_iframe_srcdocs
Installing...
Running as single site... To run multisite, use -c tests/phpunit/multisite.xml
Not running ajax tests. To execute these, use --group ajax.
Not running ms-files tests. To execute these, use --group ms-files.
Not running external-http tests. To execute these, use --group external-http.
PHPUnit 5.7.25 by Sebastian Bergmann and contributors.

.....                                                               5 / 5 (100%)

Time: 8.83 seconds, Memory: 49.25MB

OK (5 tests, 0 assertions)
@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented Mar 9, 2018

@christianc1 Try simplifying the regex a bit like this:

- '#<(?P<name>[a-zA-Z0-9_\-]+)(?P<attrs>\s+[^>]+\]=[^>]+)\s*>#'
+ '#<(?P<name>[a-zA-Z0-9_\-]+)(?P<attrs>\s[^>]+\]=[^>]+)>#'

That seems like it would reduce the amount of backtracking required.

@christianc1

This comment has been minimized.

Copy link
Contributor

christianc1 commented Mar 9, 2018

That helped a bit. Travis is still failing though.

- '#<(?P<name>[a-zA-Z0-9_\-]+)(?P<attrs>\s[^>]+\]=[^>]+)>#'
+ '#<(?P<name>[a-zA-Z0-9_\-]+)(?P<attrs>\s[^>\]]+\]=[^>]+)>#'

Excluding both > and ] from the second capture group removes some complexity and gets us a match in less steps given the following test string:

<amp-state id="myAnimals">
  <script type="application/json">
    {
      "dog": {
        "imageUrl": "/img/dog.jpg",
        "style": "greenBackground"
      },
      "cat": {
        "imageUrl": "/img/cat.jpg",
        "style": "redBackground"
      }
    }
  </script>
</amp-state>

<p [text]="'This is a ' + currentAnimal + '.'">This is a dog.</p>

<!-- CSS classes can also be added or removed with [class]. -->
<p class="greenBackground" [class]="myAnimals[currentAnimal].style">
  Each animal has a different background color.
</p>

<!-- Or change an image's src with the [src] binding. -->
<amp-img width="300" height="200" src="/img/dog.jpg"
    [src]="myAnimals[currentAnimal].imageUrl">
</amp-img>

<button on="tap:AMP.setState({currentAnimal: 'cat'})">Set to Cat</button>

'#<(?P<name>[a-zA-Z0-9_\-]+)(?P<attrs>\s[^>]+\]=[^>]+)>#' : 3 matches in 277 steps
'#<(?P<name>[a-zA-Z0-9_\-]+)(?P<attrs>\s[^>\]]+\]=[^>]+)>#' : 3 matches in 87 steps

@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented Mar 9, 2018

How are you obtaining the number of steps?

@christianc1

This comment has been minimized.

Copy link
Contributor

christianc1 commented Mar 10, 2018

@westonruter I use https://regex101.com, there is some profiling information in the top right above where you enter the regex.

@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented Mar 11, 2018

@christianc1 The row of HTML is 360 bytes. The test is failing at 10,000 iterations. That means a srcdoc that is almost 4MB (❗️) . Is it reasonable to expect this? The average AMP document is 80KB in total. As per #875 if the AMP document is more than 1MB there will be other performance problems to worry about.

@westonruter westonruter referenced this issue Mar 13, 2018

Open

Add performance profiling/testing #1017

0 of 5 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment