Bug #77193 [Opn]: Infinite loop in preg_replace_callback

Edit report at https://bugs.php.net/bug.php?id=77193&edit=1 ID: 77193 User updated by: mlocati at gmail dot com Reported by: mlocati at gmail dot com Summary: Infinite loop in preg_replace_callback Status: Open Type: Bug Package: PCRE related PHP Version: 7.3.0RC6 Block user comment: N Private report: N New Comment: @cmb I just tried your patch instead of the @ab one, but with it we still have the infinite loop... Previous Comments: ------------------------------------------------------------------------ [2018-11-29 11:34:17] cmb@php.net I wonder whether we don't have to call pcre2_get_mark() before calling pcre2_get_ovector_pointer(), i.e. partly reverting commit b81d712. I mean something like this instead the patch above: ext/pcre/php_pcre.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/ext/pcre/php_pcre.c b/ext/pcre/php_pcre.c index 5165209b85..4277ffb6aa 100644 --- a/ext/pcre/php_pcre.c +++ b/ext/pcre/php_pcre.c @@ -1792,6 +1792,7 @@ static zend_string *php_pcre_replace_func_impl(pcre_cache_entry *pce, zend_strin char *match, /* The current match */ *piece; /* The current piece of subject */ size_t result_len; /* Length of result */ + PCRE2_SPTR mark = NULL; /* Target for MARK name */ zend_string *result; /* Result of replacement */ zend_string *eval_result; /* Result of custom function */ pcre2_match_data *match_data; @@ -1852,6 +1853,8 @@ static zend_string *php_pcre_replace_func_impl(pcre_cache_entry *pce, zend_strin while (1) { piece = subject + start_offset; + mark = pcre2_get_mark(match_data); + if (count >= 0 && limit) { /* Check for too many substrings condition. */ if (UNEXPECTED(count == 0)) { @@ -1881,8 +1884,7 @@ matched: new_len = result_len + offsets[0] - start_offset; /* part before the match */ /* Use custom function to get replacement string and its length. */ - eval_result = preg_do_repl_func(fci, fcc, subject, offsets, subpat_names, count, - pcre2_get_mark(match_data)); + eval_result = preg_do_repl_func(fci, fcc, subject, offsets, subpat_names, count, mark); ZEND_ASSERT(eval_result); new_len = zend_safe_address_guarded(1, ZSTR_LEN(eval_result), new_len); [1] <http://git.php.net/?p=php-src.git;a=commit;h=b81d712961aa3cbc64dc8bb521f2427cf443e550> ------------------------------------------------------------------------ [2018-11-29 09:04:48] mlocati at gmail dot com @ab Yes! That patch fixed the issue: C:\MyPath>composer test -- --filter=ContentPageTranslateTest::testFrom > phpunit "--filter=ContentPageTranslateTest::testFrom" PHPUnit 4.8.36 by Sebastian Bergmann and contributors. Runtime: PHP 7.3.0-dev Configuration: C:\MyPath\phpunit.xml . Time: 4.68 seconds, Memory: 30.00MB OK (1 test, 1 assertion) Great job, Anatol! ------------------------------------------------------------------------ [2018-11-28 20:32:20] ab@php.net @mlocati thanks, with the docker way reproduced it as well. I still can't repro it on a reduced snippet. I guess, the method call inside the callback somehow resets the match context (so somewhere in the call trace is a PCRE API call), thus preventing loop termination. Could you please check, whether this fixes it? diff --git a/ext/pcre/php_pcre.c b/ext/pcre/php_pcre.c index 5165209b85..f8fc721d81 100644 --- a/ext/pcre/php_pcre.c +++ b/ext/pcre/php_pcre.c @@ -1880,6 +1880,9 @@ matched: new_len = result_len + offsets[0] - start_offset; /* part before the match */ + /* Advance to the next piece. */ + start_offset = offsets[1]; + /* Use custom function to get replacement string and its length. */ eval_result = preg_do_repl_func(fci, fcc, subject, offsets, subpat_names, count, pcre2_get_mark(match_data)); @@ -1908,9 +1911,6 @@ matched: limit--; - /* Advance to the next piece. */ - start_offset = offsets[1]; - /* If we have matched an empty string, mimic what Perl's /g options does. This turns out to be rather cunning. First we set PCRE2_NOTEMPTY_ATSTART and try the match again at the same point. If this fails (picked up above) we ------------------------------------------------------------------------ [2018-11-27 15:56:27] mlocati at gmail dot com > This doesn't look like a PCRE issue. URL::to() seems to block the callback, > so then it never returns. The actual issue should be somewhere later in > the call stack starting from URL::to(), something like an endless loop or > unterminated recursive call. I modified the code a bit to show what's happening. With this code: ob_end_clean(); // Required because PHPUnit enables output buffering echo 'Method: ', __METHOD__, "\n"; echo 'Text: ', $text, "\n"; $text = preg_replace_callback( '/{CCM:CID_([0-9]+)}/i', function ($matches) { print_r($matches); $cID = $matches[1]; if ($cID > 0) { $c = Page::getByID($cID, 'ACTIVE'); if ($c->isActive()) { return (string) \URL::to($c); } } }, $text ); The output is the following: Method: Concrete\Core\Editor\LinkAbstractor::translateFrom Text: <a href="{CCM:CID_3}">Super Cool!</a> Array ( [0] => {CCM:CID_3} [1] => 3 ) Array ( [0] => {CCM:CID_3} [1] => 3 ) Array ( [0] => {CCM:CID_3} [1] => 3 ) [...endless list of Array()...] As you can see, the \URL::to() call is not blocking the execution, but the callback is called forever even if we have just one match. ------------------------------------------------------------------------ [2018-11-27 15:05:34] ab@php.net This doesn't look like a PCRE issue. URL::to() seems to block the callback, so then it never returns. The actual issue should be somewhere later in the call stack starting from URL::to(), something like an endless loop or unterminated recursive call. I'm going to check the suggested repro way with the docker container next days otherwise. Thanks. ------------------------------------------------------------------------ The remainder of the comments for this report are too long. To view the rest of the comments, please view the bug report online at https://bugs.php.net/bug.php?id=77193 -- Edit this bug report at https://bugs.php.net/bug.php?id=77193&edit=1