1906451 - Table.set incorrectly set entries to null for not-nullable typed tables

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, defect)

Description

[Alessandro Pignotti]

Attachment: typedtable.wasm

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:127.0) Gecko/20100101 Firefox/127.0

Steps to reproduce:

1. Create a WebAssembly module with a typed table using the typed funcrefs feature
2. Initialize the table from the same module using the elements sections
3. Export the table
4. Read one of the previously initialized entries from the table, which is not-null as expected
5. Store the entry back in the table using the Table.set method
6. Extract the entry one more time

Attached is a minimal wasm file demonstrating the problem, it can be tested together with the following JS snippet designed to run on the Spidermonkey shell

{
        var buffer = await read('typedtable.wasm', 'binary');
        var ret = await WebAssembly.instantiate(buffer,{i:{m:new WebAssembly.Memory({initial:16384,maximum:16384,shared:true})}})
        var instance = ret.instance;
        var tbl = instance.exports.tbl;
        var oldFunc = tbl.get(0);
        if(oldFunc == null)
                console.log("Invalid test case: function is null");
        else
                console.log("Expected: Function is not null");
        tbl.set(0, oldFunc);
        var newFunc = tbl.get(0);
        if(oldFunc != newFunc)
                console.log("Failed: Table.set nullifies not-null table entry");
        else
                console.log("Success");
}
test();

Actual results:

Roundtripping the WebAssembly function via Table.get and Table.set introduces spurious null values in the table.

Expected results:

The function should correctly be stored in the table

Comment 1

[Alessandro Pignotti]

Please note that a line got lost during copy-paste above. The script should start with:

async function test()

Comment 2

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

async function test()
{
        var buffer = await read('typedtable.wasm', 'binary');
        var ret = await WebAssembly.instantiate(buffer,{i:{m:new WebAssembly.Memory({initial:16384,maximum:16384,shared:true})}})
        var instance = ret.instance;
        var tbl = instance.exports.tbl;
        var oldFunc = tbl.get(0);
        if(oldFunc == null)
                print("Invalid test case: function is null");
        else
                print("Expected: Function is not null");
        tbl.set(0, oldFunc);
        var newFunc = tbl.get(0);
        if(oldFunc != newFunc)
                print("Failed: Table.set nullifies not-null table entry");
        else
                print("Success");
}
test();

changeset:   https://hg.mozilla.org/mozilla-central/rev/3e4e0fef13e0
user:        Yury Delendik
date:        Thu Sep 28 21:46:35 2023 +0000
summary:     Bug 1845373 - Enable Wasm GC proposal in release. r=rhunt

Prior to this changeset, the testcase shows a types not enabled error, but after this changeset landed (and till now), it has always showed:

Expected: Function is not null
Failed: Table.set nullifies not-null table entry

I cannot seem to find a changeset where the Success message was shown. In any case, perhaps Yury/Ryan might know more?

Comment 3

[Ryan Hunt [:rhunt]]

This is a pre-existing bug in our wasm-GC implementation. CheckRefType returns the value through the funcref out-param, but the typed funcref case doesn't handle this right.

I don't know of any code in our system that would be exploitable by unexpectedly getting null for a non-nullable funcref, so I don't believe this is exploitable.

Comment 4

[Ryan Hunt [:rhunt]]

Attachment: Bug 1906451 - wasm: Always return an AnyRef from CheckRefType. r?bvisness

CheckRefType used to return through one of two out-params depending
on what kind of reftype you asked for. This was not correctly handled
for typed references to functions. This is overall error prone for
little benefit, so this commit refactors this so that we always
return through an anyref out-param. Users who want a JSFunction*
can just grab that through that anyref easily.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/210ec778dbfc
wasm: Always return an AnyRef from CheckRefType. r=bvisness

Comment 6

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/210ec778dbfc