Assertion failure: index < length_, at jit/FixedList.h:82 or Crash [@ ??]

VERIFIED FIXED in Firefox 39, Firefox OS master

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: decoder, Assigned: nbp)

Tracking

(Blocks: 1 bug, 5 keywords)

Trunk
mozilla39
x86_64
Linux
assertion, crash, regression, sec-high, testcase
Points:
---

Firefox Tracking Flags

(firefox37 unaffected, firefox38 unaffected, firefox39 verified, firefox-esr31 unaffected, b2g-v1.4 unaffected, b2g-v2.0 unaffected, b2g-v2.0M unaffected, b2g-v2.1 unaffected, b2g-v2.1S unaffected, b2g-v2.2 unaffected, b2g-master fixed)

Details

(Whiteboard: [jsbugmon:update,ignore])

Attachments

(2 attachments)

(Reporter)

Description

3 years ago
The following testcase crashes on mozilla-central revision eea6188b9b05 (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-debug, run with --no-threads --ion-eager):

var T = TypedObject;
var ST = new T.StructType({x:T.int32});
function check(v) {
    return v.toSource();
}
function test() {
    var fake = { toSource: ST.toSource };
    try { 
        check(fake); 
    } catch (e) {}
}
test();
var uint8 = TypedObject.uint8;
function runTests() {
  uint8.toSource();
}
runTests();
test();



Backtrace:

Program received signal SIGSEGV, Segmentation fault.
0x00000000017b7ea0 in ?? ()
#0  0x00000000017b7ea0 in ?? ()
#1  0x00000000007230a8 in is<js::jit::MPhi> (this=0x17b8bd0) at js/src/jit/MIR.h:747
#2  isPhi (this=0x17b8bd0) at js/src/jit/MIR.h:767
#3  js::jit::EliminatePhis (mir=0x17b38c8, graph=..., observe=js::jit::ConservativeObservability) at js/src/jit/IonAnalysis.cpp:739
#4  0x00000000007aadaf in js::jit::ScalarReplacement (mir=0x17b38c8, graph=...) at js/src/jit/ScalarReplacement.cpp:1073
#5  0x000000000073d3e3 in js::jit::OptimizeMIR (mir=mir@entry=0x17b38c8) at js/src/jit/Ion.cpp:1231
#6  0x000000000073dad9 in js::jit::CompileBackEnd (mir=mir@entry=0x17b38c8) at js/src/jit/Ion.cpp:1580
#7  0x000000000073eade in IonCompile (optimizationLevel=<optimized out>, recompile=false, constructing=<optimized out>, osrPc=<optimized out>, baselineFrame=<optimized out>, script=0x0, cx=0x16b7ce0) at js/src/jit/Ion.cpp:1952
#8  js::jit::Compile (cx=cx@entry=0x16b7ce0, script=script@entry=0x7ffff7e5e1f0, osrFrame=osrFrame@entry=0x0, osrPc=osrPc@entry=0x0, constructing=<optimized out>, forceRecompile=forceRecompile@entry=false) at js/src/jit/Ion.cpp:2106
#9  0x000000000073ee03 in js::jit::CanEnter (cx=0x16b7ce0, state=...) at js/src/jit/Ion.cpp:2245
#10 0x0000000000547f88 in js::RunScript (cx=cx@entry=0x16b7ce0, state=...) at js/src/vm/Interpreter.cpp:424
#11 0x00000000005483aa in js::Invoke (cx=cx@entry=0x16b7ce0, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:517
#12 0x0000000000548f9b in js::Invoke (cx=0x16b7ce0, thisv=..., fval=..., argc=0, argv=<optimized out>, rval=...) at js/src/vm/Interpreter.cpp:554
#13 0x00000000006bb5fd in js::jit::DoCallFallback (cx=0x16b7ce0, frame=0x7fffffffd308, stub_=<optimized out>, argc=0, vp=0x7fffffffd2c8, res=JSVAL_VOID) at js/src/jit/BaselineIC.cpp:9578
[...]
#24 0x0000000000000000 in ?? ()
rax	0x17ba818	24881176
rbx	0x17ba4e8	24880360
rcx	0x1	1
rdx	0x17ba598	24880536
rsi	0x17b8bd0	24873936
rdi	0x17b8bd0	24873936
rbp	0x17b8bd0	24873936
rsp	0x7fffffffbca8	140737488338088
r8	0x2	2
r9	0x20	32
r10	0x17ba7d8	24881112
r11	0x17ba4a8	24880296
r12	0x1	1
r13	0x2	2
r14	0x17b56a8	24860328
r15	0x17ba7f8	24881144
rip	0x17b7ea0	24870560
=> 0x17b7ea0:	pushq  $0x17ba8
   0x17b7ea5:	add    %al,(%rax)


Marking this s-s because the assertion indicates some out-of-bounds access.
JSBugMon is down so NI from nbp because ScalarReplacement is on the stack.
Flags: needinfo?(nicolas.b.pierron)
(Assignee)

Comment 2

3 years ago
I am unable to reproduce this issue with either a debug/optimized build while compiling with clang/gcc4.8.
Running multiple times does not show intermittent behaviour.
Flags: needinfo?(nicolas.b.pierron)
decoder: what compiler did you use, same as nbp?
Flags: needinfo?(choller)
(Assignee)

Comment 4

3 years ago
clang version 3.3 (tags/RELEASE_33/final)
gcc (GCC) 4.8.2

Decoder mention that he was no longer able to reproduce it on tip, so I will try to see if I can reproduce it on top of the revision mentionned in comment 0.
Flags: needinfo?(nicolas.b.pierron)
(Reporter)

Updated

3 years ago
Flags: needinfo?(choller)
(Assignee)

Comment 5

3 years ago
I can reproduce this issue on top of revision eea6188b9b05.
I will investigate.
(Assignee)

Comment 6

3 years ago
So far the problem seems to be that DescrToSource does

  UnsafeGetStringFromReservedSlot(this, 1);

which is inlined as a MLoadFixedSlot, where |this| is a MNewObject which has the following template object:

object 0x7ffff7e6d1f0
class 0x150a890 Object
flags:
proto <Object at 0x7ffff7e5b020>
parent <global object at 0x7ffff7e5a060>
properties:
    ((js::Shape *) 0x7ffff7e7de20) enumerate JSString* (0x7ffff7e1c970) = Latin1Char * (0x7ffff7e1c978) = "toSource"
: slot 0 = undefined

Apparently, MCallOptimize does not guard on the object type before doing the MLoadFixedSlot(slot = 1).
(Assignee)

Comment 7

3 years ago
Ok, the problem is simple, Scalar Replacement is eager to replace the MLoadFixedSlot from the MNewObject, even if the path which is being optimized can never happen.  This problem is likely to exists since the introduction of Scalar Replacement.
(Assignee)

Comment 8

3 years ago
Created attachment 8572730 [details] [diff] [review]
Check if Loads can be optimized by Scalar Replcement.

This patch add inevitable bailouts on the paths where we saw Load/Store
instructions which are making references to slots which are not defined as
part of the shape of the NewObject.

This is not a big deal, as these branches are likely to be removed by UCE
(within GVN).

The problem seen here is that the self-hosted code has guards to check that
the object is correct, before doing any operations.  These guards are
replaced by constants when the function calls are inlined.  Unfortunately,
Scalar replacement has to run before the Apply Types phase, which runs
before UCE (within GVN), and thus see both branches, including the dead-one.
Attachment #8572730 - Flags: review?(jdemooij)
(Assignee)

Comment 9

3 years ago
Created attachment 8572731 [details] [diff] [review]
Add comments and test.
Attachment #8572731 - Flags: review?(jdemooij)
(Reporter)

Updated

3 years ago
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update,ignore]
(Reporter)

Comment 10

3 years ago
JSBugMon: The testcase found in this bug no longer reproduces (tried revision 993eb76a8bd6).
JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/2a0481539f38
user:        Nicolas B. Pierron
date:        Thu Feb 26 16:18:41 2015 +0100
summary:     Bug 1129313 - Scalar Replacement: Remove PostWriteBarrier at the same time as the stores. r=h4writer

This iteration took 145.106 seconds to run.
(Assignee)

Comment 11

3 years ago
(In reply to Christian Holler (:decoder) from comment #10)
> The first bad revision is:
> summary:     Bug 1129313 - Scalar Replacement: Remove PostWriteBarrier at
> the same time as the stores. r=h4writer

I think the problem existed before this, as this patch only re-enabled scalar replacement after it got accidently disabled by the addition of new MIR nodes.
Flags: needinfo?(nicolas.b.pierron)
Comment on attachment 8572730 [details] [diff] [review]
Check if Loads can be optimized by Scalar Replcement.

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

Looks good. Some comments below that we can maybe fix in a separate patch.

::: js/src/jit/MIR.h
@@ +3180,5 @@
>          replaceOperand(slot + 1, def);
>      }
>  
> +    bool hasFixedSlot(uint32_t slot) const {
> +        return slot < numSlots() && slot < numFixedSlots();

I think it'd be nice to have the invariant numFixedSlots() <= numSlots(), to make it easier to reason about the code and so that we only need to check numFixedSlots().

@@ +3192,5 @@
>          setSlot(slot, def);
>      }
>  
> +    bool hasDynamicSlot(uint32_t slot) const {
> +        return numFixedSlots() < numSlots() && slot < numSlots() - numFixedSlots();

Then we could get rid of the && LHS here too.
Attachment #8572730 - Flags: review?(jdemooij) → review+
Comment on attachment 8572731 [details] [diff] [review]
Add comments and test.

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

::: js/src/jit/ScalarReplacement.cpp
@@ +437,5 @@
>          state_ = BlockState::Copy(alloc_, state_);
>          state_->setFixedSlot(ins->slot(), ins->value());
>          ins->block()->insertBefore(ins->toInstruction(), state_);
>      } else {
> +        // UnsafeSetReserveSlot can inject baked-in slots which are guarded by

Nit: maybe s/inject/access/ ?
Attachment #8572731 - Flags: review?(jdemooij) → review+
Keywords: sec-high
status-firefox38: --- → unaffected
status-firefox-esr31: --- → unaffected
(Assignee)

Updated

3 years ago
Assignee: nobody → nicolas.b.pierron
Status: NEW → ASSIGNED
(Assignee)

Comment 14

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/0942dd86e375
(Assignee)

Updated

3 years ago
Attachment #8572730 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/0942dd86e375
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox39: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
status-b2g-v1.4: --- → unaffected
status-b2g-v2.0: --- → unaffected
status-b2g-v2.0M: --- → unaffected
status-b2g-v2.1: --- → unaffected
status-b2g-v2.1S: --- → unaffected
status-b2g-v2.2: --- → unaffected
status-b2g-master: --- → fixed
status-firefox37: --- → unaffected
(Reporter)

Updated

3 years ago
Status: RESOLVED → VERIFIED
status-firefox39: fixed → verified
(Reporter)

Comment 16

3 years ago
JSBugMon: This bug has been automatically verified fixed.
Group: core-security

Comment 17

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd1d4134f29c
https://hg.mozilla.org/mozilla-central/rev/cd1d4134f29c

Comment 19

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/72341fb355fa
https://hg.mozilla.org/mozilla-central/rev/72341fb355fa
You need to log in before you can comment on or make changes to this bug.