Summary: | WebAssembly: disable some APIs under CSP | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | JF Bastien <jfbastien> | ||||||||||||||||||||||
Component: | JavaScriptCore | Assignee: | JF Bastien <jfbastien> | ||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||
Severity: | Normal | CC: | buildbot, cdumez, commit-queue, dbates, esprehn+autocc, fpizlo, jfbastien, kangil.han, keith_miller, mark.lam, mkwst, msaboff, rniwa, saam, webkit-bug-importer | ||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||
See Also: |
https://bugs.webkit.org/show_bug.cgi?id=111869 https://bugs.webkit.org/show_bug.cgi?id=173105 |
||||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||||
Bug Blocks: | 173977 | ||||||||||||||||||||||||
Attachments: |
|
Description
JF Bastien
2017-06-27 15:16:42 PDT
*** Bug 173688 has been marked as a duplicate of this bug. *** Created attachment 314053 [details]
patch
Here's the current patch. The test produces no output though, and I'm not sure why! Some of these should fail...
Created attachment 314058 [details]
patch
Saam helped me find my obvious mistake. I'd dropped the part that actually enables... Trying it out, and I'll add other tests. I also need to fix the async tests.
Created attachment 314071 [details]
patch
This should be ready to go!
Attachment 314071 [details] did not pass style-queue:
ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyMemory.h:44: The parameter name "exec" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 1 in 33 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 314071 [details] patch Attachment 314071 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/4016792 New failing tests: http/tests/security/contentSecurityPolicy/WebAssembly-blocked-and-sends-report.html Created attachment 314080 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 314071 [details] patch Attachment 314071 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/4016777 New failing tests: http/tests/security/contentSecurityPolicy/WebAssembly-blocked-and-sends-report.html Created attachment 314082 [details]
Archive of layout-test-results from ews112 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 314071 [details] patch Attachment 314071 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4016809 New failing tests: http/tests/security/contentSecurityPolicy/WebAssembly-blocked-and-sends-report.html http/tests/security/contentSecurityPolicy/WebAssembly-blocked-in-about-blank-iframe.html http/tests/security/contentSecurityPolicy/WebAssembly-blocked.html http/tests/security/contentSecurityPolicy/WebAssembly-blocked-in-subframe.html http/tests/security/contentSecurityPolicy/WebAssembly-blocked-in-external-script.html Created attachment 314086 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5
Comment on attachment 314071 [details] patch Attachment 314071 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/4016950 New failing tests: http/tests/security/contentSecurityPolicy/WebAssembly-blocked-and-sends-report.html Created attachment 314088 [details]
Archive of layout-test-results from ews102 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 314115 [details]
patch
Fix failures, add a test I'd missed, and fix style.
The iOS simulator failures were due to WebAssembly being disabled on the simulator.
Attachment 314115 [details] did not pass style-queue:
ERROR: LayoutTests/platform/ios-simulator/TestExpectations:137: Path does not exist. [test/expectations] [5]
ERROR: LayoutTests/platform/ios-simulator/TestExpectations:138: Path does not exist. [test/expectations] [5]
ERROR: LayoutTests/platform/ios-simulator/TestExpectations:139: Path does not exist. [test/expectations] [5]
ERROR: LayoutTests/platform/ios-simulator/TestExpectations:140: Path does not exist. [test/expectations] [5]
ERROR: LayoutTests/platform/ios-simulator/TestExpectations:141: Path does not exist. [test/expectations] [5]
Total errors found: 5 in 34 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 314118 [details]
patch
Fix path of tests in iOS simulator test expectations.
Comment on attachment 314118 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=314118&action=review This patch looks good. I had some very minor stylistic nits. Please remove leading whitespace on empty lines. Every added empty line I checked has leading whitespace. > Source/JavaScriptCore/wasm/js/WebAssemblyMemoryConstructor.cpp:103 > + auto* jsmemory = JSWebAssemblyMemory::create(exec, vm, exec->lexicalGlobalObject()->WebAssemblyMemoryStructure(), adoptRef(*memory.leakRef())); jsmemory => jsMemory (since the name contains two words: "js" - shorthand for JavaScript and "memory") > Source/WebCore/ChangeLog:13 > + Tests: http/tests/security/contentSecurityPolicy/WebAssembly-allowed.html > + http/tests/security/contentSecurityPolicy/WebAssembly-blocked-in-about-blank-iframe.html > + http/tests/security/contentSecurityPolicy/WebAssembly-blocked-in-external-script.html > + http/tests/security/contentSecurityPolicy/WebAssembly-blocked-in-subframe.html > + http/tests/security/contentSecurityPolicy/WebAssembly-blocked.html Nit: The list of tests should come after the description of the fix as seen in the example ChangeLog on <https://webkit.org/contributing-code/#changelog-files>. > Source/WebCore/ChangeLog:16 > + WebAssembly-blocked but currently only blocks neither or both. I Nit: Missing a comma before "but". > Source/WebCore/bindings/js/WorkerScriptController.cpp:198 > + JSLockHolder lock(vm()); Although it is an unwritten convention (we should make it written) we have been using C++11 brace initialization syntax in new code. Comment on attachment 314118 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=314118&action=review > LayoutTests/http/tests/security/contentSecurityPolicy/WebAssembly-blocked.html:25 > +<!-- FIXME: add WebAssembly.compile and WebAssembly.instantiate --> Why can't we do these now? We must have some promise testing api. We should at least file a bug about this. >> Source/WebCore/bindings/js/WorkerScriptController.cpp:198 >> + JSLockHolder lock(vm()); > > Although it is an unwritten convention (we should make it written) we have been using C++11 brace initialization syntax in new code. Really? We definitely don't do that in JSC... > > LayoutTests/http/tests/security/contentSecurityPolicy/WebAssembly-blocked.html:25 > > +<!-- FIXME: add WebAssembly.compile and WebAssembly.instantiate --> > > Why can't we do these now? We must have some promise testing api. We should > at least file a bug about this. https://bugs.webkit.org/show_bug.cgi?id=173977 Created attachment 314144 [details]
patch
Address comments.
Attachment 314144 [details] did not pass style-queue:
ERROR: Source/WebCore/bindings/js/WorkerScriptController.cpp:190: Missing space before { [whitespace/braces] [5]
ERROR: Source/WebCore/bindings/js/WorkerScriptController.cpp:198: Missing space before { [whitespace/braces] [5]
Total errors found: 2 in 34 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 314144 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=314144&action=review > Source/JavaScriptCore/wasm/js/JSWebAssemblyMemory.cpp:48 > + auto exception = [&] (JSObject* error) { > + throwException(exec, throwScope, error); > + return nullptr; > + }; there is no need for this function. Comment on attachment 314144 [details] patch Clearing flags on attachment: 314144 Committed r218951: <http://trac.webkit.org/changeset/218951> All reviewed patches have been landed. Closing bug. (In reply to Saam Barati from comment #23) > Comment on attachment 314144 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=314144&action=review > > > Source/JavaScriptCore/wasm/js/JSWebAssemblyMemory.cpp:48 > > + auto exception = [&] (JSObject* error) { > > + throwException(exec, throwScope, error); > > + return nullptr; > > + }; > > there is no need for this function. I imported it from another file, I like how it makes the code easier to read even if single-use here :) |