Closed Bug 452451 Opened 11 years ago Closed 10 years ago

enable relimit by default and find out if it breaks the web

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.3a5
Tracking Status
blocking2.0 --- beta1+

People

(Reporter: crowderbt, Assigned: mounir)

Details

Attachments

(1 file, 1 obsolete file)

Bug 330569 introduced a feature that allows us to bail out of exponentially (worse than O(n^3)) complex regular expressions fairly quickly, depending on the length of the input string -- this might need more tweaking, see below.

We should turn on this feature by default in the browser (until now it has only been included as an "off" about:config feature), and see how much stuff breaks.  We may also want to install a hard upper-limit on the number of backtracks we're EVER going to do for an expression (another about:config option with a configurable number?) -- right now we base the logic only on the length of the input string.
There'a bug in nsJSEnvironment.cpp's GetOptionsProperty -- it censors updates to JSOPTION_RELIMIT. SetOptionsProperty was updated to allow JSOPTION_RELIMIT in addition to STRICT and WERROR, but GetOptionsProperty was not updated.

/be
Assignee: general → sayrer
blocking2.0: --- → ?
Attached patch Patch v1 (obsolete) — Splinter Review
This is making relimit 'on' by default in the browser and this is fixing the issue reported by Brendan on comment 2.

This patch isn't making relimit 'on' by default in the js engine AFAICT.
Assignee: sayrer → mounir.lamouri
Status: NEW → ASSIGNED
Attachment #444402 - Flags: review?(jwalden+bmo)
Attachment #444402 - Flags: review?(jonas)
For the upper limit of backtracks, if you have a value to propose, I can try to implement it. Otherwise, I think a follow-up would be better.
Attachment #444402 - Flags: review?(jonas) → review?(jst)
Comment on attachment 444402 [details] [diff] [review]
Patch v1

I'm not the right person to review this
I haven't done much more than skim just yet, but at first glance this looks like a straightforward change that's probably fine.

The question is mostly one of policy and whether we *should* do this.  I think we should, to not spiral so far out of control on garbage input -- adding more people to hear potential arguments for why we shouldn't...
Seems fine with me fwiw.
I think PCRE was using a hard limit of something like 1,000,000 backtracks.  We might want to investigate the limits set by other engines on the web?
The issue here is whether we hide bugs. Sometimes we hear "hangs Firefox, does not hang (Safari|Chrome|...)". Reasons vary (including user-agent sniffing, evil) but I seem to recall occasionally there's a bug in our regexp code that leads to exponential blow-up where other engines avoid that. Or we just don't have a l33t optimization, where that optimization would mismatch quickly.

If we switch to YARR then no worries :-P.

/be
In all the cases I can remember, the Safari engine just errors out after some limit, while we keep on churning until the slow script timer breaks our face.
Ok, let's try this then. It's pre-beta, good time to turn it on and see if our face breaks less.

/be
Attachment #444402 - Flags: review?(jst) → review+
Attachment #444402 - Flags: review?(jwalden+bmo) → review+
I assume there is no sr needed. Marking checkin-needed.
Keywords: checkin-needed
Pushed: http://hg.mozilla.org/mozilla-central/rev/1711cfd31e7d
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5
Failed Jo/Jd on all platforms. (one debug log: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1274325551.1274326694.6306.gz&fulltext=1 )

Failing js1_5/extensions/regress-330569.js

Pushed backout: http://hg.mozilla.org/mozilla-central/rev/43e4cc9cb6a8
and merge: http://hg.mozilla.org/mozilla-central/rev/2e60eafd92ba

I have a gut feeling this can be fixed by fixing the failing test, but I held the tree too long, and needed to backout.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch Patch v1.1Splinter Review
Oups, I didn't know js tests where run from the browser in the tinderbox. I tried them but with jsDriver.pl. The relimit js tests were assuming the relimit was always disabled and |options('relimit')| was disabling relimit instead of enabling it because the patch is enabling relimit for the brower.
Anyway, this new patch should works correctly (tested locally at least).

Sorry for the annoyance, Justin.
Attachment #444402 - Attachment is obsolete: true
Keywords: checkin-needed
blocking2.0: ? → final+
blocking2.0: final+ → beta1+
(In reply to comment #14)
> Created an attachment (id=446622) [details]
> Patch v1.1

Repushed as: http://hg.mozilla.org/mozilla-central/rev/12af2ba87d4c
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Keywords: checkin-needed
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.