Page MenuHomePhabricator

Regex in JavascriptDistiller causes stack overflow on windows
Closed, ResolvedPublic

Description

MW 1.18 causes apache to crash on windows on every load.php request. This is due to a regex in JavascriptDistiller with bad backtracking behavior:

'/\\/\\*(.|[\\r\\n])*?\\*\\//'

Replacing the (.|[\\r\\n]) part with \\C solves the problem


Version: 1.18.x
Severity: normal

Details

Reference
bz27449

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 21 2014, 11:19 PM
bzimport set Reference to bz27449.

I see nothing wrong with the regex itself... is this a configuration error or a bug in PHP?

(In reply to comment #2)

I see nothing wrong with the regex itself... is this a configuration error or a
bug in PHP?

See comment #1 and http://www.mediawiki.org/wiki/Apache_configuration#Thread_stack_size. It's a default config issue in apache.

(In reply to comment #1)

See
http://old.nabble.com/ResourceLoader-%2B-Windows-%2B-PHP-bug-47689-to30792236.html

Right, that's where I saw this. However, the suggested fix to increase the stack size only solves the symptom, while I don't think it's necessary to use regexes which need recursion depth proportional to the length of the string passed.

(In reply to comment #4)

(In reply to comment #1)

See
http://old.nabble.com/ResourceLoader-%2B-Windows-%2B-PHP-bug-47689-to30792236.html

Right, that's where I saw this. However, the suggested fix to increase the
stack size only solves the symptom, while I don't think it's necessary to use
regexes which need recursion depth proportional to the length of the string
passed.

Fair enough. I am a bit worried that the more things use RL, the more easily it will overflow. It's something to be looked at.

It apparently needs the capture (of the code comments), which will get worse an worse as the input lengthens.

Tim recently changed the regex to be multiline and use . instead of (.|[\r\n]). Does this fix the issue?

Just to help keep things in sync...

In bug #27528 an entirely new alternative to JavaScriptDistiller called JavaScriptMinifier is proposed, which if used may close this bug.

(In reply to comment #6)

Tim recently changed the regex to be multiline and use . instead of (.|[\r\n]).
Does this fix the issue?

Yes.