Closed Bug 998785 Opened 8 years ago Closed 8 years ago
an error occurred while executing regular expression
The following regular expression does not work as expected: /<([\w\:]+)((?:[\s\w:=]+|'[^']*'|"[^"]*")*)(?:\/>|>([\d,]*)<\/[^>]+>)/g.test("<rdf:Description rdf:about=\"\" xmlns:test=\"http://test.com/pdf/1.3/\">2,<t>3,</t></rdf:Description>") It throws "an error occurred while executing regular expression" to the console. (Also hangs the Chrome browser tab with 100% CPU usage)
Before bug 953013 it would just claim to not match (possibly incorrectly)... Till, is there a way to simplify that regexp to avoid the capture group issues?
I talked to yury and he has a workaround already. The expression from comment 0 hangs both Chrome and IE, with no slow script dialog showing up. It might be good to check this expression once our irregexp import is done: we shouldn't also import the hang, after all.
Added this bug to the site compat doc. Corrections welcome. https://developer.mozilla.org/en-US/Firefox/Releases/29/Site_Compatibility https://twitter.com/FxSiteCompat/status/465897876640776192
The relevant compat bit is that the regular expressions that now throw used to claim "no match" even if they should have matched. So the old behavior was definitely incorrect.
Thank you Boris, I have just modified the document.
At: https://developer.mozilla.org/en-US/Firefox/Releases/29/Site_Compatibility#Regex_with_999998.2B_groups_now_raises_an_InternalError_(regressions_reported) there's a mention that there was also a problem with $.fn.trim in older jQuery versions. What versions are that and what's the reproducible use case? I'm asking because we've recently switched the $.fn.trim implementation in jQuery (in recently released 2.1.1/1.11.1) to not use String.prototype.trim as our implementation works good and we had to keep it anyway due to buggy trim in Android<4.1.
As per Bug 1006744, jQuery 1.4.2 is known to be affected and 1.10.2 is not. I'll check details later.
(In reply to Kohei Yoshino [:kohei] from comment #9) > As per Bug 1006744, jQuery 1.4.2 is known to be affected and 1.10.2 is not. > I'll check details later. We don't have capture groups in regexes for trim in newer jQuery versions so it'll be fine. Thx for the info!
simplified example from 1010154 : var a = ''; for(var i=0; i<19; i++) a+= "\xa0"; // if you got less symbols - error not occurs a = a+'1'; // error not occurs without this var r = /(\s|\xA0)+$/; a.replace(r, ''); // here error
(In reply to Kohei Yoshino [:kohei] from bug 976446 comment #76) > So Bug 998785 has to be fixed separately, in some way. Does it? To me, that depends on how much breakage we experience because of it. For now, it seems pretty managable, but I might be wrong. Keep in mind that this change fixes potential site-security issues and doesn't break things that truly did work before. Obviously, all that doesn't help us if too many sites really do break for their users, where previously some implementation detail didn't work as expected but the user didn't have to care about that. The thing is, it's hard to fix this. What we could do, I guess, is increase the limit after which we throw by, say, 10x. That'd cause lots of affected sites to stall for a few seconds but then continue on, instead of breaking. It might well be the right band-aid for the two releases before the switch to irregexp.
I think this should be fixed even if it's considered as a potential breakage, because * In some cases it seems like difficult for Web developers to examine and fix the issue as even a simple replace method like Comment 12 is also affected. * It also affects the older versions of the popular jQuery library * Firefox 31 will be the next ESR
(In reply to Kohei Yoshino [:kohei] from comment #14) > I think this should be fixed even if it's considered as a potential > breakage So you think we should deliberately re-introduce a bug that potentially opens sites up to attacks? I very much disagree.
Why not fix the error while avoiding potential security issues? Code like Comment 12 should be valid, Chrome and Safari won't throw. For Web developers it might be an unacceptable side effect.
(In reply to Kohei Yoshino [:kohei] from comment #16) > Why not fix the error while avoiding potential security issues? Code like > Comment 12 should be valid, Chrome and Safari won't throw. Because doing that is *hard*. Chrome doesn't throw because it actually handles the regexp correctly. Safari doesn't throw because it has the same issue that we had before we fixed this: they still have the potential security issue. Please read bug 953013 and the blog post linked from it if you want to better understand the history of this.
Also, switching to irregexp *is* the fix for this. Just as with many other fixes we do, this one isn't backportable, unfortunately.
I have already read Bug 953013 and the blog post. Seems like the bug is a longstanding issue and the reporter says "all scenarios are solely theoretical". My understanding here is the backward compatibility impact is larger than the potential security risk.
Argh. I mid-aired with comment 17 and then Bugzilla lost the comment. Let's try to recreate it, and my apologies for the terseness. Kohei, Safari matches incorrectly on my testcase in bug 1010154, if you try it... Till, how many of the uses in these bugs are things like test() (this bug) or replace() with an empty replacement string (the jQuery thing)? Those could be recompiled with capturing disabled, no? Would that be a viable backportable fix? Because Kohei is right: we've gotten a _lot_ of bug reports here, the fact that this bites some jQuery versions means there are likely tons of other broken cases where we're not getting bug reports, and shipping that sort of breakage in ESR is really not very palatable. Bumping 10x would only give freezes in the 0.5 second range on desktop hardware, but would also mean we only get about 3 extra chars of length on cases like bug 1010154, right?
Luckily we haven't got a bunch of breakage reports so far, but this has already affected some applications and their users for real. https://www.mozilla-hispano.org/foro/viewtopic.php?f=2&t=18264 (http://www.cbox.ws/) http://agilefant.freeforums.org/viewtopic.php?t=587&p=2437 (http://agilefant.com/) As end users usually won't see the error message, we cannot judge the actual impact based on quick Google searches. I'm tracking this bug in my regression dashboard. https://docs.google.com/spreadsheets/d/1048AVpvbP3O2atsV3i--xLpHCr4PR7TGXyFtgMrvt6Q
Regression + common feature: tracking.
Reports: https://input.mozilla.org/en-US/dashboard/response/4415123 http://forums.mozillazine.org/viewtopic.php?f=9&t=2831651 Selecting "All" from the "Funds / page" dropdown on this page never shows the results. http://webfund6.financialexpress.net/clients/fpi/FundCentre.aspx I have investigated the issue with the console and debugger and found that the XHR callback is never called. This site is using jQuery 1.4.2, and the XHR also triggered with jQuery.ajax(). OK: Firefox 28, 32 NG: Firefox 29, 30, 31 I guess this issue is caused by the regex breakage.
Bug 976446 has already been resolved.
(In reply to Kohei Yoshino [:kohei] from comment #23) > I guess this issue is caused by the regex breakage. No errors reported in the Console though.
Severity: normal → major
It's too late in the FF30 beta cycle to take on a speculative fix for this and given there's a proper rewrite of the regex engine coming in FF32, I don't see us getting resources put into a temp fix just for FF31. Likely we should untrack there.
Comment 20 sounds it's not difficult to fix. And as I reported in Comment 23 and 25, specific usages of jQuery.ajax() are affected and interrupted without throwing. It's difficult for developers to debug it. A right solution here is upgrading jQuery but many sites still have older versions of jQuery. Even MSN is also using jQuery 1.4.2, as per http://w3techs.com/technologies/details/js-jquery/1/all Can we *at least* throw properly and have a better error message in the console?
Lukas, I'd agree with the 31 bit if 31 were not the next ESR... > Can we *at least* throw properly We do throw properly. If you're not getting an exception, either you're not seeing this bug or the problem is not in the code you think it's in.
I'm looking into this over the next couple of days. Ideally, I can implement Boris' suggestion from comment 20, but I'm not yet sure that that'll be small enough a change to uplift.
(In reply to Boris Zbarsky [:bz] from comment #28) > We do throw properly. If you're not getting an exception, either you're not > seeing this bug or the problem is not in the code you think it's in. Or maybe jQuery is catching the exception internally, I guess.
The trim method in jQuery 1.4.2 is failing if the string to trim contains more than 1119 consecutive space characters. Is this scenario covered by the current issue? Here's a way to reproduce: <html><head> <meta charset="UTF-8"> </head> <script> a = 'Test Test"}'; var Wa = /^(\s|\u00A0)+|(\s|\u00A0)+$/g a.replace(Wa, ""); </script></html>
> Is this scenario covered by the current issue? Sort of. In older Firefox versions that replace() call just doesn't do anything. In more recent ones it throws instead....
As discussed, this is a minimal backout of bug 953013. It applies cleanly on beta and aurora, where we should land it to revert to the old behavior until FF32, which contains the new irregexp engine and doesn't have this issue at all.
Attachment #8432234 - Flags: review?(jdemooij)
Assignee: nobody → till
Status: NEW → ASSIGNED
Attachment #8432234 - Flags: review?(jdemooij) → review+
Comment on attachment 8432234 [details] [diff] [review] Don't throw error for regexps Yarr bails on [Approval Request Comment] Regression caused by (bug #): bug 953013 User impact if declined: sites behaving incorrectly/ not working at all Testing completed (on m-c, etc.): no, doesn't affect m-c Risk to taking this patch (and alternatives if risky): extremely low to non-existant. Straight, minimal backout of the patch in bug 953013, restores previous, extensively tested, behavior String or IDL/UUID changes made by this patch: none After thorough investigation, I've come to the conclusion that there's nothing else we can do here: the issue causing this is just too deeply engrained in the architecture of Yarr. I discussed this with security people, bz and jandem, and they're all ok with this change. (Requesting release approval for completeness' sake. Should there be a chemspill planned for other reasons, I think this could ride along. Doing a chemspill for it isn't warranted, though.)
Comment on attachment 8432234 [details] [diff] [review] Don't throw error for regexps Yarr bails on I am taking it for aurora. This is unlikely we are going to do an other 29. So, rejecting. For beta, Lukas will decide if she wants it or not.
(In reply to Sylvestre Ledru [:sylvestre] from comment #36) > For beta, Lukas will decide if she wants it or not. Note that this affects some versions of jQuery, so the long tail of affected sites is probably quite long.
Comment on attachment 8432234 [details] [diff] [review] Don't throw error for regexps Yarr bails on Let's get this in now for today's RC.
Attachment #8432234 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.