Closed Bug 886287 Opened 8 years ago Closed 8 years ago

OdinMonkey: Assertion failure: begin <= end, at gc/RootMarking.cpp:264


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
firefox23 --- unaffected
firefox24 --- fixed
firefox25 --- fixed
firefox-esr17 --- unaffected
b2g18 --- unaffected


(Reporter: decoder, Assigned: h4writer)



(4 keywords, Whiteboard: [jsbugmon:update,bisect])


(2 files, 1 obsolete file)

The following testcase asserts on mozilla-central revision 76820c6dff7b (run with --fuzzing-safe --ion-eager):

try {
  (function (x) {
} catch(exc1) {}
function DiagModule(stdlib, foreign) {
    "use asm";
    var test = foreign.test;
    function diag() {
        var x = 0.0;
        while (1) {
          test(1, x);
    function diag_1() {}
    return { diag: diag, diag_1:diag_1 };
var foreign = {
  test:function(a,b) {
DiagModule(this, foreign).diag();
Marked this one s-s because the assertion is range-related. Here's a backtrace:

Program received signal SIGSEGV, Segmentation fault.
0x000000000051633c in MarkRangeConservatively (end=<optimized out>, begin=<optimized out>, trc=<optimized out>) at js/src/gc/RootMarking.cpp:264
264         JS_ASSERT(begin <= end);
(gdb) bt
#0  0x000000000051633c in MarkRangeConservatively (end=<optimized out>, begin=<optimized out>, trc=<optimized out>) at js/src/gc/RootMarking.cpp:264
#1  0x0000000000517528 in MarkRangeConservatively (end=<optimized out>, begin=<optimized out>, trc=<optimized out>) at js/src/gc/RootMarking.cpp:265
#2  MarkConservativeStackRoots (trc=0x186ff60, useSavedRoots=<optimized out>) at js/src/gc/RootMarking.cpp:333
#3  0x0000000000519bf7 in js::gc::MarkRuntime (trc=0x186ff60, useSavedRoots=false) at js/src/gc/RootMarking.cpp:660
#4  0x000000000063bfba in BeginMarkPhase (rt=<optimized out>) at js/src/jsgc.cpp:2808
#5  IncrementalCollectSlice (rt=<optimized out>, budget=<optimized out>, reason=JS::gcreason::DEBUG_GC, gckind=js::GC_NORMAL) at js/src/jsgc.cpp:4238
#6  0x000000000063e188 in GCCycle (rt=0x186fc90, incremental=false, budget=<optimized out>, gckind=js::GC_NORMAL, reason=JS::gcreason::DEBUG_GC) at js/src/jsgc.cpp:4416
#7  0x000000000063e6d1 in Collect (rt=0x186fc90, incremental=false, budget=0, gckind=js::GC_NORMAL, reason=JS::gcreason::DEBUG_GC) at js/src/jsgc.cpp:4552
#8  0x0000000000557713 in NewGCThing<JSShortString, (js::AllowGC)1> (tcx=0x188b1f0, kind=<optimized out>, thingSize=<optimized out>, heap=<optimized out>) at ../jsgcinlines.h:524
#9  js_NewGCShortString<(js::AllowGC)1> (tcx=0x188b1f0) at ../jsgcinlines.h:578
#10 js::ConcatStrings<(js::AllowGC)1> (cx=0x188b1f0, left=..., right=...) at js/src/vm/String.cpp:345
#11 0x00007ffff6bc6387 in ?? ()
Whiteboard: [jsbugmon:update,bisect]
Jan: this seems related to Activations; do you suppose this is the same sort of fix as the numFrameSlots bug?
(In reply to Luke Wagner [:luke] from comment #3)
> Jan: this seems related to Activations; do you suppose this is the same sort
> of fix as the numFrameSlots bug?

I can't reproduce this on OS X (with the info in comment 0). I will try on Linux tomorrow.
Flags: needinfo?(jdemooij)
I wasn't able to repro on linux x64 (--enable-debug --disable-optimize) on the cset in comment 0 either.
Oh, there we go, --enable-debug --disable-optimize crashes x64 linux (even w/o --ion-eager or --fuzzing-safe).
Hannes, if I make TryEnablingIon always return immediately, the crash goes away so I think this is a problem with the asm.js -> Ion fast path.
Flags: needinfo?(jdemooij) → needinfo?(hv1989)
Keywords: sec-high
I had big trouble reproducing it on my own box. But decoder helped me out and provided one of his. So I'm able to reproduce and debug.
Assignee: general → hv1989
Flags: needinfo?(hv1989)
Attached patch Patch (obsolete) — Splinter Review
I made active/non-active state look more closely to what happens at construction/destruction of Activation. This  fixed it for me. This patch also fixes bug 886255
Attachment #768828 - Flags: review?(jdemooij)
Comment on attachment 768828 [details] [diff] [review]

Review of attachment 768828 [details] [diff] [review]:

::: js/src/vm/Stack.h
@@ +1178,5 @@
>      }
>      bool isActive() const {
>          return active_;
>      }
> +    virtual void setActive(JSContext *cx, bool active = true);

Can you move active_ etc from Activation to JitActivation? Other activations are never inactive so it makes sense for it to be on JitActivation I think and it avoids the virtual method.
Attachment #768828 - Flags: review?(jdemooij)
Attached patch PatchSplinter Review
Looks like the previous patch don't fix the actual issue here. I'll move that patch to bug 886255 and let jandem review my updated patch.

The bug here is that we don't zero extend the descriptor on x64. Resulting in garbage on the stack. This isn't a problem, except when shifting to get the offset. We shift the garbage in and get the wrong value.
Attachment #768828 - Attachment is obsolete: true
Attachment #770218 - Flags: review?(luke)
Attachment #770218 - Flags: review?(luke) → review+
Closed: 8 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Comment on attachment 770218 [details] [diff] [review]

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 860838

User impact if declined: No issue for x86 and arm, pretty serieus for x64. (A real security issue, with possibility to read random data, maybe even possibility to read special crafted data)

Testing completed (on m-c, etc.): m-i:2 days, m-c: 1 day

Risk to taking this patch (and alternatives if risky): Very very low. x86/arm will do the same as before this change. x64 will zerofill the upper 32bits (instead of using the garbage on the stack)

String or IDL/UUID changes made by this patch: /
Attachment #770218 - Flags: approval-mozilla-aurora?
JSBugMon: This bug has been automatically verified fixed.
Attachment #770218 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Marking Firefox 23 unaffected based on bug 860838 checkin date.
Group: core-security
You need to log in before you can comment on or make changes to this bug.