Does this happen regardless of the value of pcre.jit?  Also, does
this happen if you actually return an empty string from the
callback instead of the implicit NULL?

I tried both with
pcre.jit=0
and with
pcre.jit=1

in php.ini.

The infinite loop also occurs when I remove the "return" instruction from "return (string) \URL::to($c);", and always returning an empty string, that is:

$text = preg_replace_callback(
    '/{CCM:CID_([0-9]+)}/i',
    function ($matches) {
        $cID = $matches[1];
        if ($cID > 0) {
            $c = Page::getByID($cID, 'ACTIVE');
            if ($c->isActive()) {
                (string) \URL::to($c);
            }
        }
        return '';
    },
    $text
);

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.

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

@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

@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!

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>

@cmb I just tried your patch instead of the @ab one, but with it we still have the infinite loop...

Just for the records, I'm compiling a 32-bit PHP under Windows with

configure --disable-all --with-all-shared --enable-cli --enable-phar --enable-json --enable-filter --with-iconv --enable-pdo --with-mysqli --with-mysqlnd --with-pdo-mysql --with-openssl --with-simplexml --with-libxml --with-gd --enable-mbstring --enable-tokenizer --with-dom --with-xml --enable-xmlreader --enable-xmlwriter --enable-hash  --enable-fileinfo --enable-session --enable-zip

nmake

Thanks for testing, Michele!  Then we should go with Anatol's
patch.  Not sure if it should go into PHP-7.3.0.

Thanks for testing, Michele.

Chistoph, I'll be preparing a patch then. Still lack on a simpler repro case :/

Thanks

It took me quite some time to keep removing stuff to get to a test script as simple as possible, and here it is:

<?php
$text = '{CCM:CID_2}';
echo '1';
preg_replace_callback(
    '/([0-9]+)/i',
    function ($matches) {
        echo $matches[1];
        filter_var('http', FILTER_VALIDATE_REGEXP, ['options' => ['regexp' => '/^http$/i']]);
    },
    $text
);
echo '3';


It should print '123', but it prints '1' and an infinite number of '2'.

If you set $text to just a digit (eg '2'), just '12' is printed out, and PHP quits with an error level -1073741819 (I'm using a 32-bit PHP).

PS: I'm on Windows, so -1073741819 is the 0xC0000005 (STATUS_ACCESS_VIOLATION) system error

What happens with this?

<?php
$text = '{CCM:CID_2}';
echo '1';
preg_replace_callback(
    '/([0-9]+)/i',
    function ($matches) {
        echo $matches[1];
        preg_match('/^http$/i', '');
    },
    $text
);
echo '3';

@mlocati, excellent! I'll add it as a test.

@requinix your snippet doesn't repro, unfortunately.

Thanks.

> @requinix your snippet doesn't repro, unfortunately.
That was to test if the problem was with the PCRE2 code in general or if it was specifically about how ext/filter and FILTER_VALIDATE_REGEXP uses it.

Will this fix be include in PHP 7.3.0?

> Will this fix be include in PHP 7.3.0?

Good question!  Anatol, what do you think?

AFM this should be handled as a usual bug and the fix should go through QA. I woludn't expect any complications on this, still it's unfortunate it has been caught this late. It's anyway at your convenience, Christoph, I might not see the whole picture.

Thanks.

Thanks, Anatol.  Then I agree that we better ship this as of
7.3.1, albeit it's a regression from 7.2.  After all, 7.3.1 is
just a month away.