Closed Bug 938130 Opened 7 years ago Closed 6 years ago

Assertion failure: returnAddr < method_->raw() + method_->instructionsSize(), at jit/BaselineJIT.cpp:573 or Crash [@ GetBytecodeLength] with parallel compilation


(Core :: JavaScript Engine: JIT, defect, critical)

Not set



Tracking Status
firefox25 --- unaffected
firefox26 --- unaffected
firefox27 --- unaffected
firefox28 + verified
firefox-esr17 --- unaffected
firefox-esr24 --- unaffected
b2g18 --- unaffected
b2g-v1.1hd --- unaffected
b2g-v1.2 --- unaffected


(Reporter: decoder, Assigned: djvj)



(5 keywords)


(2 files)

The following testcase asserts on mozilla-central revision 7b014f0f3b03 (threadsafe build, run with --fuzzing-safe --ion-parallel-compile=on --ion-eager):

function f() {    }
function g() {    }
var x = [f,f,f,undefined,g];
for (var i = 0; i < 5; ++i)
  y = x[i]("x");
Crash trace:

Program received signal SIGSEGV, Segmentation fault.
js::jit::BaselineScript::pcForReturnOffset (this=<optimized out>, script=<optimized out>, nativeOffset=135957195) at  js/src/jit/BaselineJIT.cpp:729
729             curPC += GetBytecodeLength(curPC);
(gdb) bt 8
#0  js::jit::BaselineScript::pcForReturnOffset (this=<optimized out>, script=<optimized out>, nativeOffset=135957195) at  js/src/jit/BaselineJIT.cpp:729
#1  0x000000000070b121 in baselineScriptAndPc (pcRes=<synthetic pointer>, scriptRes=0x7fffffffd420, this=0x7fffffffd3a0) at  js/src/jit/IonFrames.cpp:204
#2  js::jit::GetPcScript (cx=<optimized out>, scriptRes=0x7fffffffd420, pcRes=0x7fffffffd4c0) at  js/src/jit/IonFrames.cpp:1169
#3  0x00000000004f0680 in currentScript (allowCrossCompartment=JSContext::DONT_ALLOW_CROSS_COMPARTMENT, ppc=<optimized out>, this=0x1450940) at ../jscntxtinlines.h:529
#4  JSContext::currentScript (this=0x1450940, ppc=<optimized out>, allowCrossCompartment=JSContext::DONT_ALLOW_CROSS_COMPARTMENT) at ../jscntxtinlines.h:511
#5  0x00000000004e68ab in GetPropertyHelperInline<(js::AllowGC)1> (vp=JSVAL_VOID, id=$jsid("__noSuchMethod__"), receiver=(JSObject * const) 0x7ffff5d4efa0 [object Array], obj=
    (JSObject * const) 0x7ffff5d4efa0 [object Array], cx=0x1450940) at  js/src/jsobj.cpp:4228
#6  js::baseops::GetProperty (cx=0x1450940, obj=(JSObject * const) 0x7ffff5d4efa0 [object Array], receiver=(JSObject * const) 0x7ffff5d4efa0 [object Array], id=$jsid("__noSuchMethod__"), vp=JSVAL_VOID)
    at  js/src/jsobj.cpp:4303
#7  0x0000000000585af5 in getGeneric (vp=JSVAL_VOID, id=$jsid("__noSuchMethod__"), receiver=..., obj=..., cx=0x1450940) at ../jsobj.h:994
(More stack frames follow...)
(gdb) info reg
rbp            0x101522aac      0x101522aac
(gdb) x /i $pc
=> 0x6745e5 <js::jit::BaselineScript::pcForReturnOffset(JSScript*, unsigned int)+117>:  movzbl 0x0(%rbp),%edx

Marking s-s and sec-critical based on the crash trace and assertion.

Djvj: Is the __noSuchMethod__ stuff in the trace in any way related to your recent changes?
Keywords: crash, sec-critical
(In reply to Christian Holler (:decoder) from comment #2)

> Djvj: Is the __noSuchMethod__ stuff in the trace in any way related to your
> recent changes?

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
user:        Kannan Vijayan
date:        Tue Nov 12 14:20:34 2013 -0500
summary:     Bug 912303 - Added noSuchMethod support to baseline CALLPROP/CALLELEM stubs. r=efaust

autoBisect thinks so, too?
Yeah, the issue is here:

    entersStubFrame_ = true;
    if (isCallElem_) {
        Label afterNoSuchMethod;
        Label skipNoSuchMethod;
        regs = availableGeneralRegs(0);

        masm.loadValue(element, R1);
        masm.branchTestUndefined(Assembler::NotEqual, R1, &skipNoSuchMethod);

        // Call __noSuchMethod__ checker.  Object pointer is in objReg.
        scratchReg = regs.takeAnyExcluding(BaselineTailCallReg);
        enterStubFrame(masm, scratchReg);

Can't leave the R1 pushed naked before entering the stub frame, because entering the stub frame pops the returnAddr, which has been displaced by R1.  So enterStubFrame thinks the value that was in R1 is the returnAddr.

Pushing R1 before the enterStubFrame makes no sense.
Assignee: general → kvijayan
Flags: needinfo?(kvijayan)
Fix for issue.  Mostly involves register shuffling for the sake of x86 to avoid the push.
Attachment #8335330 - Flags: review?
Attachment #8335330 - Flags: review? → review?(efaustbmo)
Comment on attachment 8335330 [details] [diff] [review]

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

Blegh. Sorry this was missed in review the first time. I totally forgot about the TailCallRegister pop.

Good. r=me.

::: js/src/jit/BaselineIC.cpp
@@ +4453,5 @@
> +        ValueOperand val = regs.takeValueOperand();
> +
> +        masm.loadValue(element, val);
> +        masm.branchTestUndefined(Assembler::NotEqual, val, &skipNoSuchMethod);
> +        regs.add(val);

blech, register pressure :P

::: js/src/jit/RegisterSets.h
@@ +415,5 @@
>          // registers are sometimes more efficient (e.g. optimized encodings for
>          // EAX on x86).
>          return getFirst();
>      }
> +    T getAnyExcluding(T preclude) {

This is a nice simplification.
Attachment #8335330 - Flags: review?(efaustbmo) → review+
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Hi decoder - did this miss our auto verification? Wanted to be sure it was fixed.
Flags: needinfo?(choller)
This is from a threadsafe build and JSBugMon cannot verify those yet. Verified fixed manually.
Flags: needinfo?(choller)
Group: core-security
You need to log in before you can comment on or make changes to this bug.