1373663 - Assertion failure: off.assigned() && offset >= 0 && unsigned(offset) < size(), at js/src/jit/shared/IonAssemblerBuffer.h:380 with wasm

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 035c25bef7b5 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-stdcxx-compat --disable-profiling --enable-debug --without-intl-api --enable-optimize --target=i686-pc-linux-gnu --enable-simulator=arm, run with --fuzzing-safe --thread-count=2 --arm-asm-nop-fill=1 --ion-offthread-compile=off):

var lfLogBuffer = `
function getAllOffsets(wast) {
  var sandbox = newGlobal('');
  var dbg = new Debugger();
  dbg.addDebuggee(sandbox);
  sandbox.eval(\`
    var wasm = wasmTextToBinary('\${wast}');
    var m = new WebAssembly.Instance(new WebAssembly.Module(wasm));
  \`);
  var lines = wasmScript.source.text.split('\\n');
}
var result1 = getAllOffsets('(module (func (nop)))');
var nopLine = result1.filter(i => i.str.indexOf('nop') >= 0);
assertEq(nopLine.length, 1);
assertEq(nopLine[0].offsets.length, 1);
assertEq(nopLine[0].offsets[0] > 0, true);
var singleOffsetLines = result1.filter(i => i.offsets.length === 1);
`;
loadFile(lfLogBuffer);
function loadFile(lfVarx) {
    try {
       oomTest(new Function(lfVarx));
       oomTest(function() {});
       oomTest(function() {});
    } catch (lfVare) {}
}



Backtrace:

 received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0xf6762b40 (LWP 11488)]
0x084a6420 in js::jit::AssemblerBuffer<1024, js::jit::Instruction>::getInst (this=0xf5dc1930, off=...) at js/src/jit/shared/IonAssemblerBuffer.h:380
#0  0x084a6420 in js::jit::AssemblerBuffer<1024, js::jit::Instruction>::getInst (this=0xf5dc1930, off=...) at js/src/jit/shared/IonAssemblerBuffer.h:380
#1  0x084d78cc in js::jit::Assembler::editSrc (bo=..., this=0xf5dc17dc) at js/src/jit/arm/Assembler-arm.h:1244
#2  js::jit::MacroAssemblerARM::offsetToInstruction (this=0xf5dc17dc, offs=...) at js/src/jit/arm/MacroAssembler-arm.cpp:382
#3  0x088f077c in js::jit::MacroAssembler::patchAdd32ToPtr (imm=..., offset=..., this=0xf5dc17dc) at js/src/jit/arm/MacroAssembler-arm-inl.h:326
#4  js::wasm::BaseCompiler::endFunction (this=0xf676161c) at js/src/wasm/WasmBaselineCompile.cpp:2344
#5  js::wasm::BaseCompiler::emitFunction (this=0xf676161c) at js/src/wasm/WasmBaselineCompile.cpp:7408
#6  0x088f1799 in js::wasm::BaselineCompileFunction (task=0xf5dc1798, unit=0xf5dc1e68, error=0xf6761b48) at js/src/wasm/WasmBaselineCompile.cpp:7599
#7  0x08936043 in js::wasm::CompileFunction (task=0xf5dc1798, error=0xf6761b48) at js/src/wasm/WasmGenerator.cpp:1263
#8  0x0874aacd in js::HelperThread::handleWasmWorkload (this=0xf795f150, locked=..., assumeThreadAvailable=false) at js/src/vm/HelperThreads.cpp:1657
#9  0x0874b1d1 in js::HelperThread::threadLoop (this=0xf795f150) at js/src/vm/HelperThreads.cpp:2146
#10 0x0874b518 in js::HelperThread::ThreadMain (arg=0xf795f150) at js/src/vm/HelperThreads.cpp:1640
#11 0x0874b58b in js::detail::ThreadTrampoline<void (&)(void*), js::HelperThread*>::callMain<0u> (this=0xf790b098) at js/src/threading/Thread.h:234
#12 js::detail::ThreadTrampoline<void (&)(void*), js::HelperThread*>::Start (aPack=0xf790b098) at js/src/threading/Thread.h:227
#13 0xf7fb228a in start_thread (arg=0xf6762b40) at pthread_create.c:333
#14 0xf7cd94ce in clone () from /lib32/libc.so.6
eax	0x0	0
ebx	0x8d1eff4	147976180
ecx	0xf7da4864	-136689564
edx	0x0	0
esi	0x0	0
edi	0x0	0
ebp	0xf67614b8	4134933688
esp	0xf6761490	4134933648
eip	0x84a6420 <js::jit::AssemblerBuffer<1024, js::jit::Instruction>::getInst(js::jit::BufferOffset)+336>
=> 0x84a6420 <js::jit::AssemblerBuffer<1024, js::jit::Instruction>::getInst(js::jit::BufferOffset)+336>:	movl   $0x0,0x0
   0x84a642a <js::jit::AssemblerBuffer<1024, js::jit::Instruction>::getInst(js::jit::BufferOffset)+346>:	ud2    


I'm filing this s-s because the assertion looks potentially dangerous and I don't know what role the Debugger plays in the test (i.e. if it is really required).

Comment 1

[Luke Wagner [:luke]]

I can reproduce on tip with a plain arm32-sim debug build, passing only --arm-asm-nop-fill=1 at runtime.  Passing --enable-optimize and --no-threads makes it run much faster (14s vs. 3min).

I think the Debugger is only incidental; it just activates wasm baseline and if I comment out the two Debugger lines and pass --wasm-always-baseline the assert reproduces.  The bug seems to be patching + --arm-asm-nop-fill=1 + OOM.

Also: if I leave off --arm-asm-nop-fill=1, we hit another assert (cx->isExceptionPending()) suggesting we're just not reporting an OOM properly here.

Can you take a look Lars?

Comment 2

[Lars T Hansen [:lth]]

Sure, I'll take a look, thanks for simplifying.  The optimizations that I made to the ARM assembler buffer made some assumptions about how OOM reporting could be delayed (so as to reduce the number of instructions along the hot path); nbp and I both thought those optimizations were correct but they could plausibly not be.  Also, I do remember some special cases wrt NOP fill.  A bit of a minefield really.

Comment 3

[Lars T Hansen [:lth]]

Also, without --wasm-always-baseline and without the Debugger and without --arm-asm-nop-fill=1 I still hit an OOM assert, probably the one Luke found.  Here "tc" has the debugger constructor call and the addDebuggee call disabled:

(lldb) run --no-threads ~/moz/tc.js

* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
    frame #0: 0x0044cce6 js`OOMTest(cx=<unavailable>, argc=<unavailable>, vp=<unavailable>) at TestingFunctions.cpp:1553 [opt]
   1550	                           "Thunk execution succeeded but an exception was raised - "
   1551	                           "missing error check?");
   1552	            } else if (expectExceptionOnFailure) {
-> 1553	                MOZ_ASSERT(cx->isExceptionPending(),
   1554	                           "Thunk execution failed but no exception was raised - "
   1555	                           "missing call to js::ReportOutOfMemory()?");

The OOM handling around the assembler seems a little brittle.  There are tests for !masm.oom() scattered here and there in the compilers, apparently to guard against specific problems while we're compiling.  Then at some point it becomes necessary to test masm.oom() to check whether the compilation succeeded.

There seems to be an invariant that a CodeOffset value (as we have in the original assert here) can be invalid as a result of OOM, with the implied guarantee that we'll check for OOM (and thus for validity of these CodeOffsets) before processing those CodeOffsets for patching.  That type of check is not happening in Rabaldr.  In any case, the fix for that assert is to propely check for OOM in the baseline compiler.  But that does not fix the second missing-oom-check assert.

Comment 4

[Lars T Hansen [:lth]]

Attachment: bug1373663-check-oom-before-patching.patch

Fix the original problem (the assertion in the call from Rabaldr) by establishing the non-oom precondition before patching. 

Does not fix the missing-oom-check assertion we're also seeing; other patch to follow.

Comment 5

[Lars T Hansen [:lth]]

Attachment: bug1373663-set-oom-flag.patch

This should fix the remaining missing-oom-report problems, if I have read the code correctly.

At three locations in code that landed recently (with the Module sharing) there is a check for allocation failure, where the allocating code does not have a JSContext* but the detecing code does.  Here we need to report the OOM failure to the context.

Comment 6

[Luke Wagner [:luke]]

Comment on attachment 8879899 [details] [diff] [review]
bug1373663-check-oom-before-patching.patch

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

Thanks!

Comment 7

[Luke Wagner [:luke]]

Comment on attachment 8879954 [details] [diff] [review]
bug1373663-set-oom-flag.patch

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

d'oh!

Comment 8

[Lars T Hansen [:lth]]

The first problem (the missing oom check before using a bogus buffer offset) /might/ be useful in some kind of attack even if it seems very hard to use reliably.  The patch should apply to beta without adjustments.

Comment 9

[Lars T Hansen [:lth]]

Comment on attachment 8879899 [details] [diff] [review]
bug1373663-check-oom-before-patching.patch

Approval Request Comment
[Feature/Bug causing the regression]:
WebAssembly baseline compiler (used for debugging only, at the moment)

[User impact if declined]:
Remote possibility of attack vector being exposed.

[Is this code covered by automated tests?]:
No.

[Has the fix been verified in Nightly?]:
Code is landing as we speak, we have a fuzzing test.

[Needs manual test from QE? If yes, steps to reproduce]: 
Probably not needed, it was easy to repro locally and now it's gone.

[List of other uplifts needed for the feature/fix]:
None.

[Is the change risky?]:
No.

[Why is the change risky/not risky?]:
Adds an OOM test that should have been there all along.

[String changes made/needed]:
None.

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/f64f3c74c7c0
https://hg.mozilla.org/mozilla-central/rev/24ba76b2331e

Comment 11

[Julien Cristau [:jcristau] (back April 22)]

Should this have a security rating?

Comment 12

[Lars T Hansen [:lth]]

If it should have a security rating it should be "not very sensitive", see comment 8.

Comment 13

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 8879899 [details] [diff] [review]
bug1373663-check-oom-before-patching.patch

oom check in wasm baseline compiler, beta55+

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a088ee43f0f0
https://hg.mozilla.org/releases/mozilla-beta/rev/8987f5c03017

Comment 15

[Al Billings [:abillings - ex-MoCo]]

(In reply to Julien Cristau [:jcristau] from comment #11)
> Should this have a security rating?

This should have had a rating (like all security bugs) before checkin.