Assertion failure: IsWasmExportedFunction(fun), at js/src/wasm/WasmTable.cpp:196
Categories
(Core :: JavaScript: WebAssembly, defect, P1)
Tracking
()
Tracking | Status | |
---|---|---|
firefox-esr68 | --- | unaffected |
firefox70 | --- | wontfix |
firefox71 | --- | wontfix |
firefox72 | --- | fixed |
People
(Reporter: decoder, Assigned: lth)
References
(Regression)
Details
(Keywords: assertion, regression, testcase)
Attachments
(3 files)
The attached testcase crashes on mozilla-central revision 72c52c0101cf (build with --enable-valgrind --enable-gczeal --disable-tests --disable-profiling --enable-debug --enable-optimize, run with --no-threads test.js).
Backtrace:
received signal SIGSEGV, Segmentation fault.
js::wasm::Table::fillFuncRef (this=this@entry=0x7ffff5f654c0, index=index@entry=0, fillCount=fillCount@entry=1, ref=ref@entry=..., cx=<optimized out>) at js/src/wasm/WasmTable.cpp:196
#0 js::wasm::Table::fillFuncRef (this=this@entry=0x7ffff5f654c0, index=index@entry=0, fillCount=fillCount@entry=1, ref=ref@entry=..., cx=<optimized out>) at js/src/wasm/WasmTable.cpp:196
#1 0x000055555667d215 in js::wasm::Instance::tableSet (instance=<optimized out>, index=0, value=0x11580149ccc0, tableIndex=<optimized out>) at js/src/wasm/WasmInstance.cpp:911
#2 0x00003392e2901359 in ?? ()
#3 0x0000000000000000 in ?? ()
rax 0x5555580b3fa0 93825037713312
rbx 0x555556fb2e58 93825019883096
rcx 0x7ffff6c1c2dd 140737333281501
rdx 0x0 0
rsi 0x7ffff6eeb770 140737336227696
rdi 0x7ffff6eea540 140737336223040
rbp 0x7fffffffbfb0 140737488338864
rsp 0x7fffffffbee0 140737488338656
r8 0x7ffff6eeb770 140737336227696
r9 0x7ffff7fe6cc0 140737354034368
r10 0x58 88
r11 0x7ffff6b927a0 140737332717472
r12 0x7ffff5f654c0 140737319949504
r13 0x1 1
r14 0x11580149ccc0 19069676408000
r15 0x0 0
rip 0x555556741c89 <js::wasm::Table::fillFuncRef(unsigned int, unsigned int, js::wasm::AnyRef, JSContext*)+1145>
=> 0x555556741c89 <js::wasm::Table::fillFuncRef(unsigned int, unsigned int, js::wasm::AnyRef, JSContext*)+1145>: movl $0x0,0x0
0x555556741c94 <js::wasm::Table::fillFuncRef(unsigned int, unsigned int, js::wasm::AnyRef, JSContext*)+1156>: ud2
Marking this s-s for now because I don't know the implications of that assertion.
Reporter | ||
Comment 1•5 years ago
|
||
Assignee | ||
Comment 2•5 years ago
|
||
Cool. I'll look today.
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 3•5 years ago
|
||
Hm, wabt doesn't understand ref.func yet, and we got rid of wasmBinaryToText. Fun times.
Assignee | ||
Comment 4•5 years ago
•
|
||
Looks like the verifier allows imported JS functions to be the targets of ref.func but when we execute the ref.func we get the plain JS function object, which fails subsequent tests:
var ins = new WebAssembly.Instance(new WebAssembly.Module(wasmTextToBinary(`
(module
(import $f "m" "f" (func (param i32) (result i32)))
(elem declared $f)
(table 1 funcref)
(func (export "run")
(table.set 0 (i32.const 0) (ref.func $f))))`)),
{m:{f:(x) => 37+x}});
ins.exports.run();
This is arguably not security-sensitive in its current form because the assert that fails is a release assert (plus ref.func is only in Nightly and only behind a pref). Leaving it hidden for now just in case, but I don't see this as critical for anything.
Assignee | ||
Comment 5•5 years ago
|
||
I'm inclined to believe that the problem is the code in Instance::funcRef() which special-cases imports. It should not -- it should always take the other path, because every target of ref.func is an exported function in the sense of this code. However, this means that we have to be careful to unwrap a funcref value that escapes into host code in callExport, at a minimum - we must go from the wasm "exported function" to the original imported function. It may also be that something additional needs to be done when callImport_funcref returns, but it's not obvious that it does.
Comment 6•5 years ago
|
||
AFAIR wabt does allow ref.func when passed --enable-reference-types
.
Assignee | ||
Comment 7•5 years ago
|
||
(In reply to Andy Wingo [:wingo] from comment #6)
AFAIR wabt does allow ref.func when passed
--enable-reference-types
.
Couldn't make that work, but realized I had old wabt. Tried pulling and recompiling but realized I had removed clang because it conflicted with something in Cranelift's toolchain (sigh). Got sick of it and just intuited the test case above :)
autobisectjs shows this is probably related to the following changeset:
The first bad revision is:
changeset: https://hg.mozilla.org/mozilla-central/rev/00e5548e9c43
user: Ryan Hunt
date: Tue Aug 13 13:29:21 2019 +0000
summary: Bug 1546138 - Wasm: Implement 'ref.func'. r=bbouvier
Probably related to bug 1546138.
Assignee | ||
Comment 9•5 years ago
|
||
I now think this is not s-s. The release assert catches a problem in the engine wherein it is confused about what its canonical representation for a funcref value is -- is it the original imported JSFunction or is it maybe something like a wrapper JSFunction? But there's no type confusion and the bad value does not end up in the table, so I don't think there's any danger.
Jan, can you open this up? I have some patches coming.
Assignee | ||
Comment 11•5 years ago
|
||
Cleanup patch:
This documents the meaning of a 'funcref' value in wasm code (it is
always a pointer to a JSFunction for which IsWasmExportedFunction is
true) and implements a new abstraction, FuncRef, that can be used to
handle functions with more precision than AnyRef.
FuncRef and AnyRef are representationally compatible, so it is
possible to have common code paths using only AnyRef for convenience,
and to use FuncRef where we wish to be more explicit.
Assignee | ||
Comment 12•5 years ago
|
||
The proper test for whether ref.func should return an original
imported function for a given function index is a double test: first
whether the index is in the import range, but then whether the
original imported function is itself an exported function.
This double test is visible several places in the code, notably in
Instance::initElems and in GetFunctionExport. The second part of the
test was however missing from Instance::funcRef.
Depends on D53003
Assignee | ||
Comment 13•5 years ago
|
||
I'll fix a review bot error and add a test case.
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Comment 14•5 years ago
|
||
Pushed by lhansen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/90b91dd79951 FuncRef abstraction + document representation of funcref. r=rhunt https://hg.mozilla.org/integration/autoland/rev/3799c836c429 Do not use imported function if it's not an export. r=rhunt
Comment 15•5 years ago
|
||
Backed out 6 changesets (bug 1581572, bug 1596026) for Spidermonkey failure in js/src/wasm/WasmStubs.cpp. CLOSED TREE
Log:
https://treeherder.mozilla.org/logviewer.html#?job_id=276957819&repo=autoland
Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&revision=b50479e36a54536e902079df79efe42f12f63937
Backout:
https://hg.mozilla.org/integration/autoland/rev/522722c8c0e2ca314f00bff3f64398b237eb1385
Assignee | ||
Comment 16•5 years ago
|
||
The problem was the other patch in this stack.
Comment 17•5 years ago
|
||
Pushed by lhansen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/17984c85d3bd FuncRef abstraction + document representation of funcref. r=rhunt https://hg.mozilla.org/integration/autoland/rev/d8324c78fc1f Do not use imported function if it's not an export. r=rhunt
Comment 18•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/17984c85d3bd
https://hg.mozilla.org/mozilla-central/rev/d8324c78fc1f
Updated•5 years ago
|
Updated•2 years ago
|
Description
•