Closed Bug 1628499 Opened 4 years ago Closed 4 years ago

Assertion failure: size_t(np) == countedPointers, at wasm/WasmBaselineCompile.cpp:2667

Categories

(Core :: JavaScript: WebAssembly, defect, P1)

x86_64
Linux
defect

Tracking

()

VERIFIED FIXED
mozilla77
Tracking Status
firefox-esr68 --- unaffected
firefox75 --- disabled
firefox76 --- disabled
firefox77 --- verified

People

(Reporter: decoder, Assigned: wingo)

Details

(4 keywords, Whiteboard: [bugmon:update,bisect])

Attachments

(2 files)

The following testcase crashes on mozilla-central revision 20200408-6663d3dc883b (build with --enable-debug, run with --fuzzing-safe --no-threads):

var g63 = newGlobal({newCompartment: true});
g63.parent = this;
g63.eval("var dbg = new Debugger(parent);");
function testAddNextThreeInts(text, imports) {
    new WebAssembly.Instance(
      new WebAssembly.Module(wasmTextToBinary(text)
    ), { imports });
}
testAddNextThreeInts(`
  (func $boxNextInt (import "imports" "boxNextInt")
    (result anyref))
  (func $boxNextThreeInts (result anyref anyref anyref)
    call $boxNextInt
    call $boxNextThreeInts
  )
`, {});

Backtrace:

received signal SIGSEGV, Segmentation fault.
0x0000555556808b95 in js::wasm::StackMapGenerator::createStackMap(char const*, mozilla::Vector<bool, 32ul, js::SystemAllocPolicy> const&, unsigned int, js::wasm::HasRefTypedDebugFrame, mozilla::Vector<js::wasm::Stk, 0ul, js::SystemAllocPolicy> const&) ()
#0  0x0000555556808b95 in js::wasm::StackMapGenerator::createStackMap(char const*, mozilla::Vector<bool, 32ul, js::SystemAllocPolicy> const&, unsigned int, js::wasm::HasRefTypedDebugFrame, mozilla::Vector<js::wasm::Stk, 0ul, js::SystemAllocPolicy> const&) ()
#1  0x00005555567c23d3 in js::wasm::BaseCompiler::emitCall() ()
#2  0x00005555567dae1d in js::wasm::BaseCompiler::emitBody() ()
#3  0x00005555567e8b2f in js::wasm::BaselineCompileFunctions(js::wasm::ModuleEnvironment const&, js::LifoAlloc&, mozilla::Vector<js::wasm::FuncCompileInput, 8ul, js::SystemAllocPolicy> const&, js::wasm::CompiledCode*, mozilla::UniquePtr<char [], JS::FreePolicy>*) ()
#4  0x000055555686d6cc in ExecuteCompileTask(js::wasm::CompileTask*, mozilla::UniquePtr<char [], JS::FreePolicy>*) ()
#5  0x000055555686e5b7 in js::wasm::ModuleGenerator::finishFuncDefs() ()
#6  0x00005555567fa81e in bool DecodeCodeSection<js::wasm::Decoder>(js::wasm::ModuleEnvironment const&, js::wasm::Decoder&, js::wasm::ModuleGenerator&) ()
#7  0x00005555567fa3b9 in js::wasm::CompileBuffer(js::wasm::CompileArgs const&, js::wasm::ShareableBytes const&, mozilla::UniquePtr<char [], JS::FreePolicy>*, mozilla::Vector<mozilla::UniquePtr<char [], JS::FreePolicy>, 0ul, js::SystemAllocPolicy>*, JS::OptimizedEncodingListener*) ()
#8  0x00005555568d1596 in js::WasmModuleObject::construct(JSContext*, unsigned int, JS::Value*) ()
#9  0x0000555555904ef2 in CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&) ()
[...]
#21 0x000055555578827d in main ()
rax	0x555556ff2c3d	93825020144701
rbx	0x2	2
rcx	0x555557f46850	93825036216400
rdx	0x0	0
rsi	0x7ffff6efd770	140737336301424
rdi	0x7ffff6efc540	140737336296768
rbp	0x7fffffff9050	140737488326736
rsp	0x7fffffff8fe0	140737488326624
r8	0x7ffff6efd770	140737336301424
r9	0x7ffff7f9bd00	140737353727232
r10	0x58	88
r11	0x7ffff6ba47a0	140737332791200
r12	0x7ffff5e28410	140737318650896
r13	0x3	3
r14	0xc	12
r15	0x0	0
rip	0x555556808b95 <js::wasm::StackMapGenerator::createStackMap(char const*, mozilla::Vector<bool, 32ul, js::SystemAllocPolicy> const&, unsigned int, js::wasm::HasRefTypedDebugFrame, mozilla::Vector<js::wasm::Stk, 0ul, js::SystemAllocPolicy> const&)+1701>
=> 0x555556808b95 <_ZN2js4wasm17StackMapGenerator14createStackMapEPKcRKN7mozilla6VectorIbLm32ENS_17SystemAllocPolicyEEEjNS0_21HasRefTypedDebugFrameERKNS5_INS0_3StkELm0ES6_EE+1701>:	movl   $0xa6b,0x0
   0x555556808ba0 <_ZN2js4wasm17StackMapGenerator14createStackMapEPKcRKN7mozilla6VectorIbLm32ENS_17SystemAllocPolicyEEEjNS0_21HasRefTypedDebugFrameERKNS5_INS0_3StkELm0ES6_EE+1712>:	callq  0x555555814706 <abort>
Attached file Testcase

Andy, related to multi-value most likely, could be a dup of another we've seen this weekend.

FYI the debugger bits just serve to force usage of the baseline compiler; this is a bug in the baseline compiler. Which unfortunately has existed for a while! Patch will have details.

Assignee: nobody → wingo

The baseline compiler needs to adjust its virtual stack to match the
machine stack at control-flow joins, and also when preparing stack
result areas for calls. In the latter case, there may be temporary
values on the stack, so the stack height before pushing stack results is
not the current block's stack height.

The patch above fixes the issue. The problem was that the captured Stk entries were incorrectly placed as if they had been pushed from the block's stackHeight, instead of the stackHeight before the call -- i.e. two Stk entries were actually at the same location. So, the MST pointer addition at https://searchfox.org/mozilla-central/source/js/src/wasm/WasmBaselineCompile.cpp#2604 happened twice for the same location.

To me it would seem that you wouldn't ever want to add one location two times, but adding an assert in MachineStackTracker::setGCPointer that the entry is currently false causes a couple test case failures:

FAIL - wasm/gc/anyref-boxing-struct.js
FAIL - wasm/gc/stackmaps4-params-n-locals.js

Probably worth looking into.

Similarly I tried added an assert in pushBlockResults that the incoming fr.stackHeight() was the block stack height, but it was not an invariant (many non-multi-value cases fail), for reasons I don't fully understand.

This is a nightly-only s-s bug as per comment 3 and IRC discussion. No sec-approval required to land the fix.

Group: javascript-core-security
Priority: -- → P1

Similar test case identified by :decoder:, also fails in a similar place:

setJitCompilerOption("wasm.ion", 0);
new WebAssembly.Module(wasmTextToBinary(`
  (module
    (func (result anyref f64 anyref)
      (call 0)
      (call 0)
      (call 0)
      (unreachable)))`));

Am triaging to see if this fix is incomplete.

Yaaargh, this is the same bug but in a different place. The stack result locations are correctly pushed onto the virtual stack before the call, but not after. Recall that in general there will be some machine stack space taken up by arguments, unless all arguments are constants. Stack result locations are pushed on after the arguments have been computed but before the call, so after the call they may need to be shuffled down. Therefore their offsets change, in general. (Not in this case, as there are no arguments, but that's not important.)

The bug comes in where we capture the results on the stack. At blocks, we do this at the block stack height. Here we need to take into account any values pushed during the course of this block, but just as we were failing to take this into account before a call, we also fail to do so after.

Hmmm. I worry slightly that we have a breakdown of abstractions in the compiler / we need better abstractions to protect us from these types of errors.

Yeah I hear what you are saying! Updated patch does slightly rework some of the abstractions.

Keywords: sec-high
Group: javascript-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla77
Status: RESOLVED → VERIFIED
Keywords: bugmon
Bugmon Analysis:
Verified bug as fixed on rev mozilla-central 20200423145559-03626342f6e6.
Removing bugmon keyword as no further action possible.
Please review the bug and re-add the keyword for further analysis.
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: