Crash [@ ??] or Assertion failure: inputStores.length() > 0, at jit/FlowAliasAnalysis.cpp:627

RESOLVED FIXED in Firefox 55

Status

()

Core
JavaScript Engine: JIT
--
critical
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: decoder, Assigned: h4writer)

Tracking

(Blocks: 1 bug, 5 keywords)

Trunk
mozilla55
x86
Linux
assertion, crash, jsbugmon, regression, testcase
Points:
---

Firefox Tracking Flags

(firefox52 disabled, firefox-esr52 disabled, firefox53 disabled, firefox54 disabled, firefox55 fixed)

Details

(Whiteboard: [jsbugmon:update,bisect], crash signature)

Attachments

(1 attachment)

(Reporter)

Description

a year ago
The following testcase crashes on mozilla-central revision 71224049c0b5 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-debug --without-intl-api --enable-optimize --target=i686-pc-linux-gnu, run with --fuzzing-safe --ion-aa=flow-sensitive):

while (true) {
    try {
        var buf = new Uint8ClampedArray(a);
    } catch (e) {
        break;
    }
}
while (true) {
    var e = inIon() ? -true.get : 1;
    while ([]) {
        inIon = 'x1';
    }
}



Backtrace:

 received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0xf7767b40 (LWP 12102)]
0x00000000 in ?? ()
#0  0x00000000 in ?? ()
#1  0x0829d590 in js::jit::MDefinition::is<js::jit::MStoreSlot> (this=0xf1350444) at js/src/jit/MIR.h:889
#2  js::jit::MDefinition::isStoreSlot (this=0xf1350444) at js/src/jit/MIR.h:909
#3  js::jit::MLoadSlot::mightAlias (this=0xf134e890, def=0xf1350444) at js/src/jit/MIR.cpp:5206
#4  0x0829ff13 in js::jit::MInstruction::foldsToStore (this=0xf134e890, alloc=...) at js/src/jit/MIR.cpp:295
#5  0x082a0183 in js::jit::MLoadSlot::foldsTo (this=0xf134e890, alloc=...) at js/src/jit/MIR.cpp:5230
#6  0x08335dbd in js::jit::ValueNumberer::simplified (def=0xf134e890, this=0xf77671ac) at js/src/jit/ValueNumbering.cpp:618
#7  js::jit::ValueNumberer::visitDefinition (this=0xf77671ac, def=0xf134e890) at js/src/jit/ValueNumbering.cpp:778
#8  0x0833691a in js::jit::ValueNumberer::visitBlock (this=0xf77671ac, block=0xf134e1e0, dominatorRoot=0xf79808b8) at js/src/jit/ValueNumbering.cpp:997
#9  0x08336d65 in js::jit::ValueNumberer::visitDominatorTree (this=0xf77671ac, dominatorRoot=0xf79808b8) at js/src/jit/ValueNumbering.cpp:1042
#10 0x08336fae in js::jit::ValueNumberer::visitGraph (this=<optimized out>) at js/src/jit/ValueNumbering.cpp:1080
#11 js::jit::ValueNumberer::run (this=0xf77671ac, updateAliasAnalysis=js::jit::ValueNumberer::UpdateAliasAnalysis) at js/src/jit/ValueNumbering.cpp:1251
#12 0x08209dc2 in js::jit::OptimizeMIR (mir=0xf1349130) at js/src/jit/Ion.cpp:1724
#13 0x0820acf0 in js::jit::CompileBackEnd (mir=0xf1349130) at js/src/jit/Ion.cpp:2068
#14 0x0854c5a0 in js::HelperThread::handleIonWorkload (this=0xf7941000, locked=...) at js/src/vm/HelperThreads.cpp:1508
#15 0x0854d3ec in js::HelperThread::threadLoop (this=0xf7941000) at js/src/vm/HelperThreads.cpp:1871
#16 0x08553cbb in js::detail::ThreadTrampoline<void (&)(void*), js::HelperThread*>::callMain<0u> (this=0xf790b080) at js/src/threading/Thread.h:234
#17 js::detail::ThreadTrampoline<void (&)(void*), js::HelperThread*>::Start (aPack=0xf790b080) at js/src/threading/Thread.h:227
#18 0xf7fb228a in start_thread (arg=0xf7767b40) at pthread_create.c:333
#19 0xf7cd44ce in clone () from /lib32/libc.so.6
eax	0xf1350444	-248183740
ebx	0xf1350444	-248183740
ecx	0x8	8
edx	0xf1350444	-248183740
esi	0x8959ff4	144023540
edi	0xf134e890	-248190832
ebp	0xf7766f18	4151734040
esp	0xf7766efc	4151734012
eip	0x0	0
=> 0x0:	


This looks like a jump to NULL but I'm not sure if this is guaranteed, so marking s-s until it has been investigated. This also only crashes on 32 bit while it also asserts on 64 bit.
Component: JavaScript Engine → JavaScript Engine: JIT
It looks like you changed FlowAliasAnalysis.cpp recently, nbp, can you take a look at this? Thanks.
Flags: needinfo?(nicolas.b.pierron)
I will look at it next week, if I have time during a firefox compilation.
Otherwise, Hannes feel free to take it.
Flags: needinfo?(hv1989)
This sounds potentially bad so I'm going to mark it sec-high. Hopefully somebody can take a look soon.
Keywords: sec-high
Flags: needinfo?(nihsanullah)
(Assignee)

Comment 4

a year ago
The correct people are needinfo'ed. This crash is flow alias analysis which is not enabled by default, currently.
Flags: needinfo?(nihsanullah)
(In reply to Hannes Verschore [:h4writer] from comment #4)
> The correct people are needinfo'ed. This crash is flow alias analysis which
> is not enabled by default, currently.

If there's a bug for enabling it by default, can you make this blocking it? Thanks.
status-firefox54: affected → disabled
(Assignee)

Comment 6

a year ago
Created attachment 8839858 [details] [diff] [review]
Patch

Due to GVN we sometimes get a disconnected graph. In that state (before fixup) the first block of the graph is not the entry of the graph. It only get deleted lateron till the start of the OSR. (Since that is the entry in that case). As a result in FlowAA we don't have any dependencies in the fake-entry and we are asserting. We can just run no FlowAA on those. They will get deleted after GVN ends and we re-run AA again.
Assignee: nobody → hv1989
Flags: needinfo?(hv1989)
Attachment #8839858 - Flags: review?(jdemooij)
(Assignee)

Updated

a year ago
Flags: needinfo?(nicolas.b.pierron)
Comment on attachment 8839858 [details] [diff] [review]
Patch

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

Hm annoying.
Attachment #8839858 - Flags: review?(jdemooij) → review+
Comment on attachment 8839858 [details] [diff] [review]
Patch

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

::: js/src/jit/FlowAliasAnalysis.cpp
@@ +469,5 @@
>          BlockStoreInfo& blockInfo = stores_->current();
> +
> +        // When the store dependencies is empty it means we have a disconnected
> +        // graph. Those blocks will never get reached but it is only fixed up
> +        // after GVN. Don't run AA on those blocks.

Are these OSR-dominated blocks, or dead blocks?

During GVN all loop header should have an extra predecessor to keep them alive, if the blocks are OSR-dominated, then they got to keep these fix-up blocks as unreachable fake parent block.
Hannes, this bug is assigned to you and the patch has an r+. What's the next step? :)
Flags: needinfo?(hv1989)
(Assignee)

Comment 10

a year ago
Opening since this is not enabled by default path.

(In reply to Frederik Braun [:freddyb] from comment #9)
> Hannes, this bug is assigned to you and the patch has an r+. What's the next
> step? :)

Landing would be the next step.
Group: javascript-core-security
Flags: needinfo?(hv1989)
Keywords: sec-high

Comment 11

a year ago
Pushed by hv1989@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8cba3e613627
IonMonkey - Don't do flow-aa on blocks without entry predecessor, r=jandem

Comment 12

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8cba3e613627
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
status-firefox52: --- → disabled
status-firefox53: --- → disabled
status-firefox-esr52: --- → disabled
You need to log in before you can comment on or make changes to this bug.