1346810 - Crash [@ js::jit::MBasicBlock::add] with OOM and asm.js

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision f9362554866b (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-stdcxx-compat --disable-profiling --enable-debug --enable-optimize, run with --fuzzing-safe --thread-count=2):

oomTest(function() { eval(`
(function(stdlib, n, heap) {
    "use asm"
    var Int32ArrayView = new stdlib.Int32Array(heap)
    function f() {
        Int32ArrayView[1]
    }
`)});



Backtrace:

 received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7ffff58fa700 (LWP 24558)]
js::jit::MBasicBlock::add (this=0x7ffff5185038, ins=ins@entry=0x0) at js/src/jit/MIRGraph.cpp:1147
#0  js::jit::MBasicBlock::add (this=0x7ffff5185038, ins=ins@entry=0x0) at js/src/jit/MIRGraph.cpp:1147
#1  0x0000000000d69a22 in (anonymous namespace)::FunctionCompiler::load (this=this@entry=0x7ffff58f8080, base=0x7ffff5185218, access=access@entry=0x7ffff58f7b60, result=result@entry=js::wasm::ValType::I32) at js/src/wasm/WasmIonCompile.cpp:772
#2  0x0000000000d72990 in EmitLoad (f=..., type=type@entry=js::wasm::ValType::I32, viewType=viewType@entry=js::Scalar::Int32) at js/src/wasm/WasmIonCompile.cpp:2328
#3  0x0000000000d91278 in EmitExpr (f=...) at js/src/wasm/WasmIonCompile.cpp:3208
#4  js::wasm::IonCompileFunction (task=task@entry=0x7ffff5183c50, unit=unit@entry=0x7ffff5184710, error=error@entry=0x7ffff58f90b0) at js/src/wasm/WasmIonCompile.cpp:3724
#5  0x0000000000d4306b in js::wasm::CompileFunction (task=0x7ffff5183c50, error=error@entry=0x7ffff58f90b0) at js/src/wasm/WasmGenerator.cpp:1223
#6  0x0000000000b4d2e9 in js::HelperThread::handleWasmWorkload (this=this@entry=0x7ffff6968280, locked=...) at js/src/vm/HelperThreads.cpp:1468
#7  0x0000000000b4d9c1 in js::HelperThread::threadLoop (this=this@entry=0x7ffff6968280) at js/src/vm/HelperThreads.cpp:1929
#8  0x0000000000b4db50 in js::HelperThread::ThreadMain (arg=0x7ffff6968280) at js/src/vm/HelperThreads.cpp:1451
#9  0x0000000000b5d642 in js::detail::ThreadTrampoline<void (&)(void*), js::HelperThread*>::callMain<0ul> (this=0x7ffff691e0a0) at js/src/threading/Thread.h:234
#10 js::detail::ThreadTrampoline<void (&)(void*), js::HelperThread*>::Start (aPack=0x7ffff691e0a0) at js/src/threading/Thread.h:227
#11 0x00007ffff7bc16fa in start_thread (arg=0x7ffff58fa700) at pthread_create.c:333
#12 0x00007ffff6c38b5d in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:109
rax	0x0	0
rbx	0x7ffff5185038	140737305399352
rcx	0x4	4
rdx	0x0	0
rsi	0x0	0
rdi	0x7ffff5185218	140737305399832
rbp	0x7ffff58f7ac0	140737313209024
rsp	0x7ffff58f7aa0	140737313208992
r8	0x7ffff5185000	140737305399296
r9	0x0	0
r10	0x0	0
r11	0x0	0
r12	0x0	0
r13	0x7ffff5185060	140737305399392
r14	0x7ffff5185218	140737305399832
r15	0x7ffff5183ca0	140737305394336
rip	0x76dc48 <js::jit::MBasicBlock::add(js::jit::MInstruction*)+56>
=> 0x76dc48 <js::jit::MBasicBlock::add(js::jit::MInstruction*)+56>:	mov    %rbx,0x8(%r12)
   0x76dc4d <js::jit::MBasicBlock::add(js::jit::MInstruction*)+61>:	mov    0x18(%rbx),%rdx

Comment 1

[Fuzzing Team]

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/ecd217c511e1
user:        Tooru Fujisawa
date:        Tue Mar 07 19:54:23 2017 +0900
summary:     Bug 420857 - Part 1: Report the position of opening brace for missing brace error in function body. r=anba

This iteration took 270.567 seconds to run.

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Arai-san/Andre, is bug 420857 a likely regressor?

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

On second thoughts, wasm seems to be on the stack, so comment 1 may not be accurate, also setting needinfo? from :bbouvier.

Comment 4

[André Bargull [:anba]]

I get:
---
Assertion failure: cx->isExceptionPending() (Thunk execution failed but no exception was raised - missing call to js::ReportOutOfMemory()?), at /home/andre/hg/mozilla-inbound/js/src/builtin/TestingFunctions.cpp:1497
---

only after adding a few ReportOutOfMemory calls:
---
diff --git a/js/src/frontend/Parser.cpp b/js/src/frontend/Parser.cpp
--- a/js/src/frontend/Parser.cpp
+++ b/js/src/frontend/Parser.cpp
@@ -1012,4 +1012,6 @@ Parser<ParseHandler>::reportMissingClosi
     auto notes = MakeUnique<JSErrorNotes>();
-    if (!notes)
+    if (!notes) {
+        ReportOutOfMemory(pc->sc()->context);
         return;
+    }
 
@@ -1050,4 +1052,6 @@ Parser<ParseHandler>::reportRedeclaratio
     auto notes = MakeUnique<JSErrorNotes>();
-    if (!notes)
+    if (!notes) {
+        ReportOutOfMemory(pc->sc()->context);
         return;
+    }
 
diff --git a/js/src/jsapi.cpp b/js/src/jsapi.cpp
--- a/js/src/jsapi.cpp
+++ b/js/src/jsapi.cpp
@@ -6343,4 +6343,6 @@ CreateErrorNoteVA(JSContext* cx,
     auto note = MakeUnique<JSErrorNotes::Note>();
-    if (!note)
+    if (!note) {
+        ReportOutOfMemory(cx);
         return nullptr;
+    }
 
@@ -6426,4 +6428,6 @@ JSErrorNotes::copy(JSContext* cx)
     auto copiedNotes = MakeUnique<JSErrorNotes>();
-    if (!copiedNotes)
+    if (!copiedNotes) {
+        ReportOutOfMemory(cx);
         return nullptr;
+    }
---

I was able to reproduce the stack trace from comment #0. 

Then I landed in [1] with |load| being a nullptr, which will cause the stack from #0. Interestingly MBasicBlock::add(...) is called multiple times with possible null-pointers in WasmIonCompile.cpp, which seems incorrect at first glance.

And I still wonder why WasmIonCompile.cpp is called even though the source string in comment #0 contains a SyntaxError (missing "})" in the eval string). I'd assume the compiler wouldn't be called at all for invalid inputs, but maybe I'm missing some of the dependencies in the asm/wasm pipeline. Nevertheless the nullptr issue is also reproducible with the corrected source string.

[1] http://searchfox.org/mozilla-central/rev/a5c2b278897272497e14a8481513fee34bbc7e2c/js/src/wasm/WasmIonCompile.cpp#772

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

There's clearly a race condition on which error is going to show up first: the function is getting compiled, while the parser realizes there's something wrong and asks to abort compilation. Running with --no-threads makes the segfault in WasmIonCompile consistent.

> Then I landed in [1] with |load| being a nullptr, which will cause the stack from #0. Interestingly MBasicBlock::add(...) is called multiple times with possible null-pointers in WasmIonCompile.cpp, which seems incorrect at first glance.

There's a call to mirGen.ensureBallast before all of these functions are being called, so this is not possible to end up with a null pointer without it being explicitly returned.

What happens here is that a recent change made that MWasmLoad/MWasmStore/MAsmJSLoad/MAsmJSStore and atomics ops are variadic nodes, and can now return nullptr in their ctors, but that's not checked by the parent (this should also abort compilation). I'm on it.

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 1.variadic.patch

Variadic MIR nodes allocation errors should be propagated.

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 2.report-errornotes.patch

All credit goes to you.

Comment 8

[Lars T Hansen [:lth]]

Comment on attachment 8847057 [details] [diff] [review]
1.variadic.patch

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

Sorry about that.  I guess in this case it might have been better to crash in the constructor if the inline list couldn't be allocated, as I assume the generated new operators do.  What you do here seems fine to me.

Comment 9

[Ted Campbell [:tcampbell]]

Comment on attachment 8847060 [details] [diff] [review]
2.report-errornotes.patch

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

Agree that these are missing ReportOutOfMemory, and it looks like they are appropriate context to call it.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/88b60605154e
Make variadic nodes instanciation fallible in WasmIonCompile; r=lth
https://hg.mozilla.org/integration/mozilla-inbound/rev/af30bac5e60a
Report OOM to the JSContext for several ErrorNotes related allocations; r=tcampbell

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ecf3f3dde1e
Fix dead code mode on a CLOSED TREE; r=bustage

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/88b60605154e
https://hg.mozilla.org/mozilla-central/rev/af30bac5e60a
https://hg.mozilla.org/mozilla-central/rev/1ecf3f3dde1e

Comment 13

[Ryan VanderMeulen [:RyanVM]]

How far back does this issue go?

Comment 14

[Benjamin Bouvier [:bbouvier] (inactive)]

Patch 1 (and followup patch 3) fix issues that were on trunk only (caused by bug 1338217).

Patch 2 fix issues caused by bug 1283712, bug 420857 and bug 104442. The first two have landed in 55, the last one in 54, so 54 is affected and needs a branch patch.

Comment 15

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: aurora.patch

Approval Request Comment
[Feature/Bug causing the regression]: bug 104442
[User impact if declined]: error not getting reported to the caller in some case
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no 
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: very low
[Why is the change risky/not risky?]: just a report
[String changes made/needed]: n/a

carrying forward r+ from patch 2, from which this one is extracted.

Thank you Ryan for the heads-up!

Comment 16

[Gerry Chang [:gchang]]

Comment on attachment 8849939 [details] [diff] [review]
aurora.patch

Fix a crash. Aurora54+.

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0dfea3e5927a