768313 - Crash with newGlobal, newContext, --dump-bytecode

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

Attachment: stack trace

./js -m -n -a -D

var sandbox = newGlobal("same-compartment");
function inSandbox(code) { evalcx(code, sandbox); };
inSandbox("function six() { return 6; }");
inSandbox("evaluate('function qq() { return six(); }', {newContext: true});");
inSandbox("for (j = 0; j <2 ; ++j) { qq(0); }")

-> Crash in mjit code

The first "bad" revision is:
changeset:   648093316d93
user:        Jason Orendorff
date:        Thu May 17 18:54:30 2012 -0500
summary:     Bug 755808 - Add more options to JS shell evaluate() function. r=jimb.

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Many kudos to Jesse for reducing to this super testcase after I found it and sent a half-reduced one over to him.

Comment 2

[Alex Keybl [:akeybl]]

This doesn't have a security rating, but has been requested for tracking. Does that imply this is an sg:crit?

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Alex Keybl [:akeybl] from comment #2)
> This doesn't have a security rating, but has been requested for tracking.
> Does that imply this is an sg:crit?

This seems likely, but we are not sure and would like double confirmation from the JS devs.

Comment 4

[Jesse Ruderman]

Bug 755808 only changed js.cpp (the shell), so unless this turns out to be a shell-only bug, we can't assume this bug was introduced when bug 755808 landed.

Comment 5

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Let's just assume sec-critical just to be safe - please reduce severity after analysis if this is not the case.

Comment 6

[Jason Orendorff [:jorendorff]]

(In reply to Jesse Ruderman from comment #4)
> Bug 755808 only changed js.cpp (the shell), so unless this turns out to be a
> shell-only bug, we can't assume this bug was introduced when bug 755808
> landed.

Right. I can't tell yet.

That patch added the "evaluate(code, {newContext: true})" option, which didn't exist before. But evaluating code in a new JSContext *is* something that might be possible in the browser -- which is why I added the ability to run such tests in the shell.

Comment 7

[Jason Orendorff [:jorendorff]]

OK, a bit simpler:

function f() { }
evaluate('function g() { f(); }', {newContext: true});
for (var i = 0; i < 2; i++)
    g(0);

I think it happens because JSOPTION_PCCOUNT is set when the function g is executed but not when it is compiled.

Comment 8

[Jason Orendorff [:jorendorff]]

Attachment: v1

It happens because jaeger inlines the non-pccount script into the pccount script. Easy fix.

Comment 9

[Jason Orendorff [:jorendorff]]

Attachment: v2 - same as v1 but how about we include the test

Comment 10

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/c20386dc1219

Helping to land to appease the fuzzbots a little earlier.

Comment 11

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

http://hg.mozilla.org/mozilla-central/rev/c20386dc1219

Comment 12

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Jason, does this affect earlier versions of Firefox? (14 & earlier, including ESR 10)

Also, I think the patch will need to be nominated for aurora landing if it affects 15.

Comment 13

[Jason Orendorff [:jorendorff]]

The bug exists in release, beta, and aurora.  I'm not sure it matters; pinging sfink about this now...

Comment 14

[Jason Orendorff [:jorendorff]]

sfink thinks the only things using pccount are a couple of experimental tools. That's unlikely to change in 15 or earlier.

Based on that, we think it's not necessary to patch this on aurora or earlier branches.

Comment 15

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Thanks, Jason & Steve for the analysis.

-> VERIFIED because a test is in the tree.