Fix crashes due to mishandling custom sections.
authorkeith_miller@apple.com <keith_miller@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 2 Feb 2018 04:30:37 +0000 (04:30 +0000)
committerkeith_miller@apple.com <keith_miller@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 2 Feb 2018 04:30:37 +0000 (04:30 +0000)
https://bugs.webkit.org/show_bug.cgi?id=182404
<rdar://problem/36935863>

Reviewed by Saam Barati.

JSTests:

* wasm/Builder.js:
(export.default.Builder.prototype._registerSectionBuilders.const.section.in.WASM.description.section.switch.section.case.string_appeared_here.this.section):
* wasm/js-api/validate.js:
(assert.truthy):

Source/JavaScriptCore:

This also cleans up some of our validation code. We also
mistakenly, allowed unknown (different from custom sections with
id: 0) section ids.

* wasm/WasmModuleParser.cpp:
(JSC::Wasm::ModuleParser::parse):
* wasm/WasmModuleParser.h:
* wasm/WasmSections.h:
(JSC::Wasm::isKnownSection):
(JSC::Wasm::decodeSection):
(JSC::Wasm::validateOrder):
(JSC::Wasm::makeString):
(JSC::Wasm::isValidSection): Deleted.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@227994 268f45cc-cd09-0410-ab3c-d52691b4dbfc

JSTests/ChangeLog
JSTests/wasm/Builder.js
JSTests/wasm/js-api/validate.js
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/wasm/WasmModuleParser.cpp
Source/JavaScriptCore/wasm/WasmModuleParser.h
Source/JavaScriptCore/wasm/WasmSections.h

index 8bec480..69b4196 100644 (file)
@@ -1,3 +1,16 @@
+2018-02-01  Keith Miller  <keith_miller@apple.com>
+
+        Fix crashes due to mishandling custom sections.
+        https://bugs.webkit.org/show_bug.cgi?id=182404
+        <rdar://problem/36935863>
+
+        Reviewed by Saam Barati.
+
+        * wasm/Builder.js:
+        (export.default.Builder.prototype._registerSectionBuilders.const.section.in.WASM.description.section.switch.section.case.string_appeared_here.this.section):
+        * wasm/js-api/validate.js:
+        (assert.truthy):
+
 2018-01-31  Saam Barati  <sbarati@apple.com>
 
         JSC incorrectly interpreting script, sets Global Property instead of Global Lexical variable (LiteralParser / JSONP path)
index 04adf8a..72e788e 100644 (file)
@@ -575,7 +575,7 @@ export default class Builder {
 
             case "Element":
                 this[section] = function(...args) {
-                    if (args.length !== 0)
+                    if (args.length !== 0 && this._checked)
                         throw new Error("You're doing it wrong. This element does not take arguments. You must chain the call with another Element()");
 
                     const s = this._addSection(section);
index 8f4e8a3..4fe83c2 100644 (file)
@@ -29,3 +29,19 @@ assert.eq(WebAssembly.validate.length, 1);
 
     assert.truthy(WebAssembly.validate(builder.WebAssembly().get()));
 }
+
+{
+    const builder = (new Builder());
+    builder.setChecked(false);
+
+    builder.Type().End()
+        .Import().Memory("imp", "memory", {initial: 20}).End()
+        .Unknown("test").End()
+        .Import().Memory("imp", "memory", {initial: 20}).End()
+        .Function().End()
+        .Export().End()
+        .Code()
+        .End();
+
+    assert.falsy(WebAssembly.validate(builder.WebAssembly().get()));
+}
index 373c820..0daafdd 100644 (file)
@@ -1,3 +1,25 @@
+2018-02-01  Keith Miller  <keith_miller@apple.com>
+
+        Fix crashes due to mishandling custom sections.
+        https://bugs.webkit.org/show_bug.cgi?id=182404
+        <rdar://problem/36935863>
+
+        Reviewed by Saam Barati.
+
+        This also cleans up some of our validation code. We also
+        mistakenly, allowed unknown (different from custom sections with
+        id: 0) section ids.
+
+        * wasm/WasmModuleParser.cpp:
+        (JSC::Wasm::ModuleParser::parse):
+        * wasm/WasmModuleParser.h:
+        * wasm/WasmSections.h:
+        (JSC::Wasm::isKnownSection):
+        (JSC::Wasm::decodeSection):
+        (JSC::Wasm::validateOrder):
+        (JSC::Wasm::makeString):
+        (JSC::Wasm::isValidSection): Deleted.
+
 2018-02-01  Michael Catanzaro  <mcatanzaro@igalia.com>
 
         -Wreturn-type warning in DFGObjectAllocationSinkingPhase.cpp
index a098789..382e6c1 100644 (file)
@@ -55,20 +55,19 @@ auto ModuleParser::parse() -> Result
     WASM_PARSER_FAIL_IF(!parseUInt32(versionNumber), "can't parse version number");
     WASM_PARSER_FAIL_IF(versionNumber != expectedVersionNumber, "unexpected version number ", versionNumber, " expected ", expectedVersionNumber);
 
-    Section previousSection = Section::Custom;
+    // This is not really a known section.
+    Section previousKnownSection = Section::Begin;
     while (m_offset < length()) {
         uint8_t sectionByte;
 
         WASM_PARSER_FAIL_IF(!parseUInt7(sectionByte), "can't get section byte");
 
         Section section = Section::Custom;
-        if (sectionByte) {
-            if (isValidSection(sectionByte))
-                section = static_cast<Section>(sectionByte);
-        }
+        WASM_PARSER_FAIL_IF(!decodeSection(sectionByte, section));
+        ASSERT(section != Section::Begin);
 
         uint32_t sectionLength;
-        WASM_PARSER_FAIL_IF(!validateOrder(previousSection, section), "invalid section order, ", previousSection, " followed by ", section);
+        WASM_PARSER_FAIL_IF(!validateOrder(previousKnownSection, section), "invalid section order, ", previousKnownSection, " followed by ", section);
         WASM_PARSER_FAIL_IF(!parseVarUInt32(sectionLength), "can't get ", section, " section's length");
         WASM_PARSER_FAIL_IF(sectionLength > length() - m_offset, section, "section of size ", sectionLength, " would overflow Module's size");
 
@@ -80,18 +79,25 @@ auto ModuleParser::parse() -> Result
             WASM_FAIL_IF_HELPER_FAILS(parse ## NAME());             \
             break;                                                  \
         }
-        FOR_EACH_WASM_SECTION(WASM_SECTION_PARSE)
+        FOR_EACH_KNOWN_WASM_SECTION(WASM_SECTION_PARSE)
 #undef WASM_SECTION_PARSE
 
         case Section::Custom: {
             WASM_FAIL_IF_HELPER_FAILS(parseCustom(sectionLength));
             break;
         }
+
+        case Section::Begin: {
+            RELEASE_ASSERT_NOT_REACHED();
+            break;
+        }
         }
 
         WASM_PARSER_FAIL_IF(end != m_offset, "parsing ended before the end of ", section, " section");
 
-        previousSection = section;
+
+        if (isKnownSection(section))
+            previousKnownSection = section;
     }
 
     return { };
index 5ad2100..a651be1 100644 (file)
@@ -48,7 +48,7 @@ public:
 private:
 
 #define WASM_SECTION_DECLARE_PARSER(NAME, ID, DESCRIPTION) PartialResult WARN_UNUSED_RETURN parse ## NAME();
-    FOR_EACH_WASM_SECTION(WASM_SECTION_DECLARE_PARSER)
+    FOR_EACH_KNOWN_WASM_SECTION(WASM_SECTION_DECLARE_PARSER)
 #undef WASM_SECTION_DECLARE_PARSER
 
     PartialResult WARN_UNUSED_RETURN parseCustom(uint32_t);
index f635c81..e6924b7 100644 (file)
@@ -34,7 +34,7 @@
 
 namespace JSC { namespace Wasm {
 
-#define FOR_EACH_WASM_SECTION(macro) \
+#define FOR_EACH_KNOWN_WASM_SECTION(macro) \
     macro(Type,     1, "Function signature declarations") \
     macro(Import,   2, "Import declarations") \
     macro(Function, 3, "Function declarations") \
@@ -48,38 +48,58 @@ namespace JSC { namespace Wasm {
     macro(Data,    11, "Data segments")
 
 enum class Section : uint8_t {
+    // It's important that Begin is less than every other section number and that Custom is greater.
+    // This only works because section numbers are currently monotonically increasing.
+    // Also, Begin is not a real section but is used as a marker for validating the ordering
+    // of sections.
+    Begin = 0,
 #define DEFINE_WASM_SECTION_ENUM(NAME, ID, DESCRIPTION) NAME = ID,
-    FOR_EACH_WASM_SECTION(DEFINE_WASM_SECTION_ENUM)
+    FOR_EACH_KNOWN_WASM_SECTION(DEFINE_WASM_SECTION_ENUM)
 #undef DEFINE_WASM_SECTION_ENUM
     Custom
 };
+static_assert(static_cast<uint8_t>(Section::Begin) < static_cast<uint8_t>(Section::Type), "Begin should come before the first known section.");
 
 template<typename Int>
-static inline bool isValidSection(Int section)
+inline bool isKnownSection(Int section)
 {
     switch (section) {
 #define VALIDATE_SECTION(NAME, ID, DESCRIPTION) case static_cast<Int>(Section::NAME): return true;
-        FOR_EACH_WASM_SECTION(VALIDATE_SECTION)
+        FOR_EACH_KNOWN_WASM_SECTION(VALIDATE_SECTION)
 #undef VALIDATE_SECTION
     default:
         return false;
     }
 }
 
-static inline bool validateOrder(Section previous, Section next)
+inline bool decodeSection(uint8_t sectionByte, Section& section)
 {
-    if (previous == Section::Custom)
+    section = Section::Custom;
+    if (!sectionByte)
         return true;
-    return static_cast<uint8_t>(previous) < static_cast<uint8_t>(next);
+
+    if (!isKnownSection(sectionByte))
+        return false;
+
+    section = static_cast<Section>(sectionByte);
+    return true;
+}
+
+inline bool validateOrder(Section previousKnown, Section next)
+{
+    ASSERT(isKnownSection(previousKnown) || previousKnown == Section::Begin);
+    return static_cast<uint8_t>(previousKnown) < static_cast<uint8_t>(next);
 }
 
-static inline const char* makeString(Section section)
+inline const char* makeString(Section section)
 {
     switch (section) {
+    case Section::Begin:
+        return "Begin";
     case Section::Custom:
         return "Custom";
 #define STRINGIFY_SECTION_NAME(NAME, ID, DESCRIPTION) case Section::NAME: return #NAME;
-        FOR_EACH_WASM_SECTION(STRINGIFY_SECTION_NAME)
+        FOR_EACH_KNOWN_WASM_SECTION(STRINGIFY_SECTION_NAME)
 #undef STRINGIFY_SECTION_NAME
     }
     ASSERT_NOT_REACHED();