815010 - "Assertion failure: hasScript()" with setTimeout(Array.prototype.indexOf)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

Attachment: testcase (asserts fatally when loaded)

Assertion failure: hasScript(), at js/src/jsfun.h:203

Comment 1

[Jesse Ruderman]

Attachment: stack trace

Comment 2

[Boris Zbarsky [:bzbarsky]]

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.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Oh, and this seems security-sensitive if the assert really means we'll treat random memory as a JSScript*....

Comment 4

[Till Schneidereit [:till]]

Attachment: Initialize lazy script in JS_GetFunctionScript

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 5

[Boris Zbarsky [:bzbarsky]]

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?

Comment 6

[Till Schneidereit [:till]]

(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?

Comment 7

[Boris Zbarsky [:bzbarsky]]

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?

Comment 8

[Luke Wagner [:luke]]

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.

Comment 9

[Till Schneidereit [:till]]

Attachment: Create lazy interpreted function's script in the jsdbg API's JS_GetFunctionScript

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?

Comment 10

[Luke Wagner [:luke]]

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.

Comment 11

[Till Schneidereit [:till]]

Attachment: Create lazy interpreted function's script in the jsdbg API's JS_GetFunctionScript.

Assert that function, not script, is in same compartment as cx

Comment 12

[Till Schneidereit [:till]]

Comment on attachment 690386 [details] [diff] [review]
Create lazy interpreted function's script in the jsdbg API's JS_GetFunctionScript.

Carrying r=luke

Comment 13

[Till Schneidereit [:till]]

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.

Comment 14

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 15

[Till Schneidereit [:till]]

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.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Backed out due to test failures.
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=531eb76b7ab6

https://hg.mozilla.org/integration/mozilla-inbound/rev/ebfa6888f98e
https://hg.mozilla.org/integration/mozilla-inbound/rev/0368b4c03c63

Comment 17

[Till Schneidereit [:till]]

Attachment: Create lazy interpreted function's script in the jsdbg API's JS_GetFunctionScript.

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

Comment 18

[Luke Wagner [:luke]]

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.

Comment 19

[Till Schneidereit [:till]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ff71c0ea0cb2

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ff71c0ea0cb2

Comment 21

[:Benjamin Peterson]

Can there be a comment explaining the use of MOZ_CRASH()? It's a rather startling statement to come across in normal API code.