Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b12pre) Gecko/20110216 Firefox/4.0b12pre ID:20110216030352 If you are using regular expressions and pass those as function parameter to a method which is part of a securable module, the evaluation of the regular expression always fails. It works if the function is contained in the same module. regex module: ============= exports.matches = function matches(aString, aRegex) { return aString.match(aRegex); } test: ===== var mod = require("regex"); function testElements() { var controller = mozmill.getBrowserController(); controller.window.alert(mod.matches("http://www.mozilla.org", /mozilla/)); } Also see the attached testcase. Steps to reproduce: 1. Download the test environment (zip file) according to your system (http://people.mozilla.com/~hskupin/mozmill-crowd/) 2. Extract environment and execute setup.sh(cmd) 3. Execute run.sh(cmd) mozmill -b %path_to_Minefield% -t test.js Check the alert message which pop-up. It will not match the given string. This has been regressed on trunk between the builds 10101307 and 10101503. Sadly I can't get a more exact regression range because Mozmill is broken in the 10101403 build. PASS: 178f26e21cfc FAIL: 19cb42fa4554 Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=178f26e21cfc&tochange=19cb42fa4554 I assume it's related to the tracemonkey merge which happened in that timeframe. I will try to get a better testcase and narrow down the range.

This test can be executed as part of the addons SDK. Just place it under example tests and execute with 'cfx test'.

I did another test with a simple web page and an included .js file and everything works fine. Blake, could that be related to compartments?

(In reply to comment #0) > > regex module: > ============= > > exports.matches = function matches(aString, aRegex) { > return aString.match(aRegex); > } This seems like a serious bug, but ... why do we need a shared module for this? /mozilla/.test("mozilla.org") is just as short as regex.matches("mozilla.org", /mozilla/) and is much more direct, don't think we should be hiding from javascript and making new APIs to remember the syntax for. Does this regex fail in a js console too?

Ok, with the Mozmill add-on I can narrow it down even more. It's not affected by the bustage we had during that day. The build from 10101403 is also affected. So the pushlog is http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=178f26e21cfc&tochange=ad0a0be8be74 I will hg bisect to find the causing checkin.

(In reply to comment #3) > This seems like a serious bug, but ... why do we need a shared module for this? Heather, this is a testcase nothing more.

Discussed in driver triage today - can't block on something that seems to only impact MozMill. Please re-nom if getting a neater regression range makes us believe this might impact a broader range of sites.

Johnathan, this does not only impact Mozmill. It impacts all projects which are using securable modules, including Add-ons SDK. I will have the causing patch hopefully soon.

Here is a simple test case that highlight a problem between Sandbox and regexp used on String.prototype.match. If you want an even more simple test, here is a snippet that you can paste in your JS console: var sandbox = Components.utils.Sandbox("http://mozilla.org"); Components.utils.evalInSandbox("var regexp = /mozilla/;", sandbox); ["http://mozilla.com".match(sandbox.regexp), sandbox.regexp.test("mozilla")] string.match will return null whereas regexp.test will succeed! (I run these test on firefox 4b11)

Let's put that back in the queue then! Thanks Henrik, merci Alexandre.

It has been definitely regressed by the tracemonkey merge. There are 4 checkins which are related to the sandbox. It's a heavy task on my machine to compile but I will make sure to have the result in the next 2 hours.

Regressor is: bug 580128 - Make evalInSandbox work with the new wrappers. r=peterv author Blake Kaplan <mrbkap@gmail.com> Fri Sep 17 14:54:41 2010 -0700 (at Fri Sep 17 14:54:41 2010 -0700) changeset 55208 b503f60e4245 parent 55207 08a441b59c4a child 55209 d6a91228ac5a pushlog: b503f60e4245

I am still inspired by mrbkap after our last trip through this code together, so I thought I'd try a bit of debugging (hope it's helpful)... So you can take the statements from Alexandre in comment 8 and run them in xpcshell in order to see the behavior while you're debugging. I also ran: var re2 = /mozilla/; "http://mozilla.com".match(re2) as well so I could compare the execution without the sandbox. So, I stumbled upon the right code to watch the wrapper being constructed but I've now lost it and can't find it. Essentially I saw the wrapper get constructed for the regex inside the sandbox. When it is crafted, it is crafted as a string and not a regex. This wrapped string object then winds up getting passed to the code at jsstr.cpp:1866[1]. The init code being called there is at jsstr.cpp:1661. In the non-sandbox case, the value is seen as a regexp, and we hit the "if" branch of the conditional. Then regxp matching is tried and it passes. In the sandbox mode, the "else" condition is hit (because the value is unwrapped and found to be a string) and we attempt flat pattern matching. However, the flat "/mozilla/" is fed to the flat pattern matcher, which does not recognize the '/' characters [2] and then decides that the two strings ('mozilla' versus '/mozilla/') do not match. I'm really kicking myself for not keeping my breakpoints where the object was constructed into a string rather than a regexp. But my guess is that mrbkap probably knows where that is. I think if the value is seen as a regexp and casted as such before it is wrapped, it will solve this bug. I'm not sure where the tests are for this code, but we might want to ensure we have test coverage of evalInSandbox with other jsnative objects which have operations (dates, for example) and ensure they work properly in the sandbox and out. This might just be specific to regexes because they can so easily masquerade as a string, but it would be a good thing to ensure we have test coverage for nonetheless. [1]: http://mxr.mozilla.org/mozilla-central/source/js/src/jsstr.cpp#1866 [2]: jsregexpinlines.h:536 (called with '/mozilla/')

Based on the latest, I'm speculatively marking this a final+ hardblocker - breaking regexes in sandboxes feels big enough to hurt. Blake?

I think I am more familiar with the wrapper layering here. Stealing.

comment 8 test case is great, thanks

Shell testcase: var sandbox = evalcx(""); evalcx("var regexp = /mozilla/", sandbox); print(["http://mozilla.com".match(sandbox.regexp), sandbox.regexp.test("mozilla")])

Better test case: var sandbox = evalcx(""); evalcx("var regexp = /mozilla/", sandbox); print("http://mozilla.com".match(/mozilla/)); print("http://mozilla.com".match(sandbox.regexp));

This is not just a problem for regexp objects. Proxies cannot model internal hidden magic properties: var sandbox = evalcx(""); evalcx("var date = new Date(5)", sandbox); print(Date.prototype.getTime.call(new Date(5))); print(Date.prototype.getTime.call(sandbox.date)); This prints: 5 x2.js:4: TypeError: Date.prototype.getTime called on incompatible Proxy

Table 9 of the ES5 spec on page 34 lists all internal properties that are not common between all objects (the latter the proxy spec already handles nicely). [[PrimitiveValue]] is a problem for Boolean, Date, Number and String [[Match]] is a problem for RegExp All other cases we handle somehow I think. There is an easy fix for this: unwrap. However, that would be an information leak since the policy on the wrapper has no chance of moderating that information flow. In our current product this only affects chrome <> content regexp/Boolean (very rare)/Date (more common)/Number (very rare)/String (pretty rare) use. content<>content is same compartment (no wrappers), or its cross origin and property access is not allowed anyway. Cross origin document.domain is affected as well.

I am inclined to not fix this for now and relnote it. This mostly affects chrome code, and there is an easy workaround: print("http://mozilla.com".match(/mozilla/)); // works print("http://mozilla.com".match(sandbox.regexp)); // fails function RecompileRE(re) { re = re.toString(); return RegExp(re.substr(1, re.length - 2)); } print("http://mozilla.com".match(RecompileRE(sandbox.regexp))); // works Similar tricks can be used for Date/Number/Boolean. Unblocking and setting relnote flag. Please re-nominate if you disagree.

Don't use substr, it's informative only (Annex B) and an ugly old Perlism. return RegExp(re.toString().slice(1, -1)); is shorter and sweeter. But wait, what if the regexp had a /g or other flag? js> re = /hi/g /hi/g js> re2 = RegExp(re.toString().slice(1, -1)) /hi\// The way to clone a regexp without pulling its .source, .global, etc. properties out is just return new RegExp(re); (the new is required). But that won't work with a wrapper. Perhaps it should, though: unwrap in the RegExp constructor. Wrapper non-transparency sure seems like a bug, even if not a blocker. /be

This patch narrowly fixes String.prototype.match for the case that a wrapped regular expression is passed in. It is pretty easy to use wrapped regexps' whereas it is really difficult to use a wrapped Date object the wrong way (call and apply are needed), so it might make sense to land this patch. I don't much like it because its really ugly (so is the underlying code it touches). This patch basically does toString() on the regexp and then strips of the // left and right and we build a new regexp from it. Retrieving the regexp via toString() allows the security layering to moderate this information flow.

Comment on attachment 513918 [details] [diff] [review] patch > static JSLinearString * >-ArgToRootedString(JSContext *cx, uintN argc, Value *vp, uintN arg) >+ArgToRootedString(JSContext *cx, uintN argc, Value *vp, uintN arg, bool unwrapWrappedRegExp = false) > { Why wouldn't we always do this for regexps? When would we want to have the old behaviour for wrappedRegexp -> string conversion? >+ if (vp->isObject()) { >+ JSObject *obj = &vp->toObject(); >+ bool isWrappedRegExp = unwrapWrappedRegExp && obj->isWrapper() && >+ JSWrapper::wrappedObject(obj)->isRegExp(); >+ if (!DefaultValue(cx, &vp->toObject(), JSTYPE_STRING, vp)) >+ return NULL; >+ if (isWrappedRegExp && vp->isString()) { >+ JSLinearString *str = vp->toString()->ensureLinear(cx); >+ size_t start = 0, end = str->length(); >+ if (end > 0 && str->chars()[0] == '/') >+ ++start; >+ if (end > start && str->chars()[end - 1] == '/') >+ --end; >+ if (start != 0 || end != str->length()) { >+ JSString *depstr = js_NewDependentString(cx, str, start, size_t(end - start)); >+ if (!depstr) >+ return false; >+ vp->setString(depstr); >+ } >+ } >+ } if (vp->isObject() && (obj = &vp->toObject()) && obj->getClass == &js_RegExpClass) { JSString *source = RegExp::extractFrom(obj)->getSource(); if (!source) return false; vp->setString(source); }

vp->isObject implies &vp->toObject() cannot be null, as does the return type of toObject. /be

Many callers to ArgToRootedString simply take a string, so we want the unmodified value of the default value conversion. Only this one specific caller wants a regexp.

I am not sure what comment 24 refers to.

Er, yes, I wanted a |,| and not a && there; got lost in the parenthesization. Thanks.

He was correcting my over-wrought replacement, which can be slightly simpler: if (vp->isObject() && (obj = &vp->toObject(), obj->getClass() == &js_RegexpClass)) { ... }

(In reply to comment #21) > js> re = /hi/g > /hi/g > js> re2 = RegExp(re.toString().slice(1, -1)) > /hi\// For now I'm using the following workaround which also takes the flags into account: let pattern = flags = ""; try { let matches = aRegex.toString().match(/\/(.*)\/(.*)/); pattern = matches[1]; flags = matches[2]; } catch (ex) { } let regex = new RegExp(pattern, flags);

Resolving as WFM because this bug seems to be fixed. Test case from comment #16 reports now: mozilla,true Test case from comment #17 reports now: mozilla mozilla Test case from comment #18 reports now: 5 5