0

We are having an odd issue with Stacks in Concrete5.7: we are starting to collect quite a few stacks (64 currently) and our server has started throwing server errors (PHP Fatal error: Maximum execution time of 30 seconds exceeded in /example/path/to/website/concrete/blocks/html/controller.php on line 89) when editing pages, specifically when getting /ccm/system/panels/add?cID=2468&tab=stacks via XHR.

I temporarily solved this by increasing max_execution_time from 30 to 60 in php.ini, but this seems like a poor workaround that I will have to again bump after adding more content to Stacks.

Is there something else I can do besides just blindly increasing max_execution_time?

Tracing into blocks/html/controller.php:89, that is in the xml_highlight() function; specifically, this line (see the code in context here):

    $s = preg_replace(
        "#<(.*)(\[)(.*)(\])>#isU",
        "&lt;\\1<font color=\"#800080\">\\2\\3\\4</font>&gt;",
        $s
    );

This seems like a fairly straightforward regex to me; am I missing something?

It also occurs to me that I could remove all the preg_replace calls from xml_highlight() and check performance, but I am unsure what functionality I would be losing by doing so.

For reference, from dashboard/system/environment/info:

# concrete5 Version
Core Version - 5.7.5.2
Version Installed - 5.7.5.2
Database Version - 20150731000000
Community
  • 1
  • 1
Matt Rice
  • 646
  • 5
  • 18
  • 2
    What is `"#<(.*)(\[)(.*)(\])>#isU"` supposed to match? You might try changing it to `"#<([^[]*)(\[)([^]]*)(])>#i"` and see if it helps. All the regexps there are `.`-based, which is not good when you deal with marked up text. – Wiktor Stribiżew Apr 18 '17 at 17:31
  • 1
    It's always a bad sign when a regex author appends the `U` option to every regex, as if it were a magical blue pill that solves all performance problems. – Alan Moore Apr 18 '17 at 20:05
  • @WiktorStribiżew I think the `s` option might be necessary though, as the matched code is/could be HTML with (user-entered) line breaks. – Matt Rice Apr 19 '17 at 18:57
  • 1
    @MattRice, `s` option makes no sense when a pattern has no `.`. – Wiktor Stribiżew Apr 19 '17 at 19:13

1 Answers1

2

The problem with that regex is that it has two instances of .* in it, and the s option at the end that lets . match newlines.

That means, after it finds a &lt; it has to scan potentially the whole rest of the text looking for [, and then again looking for ]&gt;. The U option means it will try the shortest matches first, but it will keep trying until it finds a match or eliminates all possibilities. And it will do that for every &lt; in the document.

Changing the regex as Wiktor suggested should solve the problem, but I would take it a step further and use possessive quantifiers:

"#&lt;([^[]*+)(\[)([^]]*+)(\])&gt;#i"

The other regexes in that function are just as badly written, but they contain at most one .* each, so they don't try to crash the system.

Community
  • 1
  • 1
Alan Moore
  • 73,866
  • 12
  • 100
  • 156
  • Are you missing an escape character in capture group 4 (the right square bracket)? i.e. should `(])` be `(\])`? – Matt Rice Apr 19 '17 at 19:16
  • 1
    Yes and no. It should work as is, but I usually escape it anyway, because some flavors insist on it. This time I just copy-pasted it out of RegexBuddy, where I composed it. I'll go ahead and escape it, just to be safe. – Alan Moore Apr 19 '17 at 20:42