1596026 - Assertion failure: IsWasmExportedFunction(fun), at js/src/wasm/WasmTable.cpp:196

Status: RESOLVED FIXED

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

Description

[Christian Holler (:decoder)]

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.

Comment 1

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 2

[Lars T Hansen [:lth]]

Cool. I'll look today.

Comment 3

[Lars T Hansen [:lth]]

Hm, wabt doesn't understand ref.func yet, and we got rid of wasmBinaryToText. Fun times.

Comment 4

[Lars T Hansen [:lth]]

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.

Comment 5

[Lars T Hansen [:lth]]

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

[Andy Wingo [:wingo]]

AFAIR wabt does allow ref.func when passed --enable-reference-types.

Comment 7

[Lars T Hansen [:lth]]

(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 :)

Comment 8

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

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.

Comment 9

[Lars T Hansen [:lth]]

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.

Comment 10

[Jan de Mooij [:jandem]]

Sure.

Comment 11

[Lars T Hansen [:lth]]

Attachment: Bug 1596026 - FuncRef abstraction + document representation of funcref. r=rhunt

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.

Comment 12

[Lars T Hansen [:lth]]

Attachment: Bug 1596026 - Do not use imported function if it's not an export. r=rhunt

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

Comment 13

[Lars T Hansen [:lth]]

I'll fix a review bot error and add a test case.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
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

[Dorel Luca [:dluca]]

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

Comment 16

[Lars T Hansen [:lth]]

The problem was the other patch in this stack.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
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

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/17984c85d3bd
https://hg.mozilla.org/mozilla-central/rev/d8324c78fc1f