Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

VERIFIED FIXED in Firefox 10

Status

()

Core
JavaScript Engine
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: jorendorff, Assigned: bhackett)

Tracking

({crash, testcase})

Other Branch
mozilla10
crash, testcase
Points:
---

Firefox Tracking Flags

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

Details

(Whiteboard: [sg:critical], URL)

Attachments

(1 attachment)

(Reporter)

Description

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



<!doctype html>
<script>
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 {
            f(arr[j]);
        } catch (exc) {
            console.log("fail");
        }
    }
}
</script>
(Reporter)

Comment 1

6 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 rval.data.debugView
$1 = {
  payload47 = 0, 
  tag = JSVAL_TAG_OBJECT
}
(Assignee)

Updated

6 years ago
Assignee: general → bhackett1024
(Assignee)

Updated

6 years ago
Group: core-security
status-firefox12: --- → affected
tracking-firefox12: --- → +
(Assignee)

Comment 2

6 years ago
Created attachment 586296 [details] [diff] [review]
patch

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+
(Assignee)

Comment 3

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c3cea75e5ba
(Assignee)

Updated

6 years ago
status-firefox10: --- → affected
status-firefox11: --- → affected
tracking-firefox10: --- → ?
tracking-firefox11: --- → ?
(Assignee)

Comment 4

6 years ago
Comment on attachment 586296 [details] [diff] [review]
patch

[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?
(Reporter)

Comment 5

6 years ago
This needs a test.
(Reporter)

Comment 6

6 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;
    }
}

f();

Comment 7

6 years ago
Marking fixed for edmorley.

https://hg.mozilla.org/mozilla-central/rev/6c3cea75e5ba
Status: NEW → RESOLVED
Last Resolved: 6 years ago
status-firefox12: affected → fixed
Resolution: --- → FIXED

Comment 8

6 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:]
(Assignee)

Updated

6 years ago
Whiteboard: [sg:] → [sg:critical]
(Assignee)

Comment 9

6 years ago
(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]
patch

[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+

Updated

6 years ago
tracking-firefox10: ? → +
tracking-firefox11: ? → +
(Assignee)

Comment 11

6 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/c6ef2c476d17
https://hg.mozilla.org/releases/mozilla-beta/rev/a1a53d18a8bd
status-firefox10: affected → fixed
status-firefox11: affected → fixed
Target Milestone: --- → mozilla10
status1.9.2: --- → unaffected
Whiteboard: [sg:critical] → [sg:critical][qa+]

Updated

5 years ago
status-firefox10: fixed → verified

Updated

5 years ago
status-firefox11: fixed → verified

Updated

5 years ago
status-firefox12: fixed → verified

Updated

5 years ago
Status: RESOLVED → VERIFIED

Updated

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

Updated

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

Updated

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