Closed Bug 768313 Opened 8 years ago Closed 8 years ago

Crash with newGlobal, newContext, --dump-bytecode

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla16
Tracking Status
firefox13 --- wontfix
firefox14 --- wontfix
firefox15 --- wontfix
firefox16 --- fixed
firefox-esr10 --- wontfix

People

(Reporter: jruderman, Assigned: jorendorff)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: js-triage-needed [advisory-tracking+])

Attachments

(2 files, 1 obsolete file)

Attached file 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.
Many kudos to Jesse for reducing to this super testcase after I found it and sent a half-reduced one over to him.
This doesn't have a security rating, but has been requested for tracking. Does that imply this is an sg:crit?
(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.
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.
Let's just assume sec-critical just to be safe - please reduce severity after analysis if this is not the case.
Keywords: sec-critical
(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.
Assignee: general → 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.
Attached patch v1 (obsolete) — Splinter Review
It happens because jaeger inlines the non-pccount script into the pccount script. Easy fix.
Attachment #639794 - Flags: review?(bhackett1024)
Attachment #639794 - Attachment is obsolete: true
Attachment #639794 - Flags: review?(bhackett1024)
Attachment #639851 - Flags: review?(bhackett1024)
Attachment #639851 - Flags: review?(bhackett1024) → review+
http://hg.mozilla.org/integration/mozilla-inbound/rev/c20386dc1219

Helping to land to appease the fuzzbots a little earlier.
Target Milestone: --- → mozilla16
http://hg.mozilla.org/mozilla-central/rev/c20386dc1219
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
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.
The bug exists in release, beta, and aurora.  I'm not sure it matters; pinging sfink about this now...
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.
Thanks, Jason & Steve for the analysis.

-> VERIFIED because a test is in the tree.
Status: RESOLVED → VERIFIED
Flags: in-testsuite+
Whiteboard: js-triage-needed → js-triage-needed [advisory-tracking+]
Group: core-security
You need to log in before you can comment on or make changes to this bug.