collectElementTags() in modParser is not parsing Template tags under certain conditions

[derjasa]

Bug report

Summary

At some point in processing, template tags are no longer parsed and are simply rendered as text.

I traced the problem down to these lines of code

revolution/core/src/Revolution/modParser.php

Lines 150 to 151 in f7bc013

	$pattern = '/\Q'.$prefix.'\E((?:(?:[^'.$subSuffix.$subPrefix.'][\s\S]*?|(?R))*?))\Q'.$suffix.'\E/x';
	preg_match_all($pattern, $origContent, $matches, PREG_SET_ORDER);

In ModX 3 the function collectElementTags() has been revised and is now utilizing preg_match_all(). If PHP is running with PCRE JIT Compiler enabled, preg_match_all() may silently start failing when the JIT Conpiler stack limit is exceeded,. This can happen when the given subject is to long and the given pattern to complex.

Reference https://stackoverflow.com/questions/34849485/regex-not-working-for-long-pattern-pcres-jit-compiler-stack-limit-php7

Step to reproduce

A script that I created for tests is attached. The test is failing only under PHP 7.2 and PHP 7.3. Disabling PCRE JIT Compiler fixes the problem. Starting with PHP 7.4 the test succeeded.

test .txt

Settings in php.ini (default values) when the test fails:
pcre.jit = 1;
pcre.backtrack_limit = 1000000;
pcre.recursion_limit = 100000;

Observed behavior

$matches is empty and collectElementTags() will return 0.

Expected behavior

$matches should contain one item and collectElementTags() should return 1.

Environment

modx version: 3.0.1
nginx version: nginx/1.14.0 (Ubuntu)
php version: 7.2 & 7.3 with latest fixes running as FPM or CLI.

[JoshuaLuckers]

@derjasa thanks for taking time to investigate this issue.

What happens if you replace the code on line 150 with this:

$pattern = '/\Q'.$prefix.'\E((?:(?:[^'.$subSuffix.$subPrefix.']++[\s\S]*?|(?R))*?))\Q'.$suffix.'\E/x';

Does it yield the expected results?

[derjasa]

@JoshuaLuckers thanks to you too!

I tested the new pattern and the test was successfull. The output was as expected and is now as intended to be.

[rtripault]

Hi! Just wanted to confirmed we faced the issue as well, and the proposed new pattern indeed solved the issue (disabling pcre.jit also worked, but better have a more efficient pattern ;)).

We spent quite some time digging the issue (too bad we found that issue only before trying to report the issue). I guess having an error message in case of PCRE issue might be a nice addition, ie.

        preg_match_all($pattern, $origContent, $matches, PREG_SET_ORDER);
        if (preg_last_error() !== PREG_NO_ERROR) {
            $this->logger->error(
                'Parsing caused some issue',
                [
                    'error' => [
                        'number' => preg_last_error(),
                        'message' => preg_last_error_msg(),
                    ],
                ]
            );
        }

Edit: also, FWIW, adding a comment describing the intention of the pattern might be a good idea for any future maintainer that could contribute any tweak/performance improvement.