Closed Bug 830042 Opened 7 years ago Closed 7 years ago

IonMonkey: Assertion failure: v->toGCThing(), at gc/Marking.cpp:488 or Opt-crash [@ js::gc::MarkKind]

Categories

(Core :: JavaScript Engine, defect, critical)

x86
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla21
Tracking Status
firefox19 --- unaffected
firefox20 --- verified
firefox21 --- verified
firefox-esr17 --- unaffected
b2g18 --- unaffected

People

(Reporter: decoder, Assigned: bhackett)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [jsbugmon:])

Crash Data

Attachments

(1 file)

The following testcase asserts on mozilla-central revision 1761f4a9081c (run with --ion-eager):


function f() {
    var o1 = { a: 5 };
    for (var i = 0; i < 6; - i) {
	var o = i % 2 ? o2 : o1;
	("").a = i;
    }
}
f()
Debug backtrace:

Program received signal SIGSEGV, Segmentation fault.
0x083dd740 in MarkValueInternal (trc=0x894aefc, v=0xf76970c0) at /srv/repos/mozilla-central/js/src/gc/Marking.cpp:488
488             JS_ASSERT(v->toGCThing());
(gdb) bt
#0  0x083dd740 in MarkValueInternal (trc=0x894aefc, v=0xf76970c0) at /srv/repos/mozilla-central/js/src/gc/Marking.cpp:488
#1  0x083ddb19 in js::gc::MarkValueRootRange (trc=0x894aefc, len=3, vec=0xf76970b0, name=0x86d7df5 "vm_stack") at /srv/repos/mozilla-central/js/src/gc/Marking.cpp:548
#2  0x083325a5 in js::gc::MarkValueRootRange (trc=0x894aefc, begin=0xf76970b0, end=0xf76970c8, name=0x86d7df5 "vm_stack") at ../gc/Marking.h:175
#3  0x08334959 in js::StackSpace::markFrame (this=0x894ad84, trc=0x894aefc, fp=0xf7697070, slotsEnd=0xf76970c8) at /srv/repos/mozilla-central/js/src/vm/Stack.cpp:646
#4  0x083349c6 in js::StackSpace::mark (this=0x894ad84, trc=0x894aefc) at /srv/repos/mozilla-central/js/src/vm/Stack.cpp:668
#5  0x083d8d54 in js::gc::MarkRuntime (trc=0x894aefc, useSavedRoots=false) at /srv/repos/mozilla-central/js/src/gc/RootMarking.cpp:771
#6  0x08129902 in BeginMarkPhase (rt=0x894ad30) at /srv/repos/mozilla-central/js/src/jsgc.cpp:2634
#7  0x0812d895 in IncrementalCollectSlice (rt=0x894ad30, budget=0, reason=js::gcreason::TOO_MUCH_MALLOC, gckind=js::GC_NORMAL) at /srv/repos/mozilla-central/js/src/jsgc.cpp:3973
#8  0x0812dfe2 in GCCycle (rt=0x894ad30, incremental=true, budget=0, gckind=js::GC_NORMAL, reason=js::gcreason::TOO_MUCH_MALLOC) at /srv/repos/mozilla-central/js/src/jsgc.cpp:4160
#9  0x0812e413 in Collect (rt=0x894ad30, incremental=true, budget=0, gckind=js::GC_NORMAL, reason=js::gcreason::TOO_MUCH_MALLOC) at /srv/repos/mozilla-central/js/src/jsgc.cpp:4278
#10 0x0812e5fb in js::GCSlice (rt=0x894ad30, gckind=js::GC_NORMAL, reason=js::gcreason::TOO_MUCH_MALLOC, millis=0) at /srv/repos/mozilla-central/js/src/jsgc.cpp:4316
#11 0x080dd181 in js_InvokeOperationCallback (cx=0x8971e20) at /srv/repos/mozilla-central/js/src/jscntxt.cpp:1058
#12 0x080dd1ea in js_HandleExecutionInterrupt (cx=0x8971e20) at /srv/repos/mozilla-central/js/src/jscntxt.cpp:1082
#13 0x0862d9c5 in js::mjit::stubs::Interrupt (f=..., pc=0x899e247 "mV") at /srv/repos/mozilla-central/js/src/methodjit/StubCalls.cpp:801
#14 0xf7fcc334 in ?? ()
#15 0x08912ff4 in ?? ()


S-s due to GC involvement.
Blocks: IonFuzz
Crash Signature: [@ js::gc::MarkKind]
Keywords: crash
Whiteboard: [jsbugmon:update,bisect]
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   115546:8275b86c0b62
user:        Brian Hackett
date:        Mon Dec 10 12:02:31 2012 -0700
summary:     Remove bytecode uses analysis, keep track of SSA values that were folded away when building MIR, bug 818869. r=jandem

This iteration took 133.228 seconds to run.
Needinfo from bhackett for comment 2 :)
Flags: needinfo?(bhackett1024)
The value in question isMarkable() but is apparently not a gc thing? That seems a little odd.
Attached patch patchSplinter Review
This is an interaction between Ion and JM.  When JM does OSR it does not check the types of the values on the interpreter stack, and assumes they match whatever was inferred.  When those values are dead, however, Ion can write an undefined value to the stack instead of the original, and JM can end up treating that undefined value's null payload as an object/string and write torn values to the stack.  Since the value is dead those torn values will only be observed during GC, and since the original value is undefined this can only manifest in a null deref.

I think it's best to fix this by just being tolerant of these values when marking the VM stack.  Trying to reason within Ion about what JM may or may not do is difficult and could hurt codegen quality, and fixing this during marking is not hard and can be easily removed once JM is gone.
Assignee: general → bhackett1024
Attachment #702982 - Flags: review?(wmccloskey)
Flags: needinfo?(bhackett1024)
NULL deref, not s-s.
Group: core-security
Comment on attachment 702982 [details] [diff] [review]
patch

Review of attachment 702982 [details] [diff] [review]:
-----------------------------------------------------------------

Yuck. Oh well.

::: js/src/gc/Marking.cpp
@@ +502,5 @@
> +MarkValueInternalMaybeNullPayload(JSTracer *trc, Value *v)
> +{
> +    if (v->isMarkable()) {
> +        void *thing = v->toGCThing();
> +        if (thing) {

If you don't call a mark function, I think you're supposed to call JS_UNSET_TRACING_LOCATION.
Attachment #702982 - Flags: review?(wmccloskey) → review+
Comment on attachment 702982 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 818869
User impact if declined: potential null deref
Testing completed (on m-c, etc.): on m-i
Risk to taking this patch (and alternatives if risky): none
Attachment #702982 - Flags: approval-mozilla-aurora?
Whiteboard: [jsbugmon:update] → [jsbugmon:]
JSBugMon: Cannot process bug: Unknown exception (check manually)
https://hg.mozilla.org/mozilla-central/rev/0f87c11f27b0
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Comment on attachment 702982 [details] [diff] [review]
patch

low risk null deref, approved for aurora.
Attachment #702982 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
I've got this in my queue for Aurora landing once it reopens. Clearing checkin-needed so this doesn't show up in my queries.
Keywords: checkin-needed
Verified the fix with IonMonkey from latest Firefox 20.0 beta 2: no assertion
No assertion on FF 21 2013-05-07-mozilla-beta-debug build. Verified fixed
You need to log in before you can comment on or make changes to this bug.