Closed Bug 815010 Opened 8 years ago Closed 8 years ago
"Assertion failure: has
Script()" with set Timeout(Array .prototype .index Of)
Assertion failure: hasScript(), at js/src/jsfun.h:203
Not an XPConnect problem. This API should be safe to call on all JSFunctions and should return null if there is no script. In this case, it looks like isInterpreted() is true but hasScript() is false, so JSFunction::maybeNonLazyScript totally fails to do the right thing. I will bet money this is fallout from bug 784294.
Assignee: nobody → general
Oh, and this seems security-sensitive if the assert really means we'll treat random memory as a JSScript*....
No call on your bet from my side: that's spot on. maybeNonLazyScript does what was intended for interpreted functions, but that's not what's desirable in this case. The expectation is for the (maybeN|n)onLazyScript functions to be used iff the call site can guarantee the script to be initialized. E.g., during execution of the script itself. JS_GetFunctionScript isn't such a call site and should have used getOrCreateScript. The attached patch changes to that and correctly returns NULL for native functions or the (possibly just initialized) script for interpreted ones.
Comment on attachment 685090 [details] [diff] [review] Initialize lazy script in JS_GetFunctionScript Seems reasonable at first glance. One question: For self-hosted stuff, will this return a non-null JSScript? If so, what principal is that compiled with?
Attachment #685090 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky (:bz) from comment #5) > Comment on attachment 685090 [details] [diff] [review] > Initialize lazy script in JS_GetFunctionScript > > Seems reasonable at first glance. One question: For self-hosted stuff, will > this return a non-null JSScript? If so, what principal is that compiled > with? Yes, it will always return a JSScript for interpreted functions. For self-hosted functions that haven't yet been called, it will clone the script in getOrCreateScript. The script isn't compiled for the global that it's cloned into. Instead, the version that was compiled during self-hosting initialization is cloned. Self-hosted code is compiled with a NULL principal, but I now see that cloned scripts, while getting the source script's originPrincipals, get the current compartment's principals. (at jsscript.cpp:2252) Luke, can you comment on the implications of that?
In practice, I suspect it doesn't matter that much. The only question is what should happen when Array.prototype.whatever from one window (e.g. chrome) is poked at from another window (e.g. content). Bobby?
Using the current compartment for the principals should be fine as long as cx->compartment matches script->compartment. Can you add assertSameCompartment(cx, script)? Second: if getOrCreateScript fails, an exception will be thrown which means that a NULL return from JS_GetFunctionScript will have 2 different meanings (the other being "the function is native"). Rather than gunking up this API, I propose we MOZ_ABORT if getOrCreateScript fails.
I included both changes, but I'm not entirely sure if I understand the value of the assertSameCompartment. In the case of lazily cloned scripts, this holds by necessity, doesn't it? If so, shouldn't we rather assert that the given function is in the same compartment as the current context?
Attachment #690245 - Flags: review?(luke)
Attachment #685090 - Attachment is obsolete: true
Comment on attachment 690245 [details] [diff] [review] Create lazy interpreted function's script in the jsdbg API's JS_GetFunctionScript You're right, I meant to ask that you assert the *function* is in the same compartment as cx, as a precondition.
Attachment #690245 - Flags: review?(luke) → review+
Assert that function, not script, is in same compartment as cx
Attachment #690245 - Attachment is obsolete: true
Comment on attachment 690386 [details] [diff] [review] Create lazy interpreted function's script in the jsdbg API's JS_GetFunctionScript. Carrying r=luke
Attachment #690386 - Flags: review+
Comment on attachment 690386 [details] [diff] [review] Create lazy interpreted function's script in the jsdbg API's JS_GetFunctionScript. [Security approval request comment] How easily can the security issue be deduced from the patch? Pretty easily: it's a clear NULL pointer return. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? Which older supported branches are affected by this flaw? None, it was introduced in the current Nightly. If not all supported branches, which bug introduced the flaw? Bug 791850 Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? n/a How likely is this patch to cause regressions; how much testing does it need? It introduces a assertion the pre-flaw code didn't have: that the current JSContext is in the same compartment as the function whose script is queried. I don't know how likely it is that that assert doesn't always hold. It would have been a sec-sensitive issue before this flaw, though.
Attachment #690386 - Flags: sec-approval?
Comment on attachment 690386 [details] [diff] [review] Create lazy interpreted function's script in the jsdbg API's JS_GetFunctionScript. sec-approval+. That said, if the bug only exists in trunk, you don't need to ask for approval to fix it, even if it is a security issue.
Attachment #690386 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/integration/mozilla-inbound/rev/531eb76b7ab6 (In reply to Al Billings [:abillings] from comment #14) > That said, if the bug only exists in trunk, you don't need to ask for > approval to fix it, even if it is a security issue. Ah, I didn't know that. Sorry for the extra work, then.
So it turns out that the function isn't in the same compartment as the context quite oftenly. Stupid of me not to test that on try before pushing, sorry. The only option I see here is to temporarily switch the compartment if we have to initialize the script, which this version does. Try-servering here: http://tbpl.mozilla.org/?tree=Try&rev=7e98b9f06910
Attachment #692286 - Flags: review?(luke)
Attachment #690386 - Attachment is obsolete: true
Comment on attachment 692286 [details] [diff] [review] Create lazy interpreted function's script in the jsdbg API's JS_GetFunctionScript. Whew, lameness. Sorry for not knowing either.
Attachment #692286 - Flags: review?(luke) → review+
Can there be a comment explaining the use of MOZ_CRASH()? It's a rather startling statement to come across in normal API code.
You need to log in before you can comment on or make changes to this bug.