preg_split in _filter_url breaks for long html tags

I think we have been marking PHP 8 incompatibilities as critical.

Let's add a test that triggers this behaviour.

Here's a first pass a fail test. I took the approach of adding a ton of classes to a p tag. It's maybe not realistic, but I personally like the simplicity.

It may be interesting to note that the test passed for me locally with $classes = str_repeat('dum-class ', 999999);. So the test is right at the threshold, at least for my local setup.

Also, what's the best thing to do when adding a fail test when there's an issue branch with the fix already?

Sweet. Failed as hoped:

1) Drupal\Tests\filter\Kernel\FilterKernelTest::testUrlFilterLongTag
count(): Parameter must be an array or an object that implements Countable

/var/www/html/core/modules/filter/filter.module:539
/var/www/html/core/modules/filter/tests/src/Kernel/FilterKernelTest.php:949
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:703

Is the next step to commit this to the issue fork? (Assuming it's deemed a sufficient test)

Yes, add the failing test to the issue fork, which should continue to pass if the fix is correct. The test looks good enough to me, it is enough to trigger the error that was originally reported.

The new test passes on the issue fork.

Experiencing the same issue with base64 inline images.

Investigating the issue further I had to increase the pcre.backtract_limit in order to get it working.
You can use preg_last_error_msg() or preg_last_error() to get more precise cause for your preg_split faliure: https://www.php.net/manual/en/function.preg-last-error.php

Submitting a patch to fix the produced errors if preg_split fails.

The patch @borutpiletic provided is a simpler solution (changes one line of code). I suggest committing that along with the test from @danflanagan8. Attached is a patch with both.

As the failed test showed, the approach in #11 differs from the original one in the MR in more than just style. They both prevent the fatal error but the fix in #11 ends up setting the text to an empty string.

That happens in this line, which isn't in the patch. It's near line 570.

$text = implode($chunks);

where $chunks has been set to an empty array. I think the if-block is the way to go.

@danflanagan8 is right in #14, the patches provided in #11 and #12 contain an error.

Instead of a review on the PR, I did some refactoring, it can be used as review material.

Most importantly:

- The test is now part of ::testUrlFilterContent() since this seemed to me better than introducing a new method.
- The test is now attempting to explicitly break PHP's pcre.backtrack_limit since this seemed more accurate than attempting to figure out the limit by setting it very high.
- The actual solution does not use an if condition in order to avoid nesting (readability).

Typo

I will get there eventually

@stefanos.petrakis, I really like your new test. Definitely better than the one I added. There are a few lines though that strike me as something we don't need to commit:

+++ b/core/modules/filter/tests/src/Kernel/FilterKernelTest.php
@@ -932,6 +933,22 @@ public function testUrlFilterContent() {
+    // Make sure it broke.
+    preg_split('/(<.+?>)/is', $input, -1, PREG_SPLIT_DELIM_CAPTURE);
+    $preg_last_error = preg_last_error();
+    $this->assertSame($preg_last_error, PREG_BACKTRACK_LIMIT_ERROR, 'PREG backtrack error occurred as expected.');

That's not running any Drupal code. It helped me understand what's going on, but I think we'd want to remove it before committing.

And the fix looks correct and has a really small diff, which is cool. The following line, though, I had to read it a few times.

+++ b/core/modules/filter/filter.module
@@ -566,7 +566,7 @@ function _filter_url($text, $filter) {
+    $text = !$chunks ? $text : implode($chunks);

I think it would be easier to read if the condition were not negated. That is, I think it would be easier to read as:

$text = $chunks ? implode($chunks) : $text;

I can't personally RTBC this though since I contributed code earlier, so I won't insist on either change. I'm going to leave this as NR to let someone with RTBC power to give feedback.

Hey @danflanagan8; thanks for the feedback, I really followed all the codes you placed in the PR so far, so credit's on you really :-)

Let's do it like this and stick to the process:

1. consider my patch-work as a review on the current PR, so you can update the PR accordingly. For the record, I totally agree with the second part about the negated condition; the first part I could argue some, but I leave it up to you, it's more informative than crucial.
2. re-request a review when you feel the PR is in good shape
3. then I or anyone else can re-review and RTBC if there are no further points to discuss

If you agree, you can hide the patches I submitted so that the issue stays clean.

@stefanos.petrakis,
I pushed some commits to the issue fork. They're not showing up in the MR for some reason. I didn't create theMR so there are some permissions I don't have. The MR system is tough when someone creates the MR and then leaves the issue.

Maybe I'll have to create a new MR. I don't like MRs though so I would just as happily post a new patch. This is giving me indigestion so I'm going to step away for a bit.

Hey there, saw the commits, I can help with this next week, if you wanna go the patch way also fine, anything to avoid indigestion. :-p

Instead of ignoring the error result of the regex, shouldn't we change the regex to something that allows the filters to work?

This might work for example:

$chunks = preg_split('/(<[^>]+>)/is', $text, -1, PREG_SPLIT_DELIM_CAPTURE);

This gives the same results for simple HTML: https://3v4l.org/jHa5m

However, I'm not sure if this captures invalid HTML elements correctly.
Also, I don't know what the effect will be on performance.

Edit: maybe the pattern should be changed to (<[^>]+?>) instead, to allow <> elements

This issue is caused by deep backtracking as mentioned in #11
Changing the regex cannot solve the issue; it will still potentially fail when feeding the regex with a very long string (as is the case of base64 encoded inline images).

Possibly those changes should be combined then. I tested the patterns /(<[^>]+?>)/is and /(<[^>]+>)/is with the attached html. Both seem to work fine, in contrast to /(<.+?>)/is which fails.

If the way to go is to check if a regex fails when pcre.backtrack_limit is set to 1, all regex patterns in Drupal Core should be checked for this error. Or we should provide an advised minimum value for installations..

IMHO revising the regex pattern and possibly checking/optimizing similar patterns across the whole core is another issue.

My understanding is that this issue focuses on the code's behavior when preg_split encounters some failure (and returns FALSE); I would assume it could fail in other ways apart from reaching the backtracking limit, but I think the tests provided allow to cover at least that type of failure as well as proving that the code behaves correctly when FALSE is returned.

@sjerdo: I would suggest you open a related issue that focuses on modifying/tuning this and possibly other regexes; and we keep this issue focused on refactoring the affected codes, let me know what you think!

Well, that depends on the regex being used. I don't see the suggested regex pattern exceed a backtrack limit of 1.

For example, with the regex pattern I provided, the following code states no error occurred (int(0) / PREG_NO_ERROR):

$classes = str_repeat('dum-class ', 100000);
$text = '<div><p class="' . $classes . '">Not a url.</p></div>';

$pcre_backtrack_limit = ini_get('pcre.backtrack_limit');
ini_set('pcre.backtrack_limit', 1);

$chunks = preg_split('/(<[^>]+?>)/is', $text, -1, PREG_SPLIT_DELIM_CAPTURE);

$preg_last_error = preg_last_error();
var_dump($preg_last_error);
var_dump($preg_last_error === PREG_NO_ERROR);

ini_set('pcre.backtrack_limit', $pcre_backtrack_limit);

Test: https://3v4l.org/EWJM3

Unlike the original pattern, which does result in an error (int(2) / PREG_BACKTRACK_LIMIT_ERROR)

$classes = str_repeat('dum-class ', 100000);
$text = '<div><p class="' . $classes . '">Not a url.</p></div>';

$pcre_backtrack_limit = ini_get('pcre.backtrack_limit');
ini_set('pcre.backtrack_limit', 1);

$chunks = preg_split('/(<.+?>)/is', $text, -1, PREG_SPLIT_DELIM_CAPTURE);

$preg_last_error = preg_last_error();
var_dump($preg_last_error);
var_dump($preg_last_error === PREG_BACKTRACK_LIMIT_ERROR);

ini_set('pcre.backtrack_limit', $pcre_backtrack_limit);

Test: https://3v4l.org/cVJP1

In conclusion, it seems like the backtrack limit error check is superfluous for this method if the regex pattern is changed.

Can someone come up with a a test case in which the suggested pattern isn't sufficient?

Thanks @sjerdo!

I insist that improving the regex is another issue, complementary to this one but different.

This issue is about making the code more defensive regarding preg_split.
Even if the regex is improved, there is no guarantee preg_split would never return FALSE; this is the case that the issue is about AFAICT and the expanded test with minor refactoring (discussion in comments #17-#23) tries to tackle these.

I finally got the courage to come back and try to figure out how to get my commits to show up in the MR. (See #22 regarding my indigestion.) I clicked "Rebase" and then some magic happened! So I'm setting this back to NR.

I agree with @stefanos.petrakis regarding the scope of this issue (from #29):

This issue is about making the code more defensive regarding preg_split.

That's all that the MR does. No changes to any regex patterns.

That said, I'm always happy to be overruled by the community! Cheers!

Re-rolled against 9.5.x in a new PR !2842 (don't know how it's possible to now close the outdated PR !1247).

Testing on 9.4 Umami profile and you have to make sure that the "Convert URLs into links" is enabled on the text format so updating summary.

I have tested with and without the patch on 9.4 and it is working as expected. I still need to test on 9.5 and 10. If successful there, this can be moved to RTBC based on:

1. Title is clear and accurate
2. Updated issue summary's outdated proposed resolution
3. Has steps to reproduce that have been updated and verified
4. Metadata seems fine
5. Doesn't need screenshots
6. Code change seems straight forward
7. Tests have been added and pass
8. No coding standard issues in test bot
9. Code addresses issue and does not change unrelated things
10. Issue was reproducible
11. Manual testing on 9.4 was successful

Manual testing on 9.5 (php 8.0) and 10 (php 8.1) was successful, so moving to RTBC based on this and #37. Thanks, everyone!

I think we can slightly change the code to make things a bit more obvious - see comment on MR. Which is always nice in filters and loops. Nice test and I agree with the approach.

Targeting 10.1.x and picking up after review in PR. Coming up soon.

I changed the test to:

    $pcre_backtrack_limit = ini_get('pcre.backtrack_limit');
    // If a PCRE error occurs, we expect to get the same text.
    $input = $expected = file_get_contents($path . '/filter.url-input.txt');
    // Setting the limit to the smallest possible value so that it will break.
    ini_set('pcre.backtrack_limit', 1);
    // Make sure we got the same text back without any errors.
    $result = _filter_url($input, $filter);
    $this->assertSame($expected, $result, 'Complex HTML document was correctly processed.');

    // Setting limit back to default.
    ini_set('pcre.backtrack_limit', $pcre_backtrack_limit);

And this fails because $text = preg_replace_callback('`<!--(.*?)-->`s', '_filter_url_escape_comments', $text); fails! So I think we have more things to actually fix here. We need to do something like:

    // Store the current text in case any of the preg_* functions fail.
    $saved_text = $text;

And then at the end of the loop do:

    // If there is no text at this point revert to the previous text.
    $text = strlen((string) $text) > 0 ? $text : $saved_text;

This would be way easier to fix if we could break up _filter_url into methods on \Drupal\filter\Plugin\Filter\FilterUrl - my guess is we had an issue somewhere to move this functionality into that class.

I think the additions to the test case should be:

    $pcre_backtrack_limit = ini_get('pcre.backtrack_limit');
    // Setting the limit to the smallest possible value so that it will break.
    ini_set('pcre.backtrack_limit', 1);
    // If a PCRE error occurs, we expect to get the same text.
    $result = _filter_url($input, $filter);
    $this->assertSame($input, $result, 'Complex HTML document was correctly processed.');

    $result = _filter_url('<p>No url</p>', $filter);
    $this->assertSame('<p>No url</p>', $result, 'Complex HTML document was correctly processed.');

    // Setting limit back to default.
    ini_set('pcre.backtrack_limit', $pcre_backtrack_limit);

That way we'll have test coverage of errors during preg_replace_callback and preg_split

Right you are, I saw preg_replace_callback() breaking too , it seems that any preg_* function is a candidate for breaking when playing with the backtrack limit.

I went for a ternary default value when using preg_split() and preg_replace_callback() instead of the $saved_text idea. It seemed more compact but not too cryptic. And I would have had to add a little more coding to avoid deprecation warnings when passing $text=NULL to preg_split() and preg_replace_callback()

I went for the saved text approach for several reasons - apart from the one noted in the MR I don't want to have to think about how this behaves when the $text becomes something PHP would consider to be falsey - ie. "0"...

All righty then, the main MR (Merge request !2862) is now green following Alex's suggestions (thanks).
Also opened a related Task #3315489: Introduce composer/pcre (or similar) in order to handle preg_* functions failures that focuses on preg_* functions lacking some loud failing as we witnessed here.

Setting this to NR and I mean Merge request !2862 by that.

I reviewed MR !2862 and tested on Drupal 9.4.8. The testing went well.

I have one nit about the code changes: the original version has a blank line before this loop:

    $open_tag = '';

    for ($i = 0; $i < count($chunks); $i++) {

The blank line is lost in the MR. I like it better with the blank line, but not enough to hold up this issue.

The other thing that bothers me is that the outer loop escapes comments and then restores them for each iteration:

  foreach ($tasks as $task => $pattern) {
    // HTML comments need to be handled separately, as they may contain HTML
    // markup, especially a '>'. Therefore, remove all comment contents and add
    // them back later.
    _filter_url_escape_comments('', TRUE);
    $text = preg_replace_callback('`<!--(.*?)-->`s', '_filter_url_escape_comments', $text);
// ...
    // Revert to the original comment contents
    _filter_url_escape_comments('', FALSE);
    $text = preg_replace_callback('`<!--(.*?)-->`', '_filter_url_escape_comments', $text);
  }

Why not escape once, before the loop, and then restore them once, after the loop? That is out of scope for this issue, except that it would be easier to review if the MR did not re-indent 40 lines of code. Instead of

    if ($chunks !== FALSE) {
// 40 lines re-indented here, one blank line removed
    }

it could be

    if ($chunks === FALSE) {
      continue;
    }
// 41 lines unchanged

Again, I do not want to hold up this issue for a change that is arguably out of scope. If you decide to make those changes, then I will be happy to re-review.

Left some comments around complexity, otherwise this is looking good.
We've not had a 'tests only' failing patch here, but I think the committer can run it locally given its a Kernel test to confirm it fails as expected.

Actually, we have had a failing test only patch, see #17, ignore me - mixed up my issues

I discussed this issue with @larowlan on Slack. We agreed that the changes suggested in #47 and #48 are out of scope for this issue. I am setting the status back to RTBC, and we can make those simplifications in #3325466: Reduce complexity in _filter_url().

I meant to explain, in #50, why the suggestion in #48 (and on the MR) is out of scope. The suggested changes are to the code that I described in #47 as

// 40 lines re-indented here, one blank line removed

Thanks. Can confirm that patch #26 worked for me! I was getting this error "TypeError: count(): Argument #1 ($value) must be of type Countable|array, bool given in _filter_url() (line 539 of /web/core/modules/filter/filter.module)." when running this cron job: "Updates indexable active search pages".

I am running Drupal 9.4.9 with PHP 8.1.12 & Maria DB 10.4.27.

I could not replicate this error in my local or in a DEV environment. This only happened in PROD.

@benjifisher: Thanks for the review and follow-up! Rebased Merge request !2862

@elneto: The current coding work for this issue lives in Merge request !2862
The current patch can be obtained from https://git.drupalcode.org/project/drupal/-/merge_requests/2862.diff

Setting this back to NR since there was an unresolved thread (that I tried to resolve today), regarding deprecation notices, see the previous comment here.

First of all, it is distracting to have 4 MRs open for this issue. Can we close some of them?

I find the latest version of MR 2862 confusing:

  foreach ($tasks as $task => $pattern) {
    // HTML comments need to be handled separately, as they may contain HTML
    // markup, especially a '>'. Therefore, remove all comment contents and add
    // them back later.
    _filter_url_escape_comments('', TRUE);
    $text = is_null($text) ? '' : preg_replace_callback('`<!--(.*?)-->`s', '_filter_url_escape_comments', $text);

    // Split at all tags; ensures that no tags or attributes are processed.
    $chunks = is_null($text) ? [''] : preg_split('/(<.+? >)/is', $text, -1, PREG_SPLIT_DELIM_CAPTURE);

The doc block does not have any @param annotations, but I think $text is supposed to start as a string. Why are we testing for NULL at the start of the loop? At what point can $text ever become NULL?

Looking at the code, I think the answer is that preg_replace_callback() can return NULL if there is an error. So $text might be set to NULL at the start or the end of the loop.

This means we are expanding the scope of this issue. The current issue summary (IS) and title only mention failures in preg_split(). If we also want to handle failures in preg_replace_callback(), then we should update at least the IS, maybe also the title. I am also happy to go back to the version of the code that was RTBC and keep the current scope.

If preg_replace_callback() fails, is there any way to recover? I am pretty sure that the current code ends up setting $text to an empty string, then returns $saved_text. It would be clearer to return $saved_text right away:

    $text = preg_replace_callback('`<!--(.*?)-->`s', '_filter_url_escape_comments', $text);
    if (is_null($text)) {
      return $saved_text;
    }

    // Split at all tags; ensures that no tags or attributes are processed.
    $chunks = preg_split('/(<.+? >)/is', $text, -1, PREG_SPLIT_DELIM_CAPTURE);

Then do something similar at the end of the loop.

Also, the end of the function can be simplified. Instead of

  $text = strlen((string) $text) > 0 ? $text : $saved_text;

  return $text;

just make it

  return strlen((string) $text) > 0 ? $text : $saved_text;

Or maybe just return $text, if we are confident that we have returned early in all problem cases.

We could do more. For example, we could set $saved_text at the start of the loop. That way, if the replacement fails for one link type, then we could start over with the next type. But I think we should keep the changes in this issue simple. Just fail gracefully instead of getting PHP errors. We can do more in the follow-up issues #3315489: Introduce composer/pcre (or similar) in order to handle preg_* functions failures and #3325466: Reduce complexity in _filter_url().

Two more thoughts:

First, if we expand the scope to include failures in preg_replace_callback(), then we should test that. It should be good enough to add some HTML comments to the test fixture (filter.url-input.txt).

Second, it might be worth setting $saved_text at the start of the loop, because then you can avoid re-indenting 40-ish lines of code, as I mentioned in #47. Remove all references to $saved_text outside the loop, and then

  foreach ($tasks as $task => $pattern) {
    $saved_text = $text;
    _filter_url_escape_comments('', TRUE);
    $text = preg_replace_callback('`<!--(.*?)-->`s', '_filter_url_escape_comments', $text);
    if (is_null($text)) {
      // There is no point in trying the next iteration of the loop, since we
      // will end up right here.
      return $saved_text;
    }

    $chunks = preg_split('/(<.+? >)/is', $text, -1, PREG_SPLIT_DELIM_CAPTURE);
    if ($chunks === FALSE) {
      $text = $saved_text;
      continue;
    }

    // ...

    _filter_url_escape_comments('', FALSE);
    $text = preg_replace_callback('`<!--(.*?)-->`', '_filter_url_escape_comments', $text) ?? $saved_text;
  }

That code comment is for explanation here. I do not think you want to keep it. Whatever you do, do not copy my code snippets: I had to add a space in '/(<.+? >)/is' so that it would not be seen as a closing PHP tag.

Works as expected.

Committed and pushed e7ded38076 to 10.1.x and 687473d5fc to 10.0.x and bcd7a66c05 to 9.5.x. Thanks!

Thanks for the above fix,

Just hit this with PHP 8.1 / D9.5.5 and search_api, I added fields with phonetic and spellcheck type in the search index, indexed content, it got half way and on some weird content indexing an exception occured as described in this patch declaration
using this patch until a tagged release comes:

"3239472 - PHP 8.1 filter when using spellcheck search api field type and phonetic, indexing came up with this TypeError: count(): Argument #1 ($value) must be of type Countable|array, bool given in _filter_url() (line 539 of core/modules/filter/filter.module": "https://git.drupalcode.org/project/drupal/-/commit/bcd7a66c05d470271f49939cf5e6925892041da3.diff"

Encountered this issue today. Fixed by updating to Drupal 9.5.7.

[17-Apr-2023 13:06:48 America/Toronto] TypeError: count(): Argument #1 ($value) must be of type Countable|array, bool given in /home/USER/public_html/core/modules/filter/filter.module on line 539 #0 /home/USER/public_html/core/modules/filter/src/Plugin/Filter/FilterUrl.php(42): _filter_url('<p>\xC2\xA0</p>\n\n<p><...', Object(Drupal\filter\Plugin\Filter\FilterUrl))
#1 /home/USER/public_html/core/modules/filter/src/Element/ProcessedText.php(118): Drupal\filter\Plugin\Filter\FilterUrl->process('<p>\xC2\xA0</p>\n\n<p><...', 'en')
#2 [internal function]: Drupal\filter\Element\ProcessedText::preRenderText(Array)
#3 /home/USER/public_html/core/lib/Drupal/Core/Security/DoTrustedCallbackTrait.php(101): call_user_func_array(Array, Array)
#4 /home/USER/public_html/core/lib/Drupal/Core/Render/Renderer.php(788): Drupal\Core\Render\Renderer->doTrustedCallback(Array, Array, 'Render #pre_ren...', 'exception', 'Drupal\\Core\\Ren...')
#5 /home/USER/public_html/core/lib/Drupal/Core/Render/Renderer.php(374): Drupal\Core\Render\Renderer->doCallback('#pre_render', Array, Array)
#6 /home/v/USERublic_html/core/lib/Drupal/Core/Render/Renderer.php(204): Drupal\Core\Render\Renderer->doRender(Array, false)
#7 /home/USER/public_html/core/modules/views/src/Plugin/views/field/EntityField.php(934): Drupal\Core\Render\Renderer->render(Array)
#8 /home/USER/public_html/core/modules/views/src/Plugin/views/field/FieldPluginBase.php(1171): Drupal\views\Plugin\views\field\EntityField->render_item(0, Array)
#9 /home/USER/public_html/core/modules/views/views.theme.inc(238): Drupal\views\Plugin\views\field\FieldPluginBase->advancedRender(Object(Drupal\views\ResultRow))
#10 [internal function]: template_preprocess_views_view_field(Array, 'views_view_fiel...', Array)
#11 /home/USER/public_html/core/lib/Drupal/Core/Theme/ThemeManager.php(287): call_user_func_array('template_prepro...', Array)