Closed Bug 581784 Opened 13 years ago Closed 13 years ago

TM: Crash [@ JSObject::getClass]


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
blocking2.0 --- betaN+
status2.0 --- wanted
blocking1.9.2 --- .9+
status1.9.2 --- .9-fixed
blocking1.9.1 --- .12+
status1.9.1 --- .12-fixed


(Reporter: gkw, Assigned: luke)


(5 keywords, Whiteboard: [sg:critical?][ccbr] fixed-in-tracemonkey)

Crash Data


(1 file)

for (x = 0; x < 4; x++) {
    (z = Function)()

crashes js debug shell with -j on TM tip at JSObject::getClass

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_PROTECTION_FAILURE at address: 0x00000004
0x0000f211 in JSObject::getClass (this=0x0) at jsobj.h:306
306             return clasp;
(gdb) bt
#0  0x0000f211 in JSObject::getClass (this=0x0) at jsobj.h:306
#1  0x0000f2ea in JSObject::isFunction (this=0x0) at jsfun.h:253
#2  0x0018027c in IsFunctionObject (v=@0x10000f8) at jsfun.h:277
#3  0x001849e5 in js::NativeToValue (cx=0x809400, v=@0x10000f8, type=JSVAL_TYPE_NONFUNOBJ, slot=0x84ec08) at ../jstracer.cpp:2825
#4  0x001c14ad in js::FlushNativeStackFrameVisitor::visitStackSlots (this=0xbfffdfa8, vp=0x10000f8, count=2, fp=0x10000a0) at ../jstracer.cpp:3003
#5  0x00185196 in js::VisitFrameSlots<js::FlushNativeStackFrameVisitor> (visitor=@0xbfffdfa8, cx=0x809400, depth=0, i=@0xbfffdf50, up=0x0) at ../jstracer.cpp:1787
#6  0x0018604e in js::VisitStackSlots<js::FlushNativeStackFrameVisitor> (visitor=@0xbfffdfa8, cx=0x809400, callDepth=0) at ../jstracer.cpp:1809
#7  0x001860bd in js::FlushNativeStackFrame (cx=0x809400, callDepth=0, mp=0x86cfc8, np=0x84ec00, stopFrame=0x0, ignoreSlots=0) at ../jstracer.cpp:3372
#8  0x0018d273 in js::LeaveTree (tm=0x8375d4, state=@0xbfffe458, lr=0x86cf74) at ../jstracer.cpp:7025
#9  0x0018d4fb in js::DeepBail (cx=0x809400) at ../jstracer.cpp:7952
#10 0x0007101c in js::LeaveTrace (cx=0x809400) at jscntxt.h:2959
#11 0x00071250 in js_GetTopStackFrame (cx=0x809400) at jscntxt.h:2995
#12 0x0007294e in Function (cx=0x809400, obj=0x606910, argc=0, argv=0xbfffe3b8, rval=0xbfffe3c0) at ../jsfun.cpp:2128
#13 0x003eff45 in ?? ()
#14 0x001816f0 in js::ExecuteTrace (cx=0x809400, f=0x864244, state=@0xbfffe458) at ../jstracer.cpp:6598
#15 0x0018e0db in js::ExecuteTree (cx=0x809400, f=0x864244, inlineCallCount=@0xbfffecf0, innermostNestedGuardp=0xbfffe534, lrp=0xbfffe538) at ../jstracer.cpp:6699
#16 0x0019fd22 in js::MonitorLoopEdge (cx=0x809400, inlineCallCount=@0xbfffecf0, reason=js::Record_Branch) at ../jstracer.cpp:7186
#17 0x00095bb9 in js::Interpret (cx=0x809400) at ../jsinterp.cpp:3386
#18 0x000b3588 in js::Execute (cx=0x809400, chain=0x602000, script=0x40c140, down=0x0, flags=0, result=0xbffff680) at jsinterp.cpp:904
#19 0x00017cae in JS_ExecuteScript (cx=0x809400, obj=0x602000, script=0x40c140, rval=0xbffff680) at ../jsapi.cpp:4662
#20 0x0000cc36 in Process (cx=0x809400, obj=0x602000, filename=0x0, forceTTY=0) at ../../shell/js.cpp:532
#21 0x0000d60d in ProcessArgs (cx=0x809400, obj=0x602000, argv=0xbffff84c, argc=1) at ../../shell/js.cpp:853
#22 0x0000d726 in shell (cx=0x809400, argc=1, argv=0xbffff84c, envp=0xbffff854) at ../../shell/js.cpp:4949
#23 0x0000d84a in main (argc=1, argv=0xbffff84c, envp=0xbffff854) at ../../shell/js.cpp:5036
(gdb) x/i $eip
0xf211 <_ZNK8JSObject8getClassEv+9>:    mov    0x4(%eax),%eax
(gdb) x/b $eax 
0x0:    Cannot access memory at address 0x0
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   47991:faf55b60e857
user:        Luke Wagner
date:        Tue Jul 20 19:32:04 2010 -0700
summary:     Bug 580187 - Pass vp to ComputeThisFromVp (r=dvander)
Blocks: 580187
Attached patch fixSplinter Review
It looks like this is a pre-existing bug caught be fatval's added assertions.  Bug 580187 just fixed a fatval bug that let fatval assert properly.

The problem is that TraceRecorder::callNative builds the correct this_ins value to pass to the slow native, but does not set this_ins in the tracker for vp[1].  Thus, on deep bail, the value of vp[1] is what it was before computing this, which seems to be NULL, hence the assertion when the snapshot says its supposed to be JSVAL_TYPE_OBJECT.  Modifying a pre-fatval revision to JS_ASSERT(!JSVAL_IS_NULL(v)) in NativeToValue for TT_OBJECT also fails on the test in comment 0.

The fact that this only asserts on OS X means that the value of vp[1] that is produced by LeaveTree isn't deterministic, which makes sense since its just the last LIns* for vp[1] hanging in the tracker.  So, I think that makes this a security bug.

The patch is just the obvious set(&vp[1], this_ins).  It seems like the right place to put it, but I'm not quite so familiar with the subtleties of deep-bailing, so please check me on its placement.
Attachment #460167 - Flags: review?(gal)
Attachment #460167 - Flags: review?(dvander)
No longer blocks: 580187
Attachment #460167 - Flags: review?(gal) → review+
blocking1.9.1: --- → ?
blocking1.9.2: --- → ?
Attachment #460167 - Flags: review?(dvander) → review+
blocking1.9.1: ? → .12+
blocking1.9.2: ? → .9+
status2.0: --- → wanted
Whiteboard: [ccbr] → [sg:critical?][ccbr]
Whiteboard: [sg:critical?][ccbr] → [sg:critical?][ccbr] fixed-in-tracemonkey
Whiteboard: [sg:critical?][ccbr] fixed-in-tracemonkey → [sg:critical?][ccbr] fixed-in-tracemonkey [critsmash:patch]
Assignee: general → lw
Whiteboard: [sg:critical?][ccbr] fixed-in-tracemonkey [critsmash:patch] → [sg:critical?][ccbr] fixed-in-tracemonkey
Branch patch?
Closed: 13 years ago
Resolution: --- → FIXED
blocking2.0: ? → betaN+
Gary, can you verify this fix on debug 1.9.1 and 1.9.2 as reporter?
(In reply to comment #7)
> Gary, can you verify this fix on debug 1.9.1 and 1.9.2 as reporter?

Sure, verified. :)

$ jsfunfuzz-dbg-32-191-27038-e356f6a54a72/js-dbg-32-191-darwin -j
js> for (x = 0; x < 4; x++) {
    (z = Function)()

function anonymous() {
js> js> 

$ jsfunfuzz-dbg-32-192-34511-c3119c586090/js-dbg-32-192-darwin -j
js> for (x = 0; x < 4; x++) {
    (z = Function)()
(function anonymous() {})
Well, unless you verified it on trunk as well, we should leave it resolved. I'll add the verified1.9.1 and verified1.9.2 keywords though.

Thank you.
Closed: 13 years ago13 years ago
(In reply to comment #9)
> Well, unless you verified it on trunk as well, we should leave it resolved.
> I'll add the verified1.9.1 and verified1.9.2 keywords though.

D'oh, sorry about that. Verified on trunk too. :)

$ jsfunfuzz-dbg-32-mc-49374-cbf493ff91ae/js-dbg-32-mc-darwin -j 
js> for (x = 0; x < 4; x++) {
    (z = Function)()
(function anonymous() {})
Group: core-security
Crash Signature: [@ JSObject::getClass]
Bug in removed tracer code, setting in-testsuite- flag.
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.