Bug 169794 – WebAssembly: spec-tests/memory.wast.js fails in debug

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[JF Bastien]

Looks related to Signaling versus BoundsChecking when calling current_memory.


(cd ./JSTests/wasm/ && ../../current-debug/bin/jsc -m --useWebAssembly=1 ./spec-tests/memory.wast.js); echo $?

ASSERTION FAILED: wasmFunction->instance()->codeBlock()->isSafeToRun(wasmFunction->instance()->memory())
../Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp(60) : EncodedJSValue JSC::callWebAssemblyFunction(JSC::ExecState *)
1   0x108d2bd7d WTFCrash
2   0x108c3e190 JSC::callWebAssemblyFunction(JSC::ExecState*)
3   0x10878ce8a JSC::LLInt::handleHostCall(JSC::ExecState*, JSC::Instruction*, JSC::JSValue, JSC::CodeSpecializationKind)
4   0x1087890bd JSC::LLInt::setUpCall(JSC::ExecState*, JSC::Instruction*, JSC::CodeSpecializationKind, JSC::JSValue, JSC::LLIntCallLinkInfo*)
5   0x108788383 JSC::LLInt::genericCall(JSC::ExecState*, JSC::Instruction*, JSC::CodeSpecializationKind)
6   0x108788273 llint_slow_path_call
7   0x108795ba9 llint_entry
8   0x1087959cf llint_entry
9   0x1087959cf llint_entry
10  0x10878e29e vmEntryToJavaScript
11  0x108723aee JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*)
12  0x1086d99ad JSC::Interpreter::execute(JSC::ModuleProgramExecutable*, JSC::ExecState*, JSC::JSModuleEnvironment*)
13  0x1089fee5c JSC::JSModuleRecord::evaluate(JSC::ExecState*)
14  0x1089f677e JSC::JSModuleLoader::evaluate(JSC::ExecState*, JSC::JSValue, JSC::JSValue, JSC::JSValue)
15  0x108a8f7d1 JSC::moduleLoaderPrototypeEvaluate(JSC::ExecState*)
16  0x445c14801028
17  0x1087959cf llint_entry
18  0x1087959cf llint_entry
19  0x1087959cf llint_entry
20  0x10878e29e vmEntryToJavaScript
21  0x108723aee JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*)
22  0x1086d81d2 JSC::Interpreter::executeCall(JSC::ExecState*, JSC::JSObject*, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&)
23  0x1088eede8 JSC::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&)
24  0x1088ef05a JSC::profiledCall(JSC::ExecState*, JSC::ProfilingReason, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&)
25  0x1089f0eb8 JSC::JSJobMicrotask::run(JSC::ExecState*)
26  0x108b56243 JSC::QueuedTask::run()
27  0x108b55e77 JSC::VM::drainMicrotasks()
28  0x10780b430 runWithScripts(GlobalObject*, WTF::Vector<Script, 0ul, WTF::CrashOnOverflow, 16ul> const&, WTF::String const&, bool, bool, bool)
29  0x1077cfcdf jscmain(int, char**)::$_9::operator()(JSC::VM&, GlobalObject*) const
30  0x1077c2e82 int runJSC<jscmain(int, char**)::$_9>(CommandLine, jscmain(int, char**)::$_9 const&)
31  0x1077c1b18 jscmain(int, char**)
139


The failing spec tests do the following:


(module (memory (data)) (func (export "memsize") (result i32) (current_memory)))
(assert_return (invoke "memsize") (i32.const 0))
(module (memory (data "")) (func (export "memsize") (result i32) (current_memory)))
(assert_return (invoke "memsize") (i32.const 0))
(module (memory (data "x")) (func (export "memsize") (result i32) (current_memory)))
(assert_return (invoke "memsize") (i32.const 1))

[JF Bastien]

I'm taking a look at this. I have a partial patch. Will continue next week.

[JF Bastien]

Created attachment 304839 [details]
patch

Turns out I did it now.

[WebKit Commit Bot]

Attachment 304839 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/wasm/WasmMemory.cpp:241:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/wasm/js/WebAssemblyMemoryConstructor.cpp:100:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/wasm/js/WebAssemblyInstanceConstructor.cpp:261:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/wasm/WasmMemoryInformation.cpp:89:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 4 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[JF Bastien]

Comment on attachment 304839 [details]
patch

Actually there's a bug when there *are* loads and stores.

What I need to do is change all provably out-of-bounds loads and stores to traps, and add a bunch of tests. Also, test when the memory is passed in as None...

Ugh, this zero-sized memory thing is silly.

[JF Bastien]

Created attachment 304950 [details]
patch

Fix a few logic errors.

[JF Bastien]

(In reply to comment #4)
> Comment on attachment 304839 [details]
> patch
> 
> Actually there's a bug when there *are* loads and stores.
> 
> What I need to do is change all provably out-of-bounds loads and stores to
> traps, and add a bunch of tests. Also, test when the memory is passed in as
> None...
> 
> Ugh, this zero-sized memory thing is silly.

I didn't do this: I ended up changing the checks so that Signaling code can't be used with None as well as BoundsChecking memory. Much simpler.

[WebKit Commit Bot]

Attachment 304950 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/wasm/WasmMemory.cpp:243:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/wasm/js/WebAssemblyMemoryConstructor.cpp:100:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/wasm/js/WebAssemblyInstanceConstructor.cpp:261:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/wasm/WasmMemoryInformation.cpp:89:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 4 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Keith Miller]

Comment on attachment 304950 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=304950&action=review

Can't we just have the Memory object point to null or something and fix weirdness with setMemory using fake memories. It seems like an anti-pattern to make everyone deal with all the edge cases of empty memories.

> Source/JavaScriptCore/wasm/WasmMemory.h:54
> +    enum class Mode {

Can we not make this an enum class? or if you do at least remove it from the Memory class. Writing Wasm::Memory::Mode everywhere is going to be super lame...

[JF Bastien]

(In reply to comment #8)
> Comment on attachment 304950 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=304950&action=review
> 
> Can't we just have the Memory object point to null or something and fix
> weirdness with setMemory using fake memories. It seems like an anti-pattern
> to make everyone deal with all the edge cases of empty memories.

I don't believe it would. We have the following cases to handle in the InstanceConstructor:

1. An imported memory which isn't empty.
2. An imported memory which is empty (max=0).
3. No memory.
4. A section-declared empty memory (max=0).
5. A section-declared memory.

I should rename "None" to "Empty" to express this.

What you're asking is: should we put 2., 3., and 4. together? I don't think so: having *no* memory means there can be no memory accesses *at all* in the module's code. That's a much stronger guarantee from saying that there can be no memory *access* which execute dynamically, but getting memory size and growing it to 0 is fine.

You'll note that we do collapse 2. and 4. together: that's because once instantiated, where the memory came from doesn't matter because the lifetime is controlled by a WebAssembly.Memory object.


> > Source/JavaScriptCore/wasm/WasmMemory.h:54
> > +    enum class Mode {
> 
> Can we not make this an enum class? or if you do at least remove it from the
> Memory class. Writing Wasm::Memory::Mode everywhere is going to be super
> lame...

I can drop that change. I think it reads better ("the memory mode is signaling" is more accurate than "the memory is signaling"), but that distracts from the patch. I could do MemoryMode::Signaling too.

In general I like enum class much more! It's way harder to mess up.

[JF Bastien]

Created attachment 305014 [details]
patch

Updated patch.

[Build Bot]

Attachment 305014 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/wasm/WasmMemory.cpp:255:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyModule.h:71:  The parameter name "mode" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/wasm/js/WebAssemblyMemoryConstructor.cpp:100:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/wasm/js/WebAssemblyInstanceConstructor.cpp:261:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/wasm/WasmMemoryInformation.cpp:89:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 5 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[JF Bastien]

Created attachment 305246 [details]
patch

Remove the enum value I was adding, Keith was right the code stays simpler without the new value. Empty is just denoted by having max=0.

[Build Bot]

Attachment 305246 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/wasm/WasmMemory.cpp:286:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyModule.h:71:  The parameter name "mode" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/wasm/WasmMemoryInformation.cpp:88:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/wasm/WasmMemoryInformation.cpp:92:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 4 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Keith Miller]

Comment on attachment 305246 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=305246&action=review

r=me if you fix the copyright thingy.

> Source/JavaScriptCore/wasm/WasmMemory.cpp:171
> +    , m_mode(MemoryMode::BoundsChecking)

Nit: I don't think you need this. It has an initializer in the class definition.

> Source/JavaScriptCore/wasm/WasmMemory.cpp:235
> +        // FIXME: should this ever occur? https://bugs.webkit.org/show_bug.cgi?id=169890

I think we have proven this can happen. I'm not sure this FIXME is needed.

> Source/JavaScriptCore/wasm/WasmMemory.cpp:298
> +            // FIXME: should this ever occur? https://bugs.webkit.org/show_bug.cgi?id=169890

ditto.

> Source/JavaScriptCore/wasm/js/JSWebAssemblyCodeBlock.cpp:65
> +        // because the page protection detect out-of-bounds accesses.

typo: detects

> Source/JavaScriptCore/wasm/js/WebAssemblyInstanceConstructor.cpp:2
> + * Copyright (C) 2016-2017 Apple Inc. All rights reserved.

Did we have changes in here earlier that we forgot to update? If so, I think you should state that's why you are changing the copyright in the Changelog.

> Source/JavaScriptCore/wasm/js/WebAssemblyMemoryConstructor.cpp:2
> + * Copyright (C) 2016-2017 Apple Inc. All rights reserved.

ditto.

[JF Bastien]

Created attachment 305324 [details]
patch

> > Source/JavaScriptCore/wasm/WasmMemory.cpp:235
> > +        // FIXME: should this ever occur? https://bugs.webkit.org/show_bug.cgi?id=169890
> 
> I think we have proven this can happen. I'm not sure this FIXME is needed.
> 
> > Source/JavaScriptCore/wasm/WasmMemory.cpp:298
> > +            // FIXME: should this ever occur? https://bugs.webkit.org/show_bug.cgi?id=169890
> 
> ditto.

Right, I want to add checks for errno though because some of the failure cases mean we messed up. So the FIXME should stay there, I'll address as part of bug #169890.

[Build Bot]

Attachment 305324 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/wasm/WasmMemory.cpp:286:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyModule.h:71:  The parameter name "mode" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/wasm/WasmMemoryInformation.cpp:88:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/wasm/WasmMemoryInformation.cpp:92:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 4 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 305324 [details]
patch

Clearing flags on attachment: 305324

Committed r214380: <http://trac.webkit.org/changeset/214380>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.