Bug 234967 - JSC::Wasm::setWasmTableElement() falls through ASSERT_NOT_REACHED()
Summary: JSC::Wasm::setWasmTableElement() falls through ASSERT_NOT_REACHED()
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebAssembly (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on: 234932
Blocks:
  Show dependency treegraph
 
Reported: 2022-01-07 10:38 PST by David Kilzer (:ddkilzer)
Modified: 2022-01-21 09:23 PST (History)
2 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2022-01-07 10:38:47 PST
JSC::Wasm::setWasmTableElement() falls through ASSERT_NOT_REACHED().

The functions should return `false` after both ASSERT_NOT_REACHED() statements rather than falling through and returning `true`.

static bool setWasmTableElement(Instance* instance, unsigned tableIndex, uint32_t index, EncodedJSValue encValue)
{   
    ASSERT(tableIndex < instance->module().moduleInformation().tableCount());
    
    if (index >= instance->table(tableIndex)->length())
        return false;
    
    JSValue value = JSValue::decode(encValue); 
    if (instance->table(tableIndex)->type() == Wasm::TableElementType::Externref)
        instance->table(tableIndex)->set(index, value);
    else if (instance->table(tableIndex)->type() == Wasm::TableElementType::Funcref) {
        WebAssemblyFunction* wasmFunction;
        WebAssemblyWrapperFunction* wasmWrapperFunction;
        
        if (isWebAssemblyHostFunction(instance->owner<JSObject>()->vm(), value, wasmFunction, wasmWrapperFunction)) {
            ASSERT(!!wasmFunction || !!wasmWrapperFunction);
            if (wasmFunction)
                instance->table(tableIndex)->asFuncrefTable()->setFunction(index, jsCast<JSObject*>(value), wasmFunction->importableFunction(), &wasmFunction->instance()->instance());
            else
                instance->table(tableIndex)->asFuncrefTable()->setFunction(index, jsCast<JSObject*>(value), wasmWrapperFunction->importableFunction(), &wasmWrapperFunction->instance()->instance());
        } else if (value.isNull())
            instance->table(tableIndex)->clear(index);
        else
            ASSERT_NOT_REACHED();
    } else
        ASSERT_NOT_REACHED();
    
    return true;
}

See Source/JavaScriptCore/wasm/WasmOperations.cpp.
Comment 1 Radar WebKit Bug Importer 2022-01-07 10:39:05 PST
<rdar://problem/87260620>
Comment 2 Keith Miller 2022-01-21 09:23:32 PST
I don't think it matters if it returns `true` or `false`. That return boolean is to flag whether an Out of Bounds memory exception should be thrown but any access triggering one of those asserts isn't out of bounds.

Do you think that an OoB exception is better than a no-op if we have a bug in our implementation? It seems to me a no-op is easier to work around for a dev?