Closed Bug 627486 Opened 9 years ago Closed 9 years ago

Assertion failure: isShape() | Crash [@ js::mjit::stubs::SetName<int>(js::VMFrame&, JSAtom*)]

Categories

(Core :: JavaScript Engine, defect, critical)

x86
Windows 7
defect
Not set
critical

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: bc, Assigned: dvander)

References

(Blocks 1 open bug, )

Details

(4 keywords, Whiteboard: [sg:critical?][hardblocker][has patch][fixed-in-tracemonkey])

Crash Data

Attachments

(3 files)

1. http://www.hlcsb.de/SL/topwerte.html?url=www.solarlog-home2.de/pesoelko
2. Assertion failure: isShape(), at c:\work\mozilla\builds\2.0.0\mozilla\js\src\jspropertycache.h:112


Appears to be Windows 7 only at the moment. I don't have Windows 7, but will try to reproduce on Vista.

See also bug 607502

Operating system: Windows NT
                  6.1.7600 
CPU: x86
     GenuineIntel family 6 model 44 stepping 2
     1 CPU

Crash reason:  EXCEPTION_ACCESS_VIOLATION_WRITE
Crash address: 0x0

Thread 0 (crashed)
 0  mozjs.dll!JS_Assert [jsutil.cpp : 73 + 0x0]
    eip = 0x68a41dca   esp = 0x002cb7cc   ebp = 0x002cb7cc   ebx = 0x05650198
    esi = 0x00000001   edi = 0x000068ab   eax = 0x00000000   ecx = 0x9e9a706a
    edx = 0x6eff1d48   efl = 0x00010206
    Found by: given as instruction pointer in context
 1  mozjs.dll!js::PCVal::toShape() [jspropertycache.h : 112 + 0x21]
    eip = 0x689769e9   esp = 0x002cb7d4   ebp = 0x002cb7e4
    Found by: call frame info
 2  mozjs.dll!js::mjit::stubs::SetName<0>(js::VMFrame &,JSAtom *) [StubCalls.cpp : 165 + 0xa]
    eip = 0x68af712a   esp = 0x002cb7ec   ebp = 0x002cb834
    Found by: call frame info
 3  mozjs.dll!js::mjit::stubs::SetGlobalName<0>(js::VMFrame &,JSAtom *) [StubCalls.cpp : 316 + 0xa]
    eip = 0x68af7b37   esp = 0x002cb83c   ebp = 0x002cb844
    Found by: call frame info
 4  mozjs.dll!js::mjit::ic::SetGlobalName(js::VMFrame &,js::mjit::ic::MICInfo *) [MonoIC.cpp : 188 + 0x26]
    eip = 0x68b45a76   esp = 0x002cb84c   ebp = 0x002cb880
    Found by: call frame info
 5  mozjs.dll!js::mjit::EnterMethodJIT(JSContext *,JSStackFrame *,void *,js::Value *) [MethodJIT.cpp : 748 + 0x14]
    eip = 0x68aed90a   esp = 0x002cb8c8   ebp = 0x002cb8c0
    Found by: call frame info with scanning
blocking2.0: --- → ?
bp-40b67d42-9273-448e-8a8c-628822110120
bp-3a3a3e17-52f0-475f-abcb-ee5bf2110120

the opt crash is reproducible on winxp.

0 	mozjs.dll 	js::mjit::stubs::SetName<0> 	js/src/methodjit/StubCalls.cpp:191
1 	mozjs.dll 	js::mjit::ic::SetGlobalName 	js/src/methodjit/MonoIC.cpp:188
2 	mozjs.dll 	js::mjit::EnterMethodJIT 	js/src/methodjit/MethodJIT.cpp:748
3 	mozjs.dll 	CheckStackAndEnterMethodJIT 	js/src/methodjit/MethodJIT.cpp:774
4 	mozjs.dll 	js::mjit::JaegerShot 	js/src/methodjit/MethodJIT.cpp:791
5 	mozjs.dll 	js::RunScript 	js/src/jsinterp.cpp:654
6 	mozjs.dll 	js::Execute 	js/src/jsinterp.cpp:1023
7 	mozjs.dll 	JS_EvaluateUCScriptForPrincipals 	js/src/jsapi.cpp:4930
8 	mozjs.dll 	JS_EvaluateUCScriptForPrincipalsVersion 	js/src/jsapi.cpp:4906
9 	xul.dll 	nsJSContext::EvaluateString 	dom/base/nsJSEnvironment.cpp:1551
Keywords: crash
Summary: Assertion failure: isShape() → Assertion failure: isShape() | Crash [@ js::mjit::stubs::SetName<int>(js::VMFrame&, JSAtom*)]
Smells exploitable. Lets hide it until we know more.
Group: core-security
I can reproduce this, investigating.
I can't reproduce this on Linux (either x86 or x64) though, so it looks like I'll have to debug on Windows.
I can only reproduce this sporadically, and the second time I was able to get it, my computer BSOD'd. But I think I know what's going on here, obj->branded() is true and this is an edge case of SetGlobalName I'd hoped wouldn't come up. This is probably the same bug:

// vim: set ts=4 sw=4 tw=99 et:
g = undefined;
function L() { }

function h() {
    with (h) { }
    for (var i = 0; i < 10; i++)
        g();
}

function f(x) {
    g = x;
}

f(L);
h();
f(L);
f(2);
h();

bash-3.2$ ./Debug32/js -m k.js 
Assertion failure: isObject(), at ../jsvalue.h:602
Bus error
Assignee: general → danderson
Status: NEW → ASSIGNED
blocking2.0: ? → betaN+
Whiteboard: hardblocker
Attached patch fixSplinter Review
When obj->branded() is true, you're not supposed to change any method value on |obj| without changing the shape. SETGNAME bypasses this, which is wrong.

This patch lets JSOP_SETGNAME ICs gain a single extra stub. It also cleans up some historic junk inside the MIC structures. The stub is the same as the inline fast path, except there's an extra extra set of guards to make sure we're not overwriting a function-valued slot.
Attachment #506623 - Flags: review?(cdleary)
Verified that the site no longer crashes with this patch.

Chris, there's one bug I fixed locally: the verifyRange check has an extra, erroneous decref.
Whiteboard: hardblocker → [sg:critical?]hardblocker
Comment on attachment 506623 [details] [diff] [review]
fix

- Test case makes sense.
- Shimmy does all of the constant {type/data} prop that we were manually doing inline, right? I would think so... that's a nice feature for the FrameState to have!
- Nice member renames.
- Attach/Update reads well, nice catch on the verify release... makes sense to leave that EP sitting around doing nothing and not error out of the IC. A comment about that might be nice, but it wasn't too unintuitive.

+            /*
+             * If we're going to rebrand, the object may unbrand, allowing this
+             * IC to come back to life. In that case, we don't disable the IC.
+             */

I may be wrong, but I think this should be "If we're going to rebrand, it's more likely that the object may later unbrand." Is there some reason failing to rebrand at the time of the IC update implies never unbranding? Clarifying that would be awesome.

+    if (status == Lookup_Error)
+        THROWV();

I thought THROWV required a value argument.

Sorry that the time-to-review was so high!
Attachment #506623 - Flags: review?(cdleary) → review+
Whiteboard: [sg:critical?]hardblocker → [sg:critical?][hardblocker][has patch]
Chris cautioned me in person that this would probably bloat the MIC structures, and it does; x86 went from 24 -> 56 bytes, and x64 went from 48 -> 88 bytes. I'll attach a follow-up patch in a moment so njn doesn't go into conniptions from me undoing his work :)
This splits MICInfo into Get and Set variants. On x86 they are 12 and 48 bytes, respectively. On x64 they are 24 and 72 bytes, respectively. We expect that on average there are more gets than sets.
Attachment #508479 - Flags: review?(cdleary)
Comment on attachment 508479 [details] [diff] [review]
avoid memory bloat

Nice cleanup. Only comment is that you don't need to start the reserve IC space until the start of the actual fast path here:

-    MICGenInfo mic(ic::MICInfo::SET);
-    frame.pinEntry(fe, mic.vr);
-
+    SetGlobalNameICInfo ic;
     RESERVE_IC_SPACE(masm);
+
+    frame.pinEntry(fe, ic.vr);

Doesn't matter much, though.
Attachment #508479 - Flags: review?(cdleary) → review+
http://hg.mozilla.org/tracemonkey/rev/54a515a97151
http://hg.mozilla.org/tracemonkey/rev/aa1cf1121a20
Whiteboard: [sg:critical?][hardblocker][has patch] → [sg:critical?][hardblocker][has patch][fixed-in-tracemonkey]
Backed out, may be responsible for a new webgl test failure.
Whiteboard: [sg:critical?][hardblocker][has patch][fixed-in-tracemonkey] → [sg:critical?][hardblocker][has patch]
Whiteboard: [sg:critical?][hardblocker][has patch] → [sg:critical?][hardblocker]
(In reply to comment #10)
> 
> This splits MICInfo into Get and Set variants. On x86 they are 12 and 48 bytes,
> respectively. On x64 they are 24 and 72 bytes, respectively. We expect that on
> average there are more gets than sets.

Bug 615199 comment 91 shows that MICInfo space usage isn't that high, so this'll be a marginal win, esp. as it increases the size of JITScript (though bug 630445 will mitigate that).  Measurements are always good.
There was a real bug, here:

> +    /* Restore shapeReg to obj->slots, since we clobbered it. */
> +    if (ic->objConst) {
> +        masm.move(ImmPtr(obj), ic->objReg);
> +        masm.loadPtr(Address(ic->objReg, offsetof(JSObject, slots)), ic->shapeReg);
> +    }

The second load should have been outside the |if|, since ic->objReg is always clobbered. This was pretty hard to track down because it caused silent memory corruption, and the GC would die randomly much later. Also, it only happened when ic->objConst was false. I wrongly assumed this would only happen in chrome code which we don't JIT, but in fact this kind of expression:

x = a ? b : c

Will have a branch in between binding "x" and emitting the store, so the compiler loses constant propagation. That turns out to be really rare.

I'll re-land this tonight.
Whiteboard: [sg:critical?][hardblocker] → [sg:critical?][hardblocker][has patch]
Severity: normal → critical
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/54a515a97151
http://hg.mozilla.org/mozilla-central/rev/aa1cf1121a20
Note: not marking as fixed because fixed-in-tracemonkey is not present on the whiteboard.
re-landed, stuck:
http://hg.mozilla.org/tracemonkey/rev/1121f56087a8
http://hg.mozilla.org/tracemonkey/rev/410fe81fff16
Whiteboard: [sg:critical?][hardblocker][has patch] → [sg:critical?][hardblocker][has patch][fixed-in-tracemonkey]
Apparently this broke mobile talos.
Not sure how useful this is, since talos is a black box, but it broke tsvg, tp4, tsspider and tdhtml, but not twinopen, ts, tpan or tgfx.
Comment on attachment 511551 [details] [diff] [review]
part 3: fix ARM

Looks great. Nice catch on the WithAddressOffsetPatches.
Attachment #511551 - Flags: review?(cdleary) → review+
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
I just got this in the automation but so far haven't been able to reproduce:

1. http://www.superconsole.it/
2. assert

Operating system: Windows NT
                  6.1.7600 
CPU: x86
     GenuineIntel family 6 model 44 stepping 2
     1 CPU

Crash reason:  EXCEPTION_ACCESS_VIOLATION_WRITE
Crash address: 0x0

Thread 0 (crashed)
 0  mozjs.dll!JS_Assert [jsutil.cpp : 73 + 0x0]
    eip = 0x6959811a   esp = 0x001cc238   ebp = 0x001cc238   ebx = 0x03540030
    esi = 0x00007018   edi = 0x0786c760   eax = 0x00000000   ecx = 0xae4858c4
    edx = 0x6f161d48   efl = 0x00010202
    Found by: given as instruction pointer in context
 1  mozjs.dll!js::PCVal::toShape() [jspropertycache.h : 112 + 0x21]
    eip = 0x694cce59   esp = 0x001cc240   ebp = 0x001cc250
    Found by: call frame info
 2  mozjs.dll!js::mjit::stubs::SetName<0>(js::VMFrame &,JSAtom *) [StubCalls.cpp : 165 + 0xa]
    eip = 0x6964de1a   esp = 0x001cc258   ebp = 0x001cc2a0
    Found by: call frame info
 3  mozjs.dll!js::mjit::stubs::SetGlobalName<0>(js::VMFrame &,JSAtom *) [StubCalls.cpp : 316 + 0xa]
    eip = 0x6964e827   esp = 0x001cc2a8   ebp = 0x001cc2b0
    Found by: call frame info
 4  mozjs.dll!js::mjit::ic::SetGlobalName(js::VMFrame &,js::mjit::ic::SetGlobalNameIC *) [MonoIC.cpp : 330 + 0x26]
    eip = 0x6969d564   esp = 0x001cc2b8   ebp = 0x001cc2d8
    Found by: call frame info
 5  mozjs.dll!js::mjit::EnterMethodJIT(JSContext *,JSStackFrame *,void *,js::Value *) [MethodJIT.cpp : 748 + 0x14]
    eip = 0x6964428a   esp = 0x001cc320   ebp = 0x001cc318
    Found by: call frame info with scanning

I'll keep looking but thought I would alert you first in case you can see something.
Ok. I can reproduce with Windows XP and a build from this morning and have locally saved version that will assert after a few reloads. I'll work on a reduced test. Do we want a new bug?
Yeah, new bug would be good, this one is fixed. Thanks!
filed Bug 634689
Crash Signature: [@ js::mjit::stubs::SetName<int>(js::VMFrame&, JSAtom*)]
Group: core-security
You need to log in before you can comment on or make changes to this bug.