164023 – WebAssembly JS API: Module should decode strings

RESOLVED FIXED 164023

WebAssembly JS API: Module should decode strings

https://bugs.webkit.org/show_bug.cgi?id=164023

Summary WebAssembly JS API: Module should decode strings

JF Bastien

Reported 2016-10-26 10:47:05 PDT

The spec says: https://github.com/WebAssembly/design/blob/master/JS.md#webassemblymodule-constructor > The spec string values inside Ast.module are decoded as UTF8 as described in Web.md. Referring to: https://github.com/WebAssembly/design/blob/master/Web.md#names > A WebAssembly module imports and exports functions. WebAssembly names functions using arbitrary-length byte sequences. Any 8-bit values are permitted in a WebAssembly name, including the null byte and byte sequences that don't correspond to any Unicode code point regardless of encoding. The most natural Web representation of a mapping of function names to functions is a JS object in which each function is a property. Property names in JS are UTF-16 encoded strings. A WebAssembly module may fail validation on the Web if it imports or exports functions whose names do not transcode cleanly to UTF-16 according to the following conversion algorithm, assuming that the WebAssembly name is in a Uint8Array called array: > > function convertToJSString(array) > { > var string = ""; > for (var i = 0; i < array.length; ++i) > string += String.fromCharCode(array[i]); > return decodeURIComponent(escape(string)); > } > This performs the UTF8 decoding (decodeURIComponent(escape(string))) using a common JS idiom. Transcoding failure is detected by decodeURIComponent, which may throw URIError. If it does, the WebAssembly module will not validate. This validation rule is only mandatory for Web embedding.

Attachments
patch (134.56 KB, patch) 2016-11-02 16:42 PDT, JF Bastien	no flags	Details Formatted Diff Diff
patch (133.73 KB, patch) 2016-11-02 17:13 PDT, JF Bastien	no flags	Details Formatted Diff Diff
patch (134.82 KB, patch) 2016-11-02 17:42 PDT, JF Bastien	keith_miller: review- keith_miller: commit-queue-	Details Formatted Diff Diff
patch (147.77 KB, patch) 2016-11-03 17:29 PDT, JF Bastien	keith_miller: review+	Details Formatted Diff Diff
patch (150.30 KB, patch) 2016-11-03 22:24 PDT, JF Bastien	no flags	Details Formatted Diff Diff
patch (148.94 KB, patch) 2016-11-04 14:40 PDT, JF Bastien	no flags	Details Formatted Diff Diff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.

JF Bastien

Comment 1 2016-10-26 10:53:00 PDT

This follows up with Instance's: If the list of module.imports is not empty and Type(importObject) is not Object, a TypeError is thrown.

JF Bastien

Comment 2 2016-10-26 11:01:38 PDT

As well as: Let exports be a list of (string, JS value) pairs that is mapped from each external value e in instance.exports as follows: [...]

JF Bastien

Comment 3 2016-10-28 17:33:43 PDT

*** Bug 164039 has been marked as a duplicate of this bug. ***

JF Bastien

Comment 4 2016-11-02 16:42:37 PDT

Created attachment 293714 [details] patch There's still a bunch more to do, but that's a decent enough spot to get a review and maybe commit.

WebKit Commit Bot

Comment 5 2016-11-02 16:44:47 PDT

Attachment 293714 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/ChangeLog:9: Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: malicious [changelog/unwantedsecurityterms] [3] ERROR: Source/JavaScriptCore/wasm/WasmFormat.h:106: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: JSTests/ChangeLog:9: Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: malicious [changelog/unwantedsecurityterms] [3] ERROR: Source/JavaScriptCore/wasm/WasmParser.h:100: 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 26 files If any of these errors are false positives, please file a bug against check-webkit-style.

JF Bastien

Comment 6 2016-11-02 17:13:10 PDT

Created attachment 293723 [details] patch Un-spaghetti some horrible code that had grown very ugly.

WebKit Commit Bot

Comment 7 2016-11-02 17:14:44 PDT

Attachment 293723 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/ChangeLog:9: Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: malicious [changelog/unwantedsecurityterms] [3] ERROR: Source/JavaScriptCore/wasm/WasmFormat.h:106: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: JSTests/ChangeLog:9: Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: malicious [changelog/unwantedsecurityterms] [3] ERROR: Source/JavaScriptCore/wasm/WasmParser.h:100: 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 26 files If any of these errors are false positives, please file a bug against check-webkit-style.

JF Bastien

Comment 8 2016-11-02 17:42:23 PDT

Created attachment 293726 [details] patch Fix Type section parsing, it now matches the builder's generation.

WebKit Commit Bot

Comment 9 2016-11-02 17:44:41 PDT

Attachment 293726 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/ChangeLog:9: Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: malicious [changelog/unwantedsecurityterms] [3] ERROR: Source/JavaScriptCore/wasm/WasmFormat.h:106: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: JSTests/ChangeLog:9: Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: malicious [changelog/unwantedsecurityterms] [3] ERROR: Source/JavaScriptCore/wasm/WasmParser.h:102: 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/WasmParser.h:130: 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 26 files If any of these errors are false positives, please file a bug against check-webkit-style.

Keith Miller

Comment 10 2016-11-03 09:33:48 PDT

Comment on attachment 293726 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=293726&action=review r- on the lack of assert(...). It makes the code very hard to read. > JSTests/ChangeLog:3 > + WebAssembly JS API: implement more sections This should have a link to the bugzilla. > JSTests/ChangeLog:26 > + Reviewed by NOBODY (OOPS!). I don't think it matters but usually people the "Reviewed by ..." above the comments on what the patch does. > JSTests/wasm/Builder.js:51 > + if (!Array.isArray(params)) throw new Error(`expected a parameter array, got "${params}"`); > + for (const p of params) if (!WASM.isValidValueType(p)) throw new Error(`Type parameter ${p} isn't a valid value type`); > + if (typeof(ret) === "undefined") ret = "void"; > + if (Array.isArray(ret)) throw new Error(`Multiple return values aren't supported by WebAssembly yet`); > + if (ret !== "void" && !WASM.isValidValueType(ret)) throw new Error(`Type return ${ret} isn't a valid value type`); These should all have a new line and elsewhere in patch. It's generally against webkit style to have the if and the single statement on the same line. It might be easier to read if you have an assert function. like: assert(ret !== "void" && !WASM.isValidValueType(ret), `Type return ${ret} isn't a valid value type`); an assert function would make it clearer where the interesting control flow is and where error checking is happening. > JSTests/wasm/Builder.js:69 > + types: nit: I hate labels... > JSTests/wasm/Builder.js:73 > + for (let j = 0; j !== t.params.length; ++j) Should have { } > JSTests/wasm/Builder.js:77 > + break types; I think you can just use break here. > JSTests/wasm/Builder.js:117 > + case "undefined": index = field; break; // Assume it's the same as the field (i.e. it's not being renamed). multi-line

JF Bastien

Comment 11 2016-11-03 17:29:15 PDT

Created attachment 293835 [details] patch Address Keith's comments. Lots of asserts now!

WebKit Commit Bot

Comment 12 2016-11-03 17:31:13 PDT

Attachment 293835 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/ChangeLog:12: Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: malicious [changelog/unwantedsecurityterms] [3] ERROR: Source/JavaScriptCore/wasm/WasmFormat.h:106: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: JSTests/ChangeLog:12: Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: malicious [changelog/unwantedsecurityterms] [3] ERROR: Source/JavaScriptCore/wasm/WasmParser.h:102: 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/WasmParser.h:130: 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 28 files If any of these errors are false positives, please file a bug against check-webkit-style.

JF Bastien

Comment 13 2016-11-03 22:24:22 PDT

Created attachment 293867 [details] patch Merge patch and fix new code which used an API that I renamed.

WebKit Commit Bot

Comment 14 2016-11-03 22:27:02 PDT

Attachment 293867 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/ChangeLog:12: Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: malicious [changelog/unwantedsecurityterms] [3] ERROR: Source/JavaScriptCore/wasm/WasmFormat.h:106: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: JSTests/ChangeLog:12: Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: malicious [changelog/unwantedsecurityterms] [3] ERROR: Source/JavaScriptCore/wasm/WasmParser.h:102: 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/WasmParser.h:130: 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 28 files If any of these errors are false positives, please file a bug against check-webkit-style.

Keith Miller

Comment 15 2016-11-04 12:06:05 PDT

Comment on attachment 293835 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=293835&action=review r=me with some more comments. Looks like you need to rebase. > Source/JavaScriptCore/wasm/WasmModuleParser.cpp:182 > - > + > if (verbose) > dataLogLn("Got function type."); > - > + > uint32_t argumentCount; > if (!parseVarUInt32(argumentCount)) > return false; > - > + > if (verbose) > dataLogLn("argumentCount: ", argumentCount); > - > + undo. > Source/JavaScriptCore/wasm/WasmModuleParser.cpp:288 > + // FIXME ditto. > Source/JavaScriptCore/wasm/WasmModuleParser.cpp:372 > + // FIXME Add a comment / link to bugzilla? > Source/JavaScriptCore/wasm/WasmModuleParser.cpp:378 > + // FIXME ditto. > Source/JavaScriptCore/wasm/WasmModuleParser.cpp:409 > + // FIXME ditto. > Source/JavaScriptCore/wasm/WasmParser.h:47 > + bool WARN_UNUSED_RETURN consumeUTF8String(String &, size_t); String& > Source/JavaScriptCore/wasm/WasmParser.h:100 > +ALWAYS_INLINE bool Parser::consumeUTF8String(String &result, size_t stringLength) Should be String& result > Source/JavaScriptCore/wasm/WasmPlan.h:57 > + const Memory* getMemory() const I think this should just be memory(), that's more consistent with the naming we use elsewhere.

JF Bastien

Comment 16 2016-11-04 14:40:17 PDT

Created attachment 293931 [details] patch Addressed all comments.

WebKit Commit Bot

Comment 17 2016-11-04 15:15:12 PDT

Comment on attachment 293931 [details] patch Clearing flags on attachment: 293931 Committed r208401: <http://trac.webkit.org/changeset/208401>

WebKit Commit Bot

Comment 18 2016-11-04 15:15:16 PDT

All reviewed patches have been landed. Closing bug.

Keith Miller

Comment 19 2016-11-04 17:17:51 PDT

It looks like this patch breaks all the testWasm tests :(. We need to rebaseline those tests until we can migrate them to the new testing infrastructure.

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version WebKit Nightly Build

Hardware Unspecified

OS Unspecified

Product WebKit

Component JavaScriptCore

Assignee

JF Bastien

Reported

2016-10-26 10:47 PDT

Modified

2016-11-04 17:17 PDT History

CC List

2 users Show

URL

Keywords

Duplicates (1)

164039 View as bug list

Depends on

Blocks

161709

Dependencies

tree graph