Closed Bug 715662 Opened 13 years ago Closed 13 years ago

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

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Version:
Other Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla10
Tracking Flags:
Tracking Status
firefox10 + verified
firefox11 + verified
firefox12 + verified
firefox-esr10 --- unaffected
status1.9.2 --- unaffected

People

(Reporter: jorendorff, Assigned: bhackett1024)

References

(
URL
)

Details

(Keywords: crash, testcase, Whiteboard: [sg:critical])

Attachments

(1 file)

patch
822 bytes, patch
dvander
: review+
akeybl
: approval-mozilla-aurora+
akeybl
: approval-mozilla-beta+
Details | Diff | Splinter Review 
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>
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: general → bhackett1024
Group: core-security
Attached patch patch — — Splinter 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+
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?
This needs a test.
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();
Marking fixed for edmorley.

https://hg.mozilla.org/mozilla-central/rev/6c3cea75e5ba
Status: NEW → RESOLVED
Closed: 13 years ago
status-firefox12: affected → fixed
Resolution: --- → FIXED
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]
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+
Whiteboard: [sg:critical] → [sg:critical][qa+]
Status: RESOLVED → VERIFIED
Whiteboard: [sg:critical][qa+] → [sg:critical]
Group: core-security
Keywords: crash, testcase
Blocks: 716900
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: