The default bug view has changed. See this FAQ.

browser-only methodjit crash (triggered by CSS transform tests)

VERIFIED FIXED in Firefox 10



JavaScript Engine
5 years ago
4 years ago


(Reporter: jorendorff, Assigned: bhackett)


({crash, testcase})

Other Branch
crash, testcase

Firefox Tracking Flags

(firefox10+ verified, firefox11+ verified, firefox12+ verified, firefox-esr10 unaffected, status1.9.2 unaffected)


(Whiteboard: [sg:critical], URL)


(1 attachment)



5 years ago
Found by Aryeh Gregor, see URL. I can't get it to happen in the shell.

<!doctype html>
var p = [[0]];
function f(y) {
    var a, b, c, d;
    for (var k = 0; k < 2; k++) {
	var z = 0 + p[0][0] - y;
	if (a === undefined || z < a)
	    a = z;
	if (c === undefined || z > c)
	    c = z;
    throw 0;

var arr = [1, 1, 1, 1, 1, 1, 1, 0.5];
for (var i = 0; i < 5; i++) {
    for (var j = 0; j < 8; j++) {
        try {
        } catch (exc) {

Comment 1

5 years ago
It crashes computing `z > c`.

c should be undefined, but the value on the stack is
  ObjectValue(*(JSObject *) NULL).

#0  0x0000000103f3701c in js::HeapPtr<js::Shape, unsigned long>::operator js::Shape* (this=0x0) at Barrier.h:231
#1  0x0000000103fa5d48 in JSObject::lastProperty (this=0x0) at jsobj.h:515
#2  0x0000000104005378 in JSObject::getClass (this=0x0) at jsscope.h:1116
#3  0x0000000103de984b in JSObject::defaultValue (this=0x0, cx=0x113faaf10, hint=JSTYPE_NUMBER, vp=0x10ae2e150) at jsobjinlines.h:125
#4  0x00000001040d507d in ToPrimitive (cx=0x113faaf10, preferredType=JSTYPE_NUMBER, vp=0x10ae2e150) at jsobjinlines.h:1443
#5  0x00000001040e10e5 in js::mjit::stubs::GreaterThan (f=@0x7fff5fbf6fc0) at StubCalls.cpp:808
#6  0x0000000117c93a53 in ?? ()
#7  0x00000001040d0f6f in js::mjit::EnterMethodJIT (cx=0x113faaf10, fp=0x10ae2e030, code=0x117c94c17, stackLimit=0x10b20e000, partial=true) at MethodJIT.cpp:1051
#8  0x00000001040d13b5 in CheckStackAndEnterMethodJIT (cx=0x113faaf10, fp=0x10ae2e030, code=0x117c94c17, partial=true) at MethodJIT.cpp:1109
#9  0x00000001040d1424 in js::mjit::JaegerShotAtSafePoint (cx=0x113faaf10, safePoint=0x117c94c17, partial=true) at MethodJIT.cpp:1127
#10 0x0000000103e841f4 in js::Interpret (cx=0x113faaf10, entryFrame=0x10ae2e030, interpMode=js::JSINTERP_NORMAL) at jsinterp.cpp:1847
#11 0x0000000103ea5811 in js::RunScript (cx=0x113faaf10, script=0x10b40f350, fp=0x10ae2e030) at jsinterp.cpp:478
#12 0x0000000103ea5c1d in js::ExecuteKernel (cx=0x113faaf10, script=0x10b40f350, scopeChain=@0x10b411060, thisv=@0x7fff5fbfae30, type=js::EXECUTE_GLOBAL, evalInFrame=0x0, result=0x0) at jsinterp.cpp:714
#13 0x0000000103ea6075 in js::Execute (cx=0x113faaf10, script=0x10b40f350, scopeChainArg=@0x10b411060, rval=0x0) at jsinterp.cpp:755
#14 0x0000000103d5f078 in EvaluateUCScriptForPrincipalsCommon (cx=0x113faaf10, obj=0x10b411060, principals=0x10d707118, originPrincipals=0x0, chars=0x114324408, length=434, filename=0x11427b158 "file:///Users/jorendorff/dev/gecko-crash/gecko-crash.html", lineno=2, rval=0x0, compileVersion=JSVERSION_DEFAULT) at jsapi.cpp:5321

(gdb) p
$1 = {
  payload47 = 0, 
Assignee: general → bhackett1024
Group: core-security
status-firefox12: --- → affected
tracking-firefox12: --- → +
Created attachment 586296 [details] [diff] [review]

At control flow join points, if a variable is known to be a double and is being held in a FP register then it isn't required to be synced to memory at the join.  Its type tag, however, was being marked as synced, and in the example after some later copies between the variable z and a (whose type is unknown) we forgot that z was a double but still treated its type as synced, causing later codegen to be incorrect.  The fix marks both the type and payload as unsynced.
Attachment #586296 - Flags: review?(dvander)
Attachment #586296 - Flags: review?(dvander) → review+
status-firefox10: --- → affected
status-firefox11: --- → affected
tracking-firefox10: --- → ?
tracking-firefox11: --- → ?
Comment on attachment 586296 [details] [diff] [review]

[Approval Request Comment]
User impact if declined: Potential crash in jitcode on a torn value (security risk).
Risk to taking this patch (and alternatives if risky): Should be low risk, affects code generation in a corner case, but should bake in nightlies for a few days at least.
Attachment #586296 - Flags: approval-mozilla-beta?
Attachment #586296 - Flags: approval-mozilla-aurora?

Comment 5

5 years ago
This needs a test.

Comment 6

5 years ago
OK, here's the first half of a shell test. If you run this with -m -n -a, it triggers the exact situation being patched here: fe->data.unsync() executes, and reg.isReg() is false.

However that is not enough, because the test case then passes. Is there an assertion we could add to a later stage of the JIT that would detect this class of bug? If not, how can I make it cause some observable runtime effect?

function f() {
    var a = [1, 0.5, 1];
    var b;
    for (var i = 0; i < a.length; i++) {
	var z = -a[i];
	if (b === undefined || z < b)
	    b = z;


Comment 7

5 years ago
Marking fixed for edmorley.
Last Resolved: 5 years ago
status-firefox12: affected → fixed
Resolution: --- → FIXED

Comment 8

5 years ago
Looking for a security rating prior to deciding on whether or not to take based upon that analysis. If not, there's no need to track until it's verified that this is a significant crasher or recent regression.
Whiteboard: [sg:]
Whiteboard: [sg:] → [sg:critical]
(In reply to Jason Orendorff [:jorendorff] from comment #6)
> However that is not enough, because the test case then passes. Is there an
> assertion we could add to a later stage of the JIT that would detect this
> class of bug? If not, how can I make it cause some observable runtime effect?

There really isn't a way for the JIT to assert the correctness of its information here, as the basic problem is that the JIT has incorrect information about which parts of the frame are coherent in the generated machine code, and that correctness is not a property that can be asserted.
Comment on attachment 586296 [details] [diff] [review]

[Triage Comment]
Approving for aurora and beta since this is externally found, sg-crit and deemed low-risk.
Attachment #586296 - Flags: approval-mozilla-beta?
Attachment #586296 - Flags: approval-mozilla-beta+
Attachment #586296 - Flags: approval-mozilla-aurora?
Attachment #586296 - Flags: approval-mozilla-aurora+


5 years ago
tracking-firefox10: ? → +
tracking-firefox11: ? → +
status-firefox10: affected → fixed
status-firefox11: affected → fixed
Target Milestone: --- → mozilla10
status1.9.2: --- → unaffected
Whiteboard: [sg:critical] → [sg:critical][qa+]


5 years ago
status-firefox10: fixed → verified


5 years ago
status-firefox11: fixed → verified


5 years ago
status-firefox12: fixed → verified


5 years ago


5 years ago
Whiteboard: [sg:critical][qa+] → [sg:critical]


5 years ago
status-firefox-esr10: --- → unaffected
Group: core-security
Keywords: crash, testcase


4 years ago
Blocks: 716900
You need to log in before you can comment on or make changes to this bug.