Bug 165833

We should probably adopt the same limits as other browsers. They're fairly conservative, and having the same limits means one of us doesn't reject an odd binary whereas another does, reducing the risk that a developer tries it out in browser X and it fails in Y.

Checked<> is indeed your friend -- you could also just make a specific override for the webasm case (one that takes a failure block or some such). The implementation actually predates blocks, etc.

<rdar://problem/29760219>

Created attachment 310233 [details] patch

Comment on attachment 310233 [details] patch r=me.

Comment on attachment 310233 [details] patch Makes some benchmark sad. Weird that V8 uses this...

Created attachment 310240 [details] patch Drop function size limit, it was ridiculously low.

Comment on attachment 310240 [details] patch Rejecting attachment 310240 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 310240, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in Source/JavaScriptCore/ChangeLog contains OOPS!. Full output: http://webkit-queues.webkit.org/results/3749015

Comment on attachment 310240 [details] patch >From 3d44a3ee1dc1f9dd20bd1dc2588c42ac70e3f424 Mon Sep 17 00:00:00 2001 >From: JF Bastien <jfbastien@apple.com> >Date: Mon, 15 May 2017 23:50:37 -0700 >Subject: [PATCH] WebAssembly: enforce size limits > >--- > Source/JavaScriptCore/ChangeLog | 16 +++++++ > .../JavaScriptCore.xcodeproj/project.pbxproj | 4 ++ > Source/JavaScriptCore/wasm/WasmLimits.h | 55 ++++++++++++++++++++++ > Source/JavaScriptCore/wasm/WasmModuleParser.cpp | 17 ++++--- > Source/JavaScriptCore/wasm/WasmParser.h | 3 ++ > 5 files changed, 88 insertions(+), 7 deletions(-) > create mode 100644 Source/JavaScriptCore/wasm/WasmLimits.h > >diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog >index 6954d9b..f7694af 100644 >--- a/Source/JavaScriptCore/ChangeLog >+++ b/Source/JavaScriptCore/ChangeLog >@@ -1,3 +1,19 @@ >+2017-05-15 JF Bastien <jfbastien@apple.com> >+ >+ WebAssembly: enforce size limits >+ https://bugs.webkit.org/show_bug.cgi?id=165833 >+ <rdar://problem/29760219> >+ >+ Reviewed by Keith Miller. >+ >+ Use the same limits as V8. >+ >+ * JavaScriptCore.xcodeproj/project.pbxproj: >+ * wasm/WasmLimits.h: Added. >+ * wasm/WasmModuleParser.cpp: >+ * wasm/WasmParser.h: >+ (JSC::Wasm::Parser<SuccessType>::consumeUTF8String): >+ > 2017-05-15 Mark Lam <mark.lam@apple.com> > > Rolling out r214038 and r213697: Crashes when using computed properties with rest destructuring and object spread. >diff --git a/Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj b/Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj >index 58d1829..19aa6e7 100644 >--- a/Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj >+++ b/Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj >@@ -2080,6 +2080,7 @@ > A7FB61001040C38B0017A286 /* PropertyDescriptor.h in Headers */ = {isa = PBXBuildFile; fileRef = A7FB604B103F5EAB0017A286 /* PropertyDescriptor.h */; settings = {ATTRIBUTES = (Private, ); }; }; > A7FCC26D17A0B6AA00786D1A /* FTLSwitchCase.h in Headers */ = {isa = PBXBuildFile; fileRef = A7FCC26C17A0B6AA00786D1A /* FTLSwitchCase.h */; settings = {ATTRIBUTES = (Private, ); }; }; > A8A4748E151A8306004123FF /* libWTF.a in Frameworks */ = {isa = PBXBuildFile; fileRef = A8A4748D151A8306004123FF /* libWTF.a */; }; >+ AD00659E1ECAC812000CA926 /* WasmLimits.h in Headers */ = {isa = PBXBuildFile; fileRef = AD00659D1ECAC7FE000CA926 /* WasmLimits.h */; settings = {ATTRIBUTES = (Private, ); }; }; > AD2FCBE21DB58DAD00B3E736 /* JSWebAssemblyCompileError.cpp in Sources */ = {isa = PBXBuildFile; fileRef = AD2FCBA61DB58DA400B3E736 /* JSWebAssemblyCompileError.cpp */; }; > AD2FCBE31DB58DAD00B3E736 /* JSWebAssemblyCompileError.h in Headers */ = {isa = PBXBuildFile; fileRef = AD2FCBA71DB58DA400B3E736 /* JSWebAssemblyCompileError.h */; }; > AD2FCBE41DB58DAD00B3E736 /* JSWebAssemblyInstance.cpp in Sources */ = {isa = PBXBuildFile; fileRef = AD2FCBA81DB58DA400B3E736 /* JSWebAssemblyInstance.cpp */; }; >@@ -4711,6 +4712,7 @@ > A8A4748D151A8306004123FF /* libWTF.a */ = {isa = PBXFileReference; lastKnownFileType = archive.ar; path = libWTF.a; sourceTree = BUILT_PRODUCTS_DIR; }; > A8E894310CD0602400367179 /* JSCallbackObjectFunctions.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = JSCallbackObjectFunctions.h; sourceTree = "<group>"; }; > A8E894330CD0603F00367179 /* JSGlobalObject.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = JSGlobalObject.h; sourceTree = "<group>"; }; >+ AD00659D1ECAC7FE000CA926 /* WasmLimits.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = WasmLimits.h; sourceTree = "<group>"; }; > AD1CF06816DCAB2D00B97123 /* PropertyTable.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = PropertyTable.cpp; sourceTree = "<group>"; }; > AD2FCB8C1DB5844000B3E736 /* JSWebAssemblyModule.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; name = JSWebAssemblyModule.cpp; path = js/JSWebAssemblyModule.cpp; sourceTree = "<group>"; }; > AD2FCB8D1DB5844000B3E736 /* JSWebAssemblyModule.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = JSWebAssemblyModule.h; path = js/JSWebAssemblyModule.h; sourceTree = "<group>"; }; >@@ -6430,6 +6432,7 @@ > 53F40E8A1D5901BB0099A1B6 /* WasmFunctionParser.h */, > AD8FF3961EB5BD850087FF82 /* WasmIndexOrName.cpp */, > AD8FF3951EB5BD850087FF82 /* WasmIndexOrName.h */, >+ AD00659D1ECAC7FE000CA926 /* WasmLimits.h */, > 53E9E0A91EAE83DE00FEE251 /* WasmMachineThreads.cpp */, > 53E9E0AA1EAE83DE00FEE251 /* WasmMachineThreads.h */, > 535557151D9DFA32006D583B /* WasmMemory.cpp */, >@@ -9469,6 +9472,7 @@ > 0F1FB38F1E173A6700A9BE50 /* SynchronousStopTheWorldMutatorScheduler.h in Headers */, > A784A26411D16622005776AC /* SyntaxChecker.h in Headers */, > DC7997831CDE9FA0004D4A09 /* TagRegistersMode.h in Headers */, >+ AD00659E1ECAC812000CA926 /* WasmLimits.h in Headers */, > 70ECA6081AFDBEA200449739 /* TemplateRegistry.h in Headers */, > 70ECA6091AFDBEA200449739 /* TemplateRegistryKey.h in Headers */, > 95D4261AF4C84CE2ACBAC981 /* TemplateRegistryKeyTable.h in Headers */, >diff --git a/Source/JavaScriptCore/wasm/WasmLimits.h b/Source/JavaScriptCore/wasm/WasmLimits.h >new file mode 100644 >index 0000000..92a6e85 >--- /dev/null >+++ b/Source/JavaScriptCore/wasm/WasmLimits.h >@@ -0,0 +1,55 @@ >+/* >+ * Copyright (C) 2017 Apple Inc. All rights reserved. >+ * >+ * Redistribution and use in source and binary forms, with or without >+ * modification, are permitted provided that the following conditions >+ * are met: >+ * 1. Redistributions of source code must retain the above copyright >+ * notice, this list of conditions and the following disclaimer. >+ * 2. Redistributions in binary form must reproduce the above copyright >+ * notice, this list of conditions and the following disclaimer in the >+ * documentation and/or other materials provided with the distribution. >+ * >+ * THIS SOFTWARE IS PROVIDED BY APPLE INC. ``AS IS'' AND ANY >+ * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE >+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR >+ * PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL APPLE INC. OR >+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, >+ * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, >+ * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR >+ * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY >+ * OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT >+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE >+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. >+ */ >+ >+#pragma once >+ >+#if ENABLE(WEBASSEMBLY) >+ >+#include <cstdint> >+ >+namespace JSC { >+ >+namespace Wasm { >+ >+// These limits are arbitrary except that they match the limits imposed >+// by other browsers' implementation of WebAssembly. It is desirable for >+// us to accept at least the same inputs. >+ >+constexpr size_t maxTypes = 1000000; >+constexpr size_t maxFunctions = 1000000; >+constexpr size_t maxImports = 100000; >+constexpr size_t maxExports = 100000; >+constexpr size_t maxGlobals = 1000000; >+constexpr size_t maxDataSegments = 100000; >+ >+constexpr size_t maxStringSize = 100000; >+constexpr size_t maxModuleSize = 1024 * 1024 * 1024; >+constexpr size_t maxFunctionParams = 1000; >+ >+constexpr size_t maxTableEntries = 10000000; >+ >+} } // namespace JSC::Wasm >+ >+#endif // ENABLE(WEBASSEMBLY) >diff --git a/Source/JavaScriptCore/wasm/WasmModuleParser.cpp b/Source/JavaScriptCore/wasm/WasmModuleParser.cpp >index 7a6a1f5..7d07500 100644 >--- a/Source/JavaScriptCore/wasm/WasmModuleParser.cpp >+++ b/Source/JavaScriptCore/wasm/WasmModuleParser.cpp >@@ -51,6 +51,7 @@ auto ModuleParser::parse() -> Result > uint32_t versionNumber; > > WASM_PARSER_FAIL_IF(length() < minSize, "expected a module of at least ", minSize, " bytes"); >+ WASM_PARSER_FAIL_IF(length() > maxModuleSize, "module size ", length(), " is too large, maximum ", maxModuleSize); > WASM_PARSER_FAIL_IF(!consumeCharacter(0) || !consumeString("asm"), "modules doesn't start with '\\0asm'"); > WASM_PARSER_FAIL_IF(!parseUInt32(versionNumber), "can't parse version number"); > WASM_PARSER_FAIL_IF(versionNumber != expectedVersionNumber, "unexpected version number ", versionNumber, " expected ", expectedVersionNumber); >@@ -102,7 +103,7 @@ auto ModuleParser::parseType() -> PartialResult > uint32_t count; > > WASM_PARSER_FAIL_IF(!parseVarUInt32(count), "can't get Type section's count"); >- WASM_PARSER_FAIL_IF(count == std::numeric_limits<uint32_t>::max(), "Type section's count is too big ", count); >+ WASM_PARSER_FAIL_IF(count > maxTypes, "Type section's count is too big ", count, " maximum ", maxTypes); > WASM_PARSER_FAIL_IF(!m_info->usedSignatures.tryReserveCapacity(count), "can't allocate enough memory for Type section's ", count, " entries"); > > for (uint32_t i = 0; i < count; ++i) { >@@ -113,7 +114,7 @@ auto ModuleParser::parseType() -> PartialResult > WASM_PARSER_FAIL_IF(!parseInt7(type), "can't get ", i, "th Type's type"); > WASM_PARSER_FAIL_IF(type != Func, i, "th Type is non-Func ", type); > WASM_PARSER_FAIL_IF(!parseVarUInt32(argumentCount), "can't get ", i, "th Type's argument count"); >- WASM_PARSER_FAIL_IF(argumentCount == std::numeric_limits<uint32_t>::max(), i, "th argument count is too big ", argumentCount); >+ WASM_PARSER_FAIL_IF(argumentCount > maxFunctionParams, i, "th argument count is too big ", argumentCount, " maximum ", maxFunctionParams); > RefPtr<Signature> maybeSignature = Signature::tryCreate(argumentCount); > WASM_PARSER_FAIL_IF(!maybeSignature, "can't allocate enough memory for Type section's ", i, "th signature"); > Ref<Signature> signature = maybeSignature.releaseNonNull(); >@@ -145,7 +146,7 @@ auto ModuleParser::parseImport() -> PartialResult > { > uint32_t importCount; > WASM_PARSER_FAIL_IF(!parseVarUInt32(importCount), "can't get Import section's count"); >- WASM_PARSER_FAIL_IF(importCount == std::numeric_limits<uint32_t>::max(), "Import section's count is too big ", importCount); >+ WASM_PARSER_FAIL_IF(importCount > maxImports, "Import section's count is too big ", importCount, " maximum ", maxImports); > WASM_PARSER_FAIL_IF(!m_info->globals.tryReserveCapacity(importCount), "can't allocate enough memory for ", importCount, " globals"); // FIXME this over-allocates when we fix the FIXMEs below. > WASM_PARSER_FAIL_IF(!m_info->imports.tryReserveCapacity(importCount), "can't allocate enough memory for ", importCount, " imports"); // FIXME this over-allocates when we fix the FIXMEs below. > WASM_PARSER_FAIL_IF(!m_info->importFunctionSignatureIndices.tryReserveCapacity(importCount), "can't allocate enough memory for ", importCount, " import function signatures"); // FIXME this over-allocates when we fix the FIXMEs below. >@@ -211,7 +212,7 @@ auto ModuleParser::parseFunction() -> PartialResult > { > uint32_t count; > WASM_PARSER_FAIL_IF(!parseVarUInt32(count), "can't get Function section's count"); >- WASM_PARSER_FAIL_IF(count == std::numeric_limits<uint32_t>::max(), "Function section's count is too big ", count); >+ WASM_PARSER_FAIL_IF(count > maxFunctions, "Function section's count is too big ", count, " maximum ", maxFunctions); > WASM_PARSER_FAIL_IF(!m_info->internalFunctionSignatureIndices.tryReserveCapacity(count), "can't allocate enough memory for ", count, " Function signatures"); > WASM_PARSER_FAIL_IF(!m_info->functionLocationInBinary.tryReserveCapacity(count), "can't allocate enough memory for ", count, "Function locations"); > >@@ -334,6 +335,7 @@ auto ModuleParser::parseGlobal() -> PartialResult > { > uint32_t globalCount; > WASM_PARSER_FAIL_IF(!parseVarUInt32(globalCount), "can't get Global section's count"); >+ WASM_PARSER_FAIL_IF(globalCount > maxGlobals, "Global section's count is too big ", globalCount, " maximum ", maxGlobals); > WASM_PARSER_FAIL_IF(!m_info->globals.tryReserveCapacity(globalCount + m_info->firstInternalGlobal), "can't allocate memory for ", globalCount + m_info->firstInternalGlobal, " globals"); > > for (uint32_t globalIndex = 0; globalIndex < globalCount; ++globalIndex) { >@@ -359,7 +361,7 @@ auto ModuleParser::parseExport() -> PartialResult > { > uint32_t exportCount; > WASM_PARSER_FAIL_IF(!parseVarUInt32(exportCount), "can't get Export section's count"); >- WASM_PARSER_FAIL_IF(exportCount == std::numeric_limits<uint32_t>::max(), "Export section's count is too big ", exportCount); >+ WASM_PARSER_FAIL_IF(exportCount > maxExports, "Export section's count is too big ", exportCount, " maximum ", maxExports); > WASM_PARSER_FAIL_IF(!m_info->exports.tryReserveCapacity(exportCount), "can't allocate enough memory for ", exportCount, " exports"); > > HashSet<String> exportNames; >@@ -424,7 +426,7 @@ auto ModuleParser::parseElement() -> PartialResult > > uint32_t elementCount; > WASM_PARSER_FAIL_IF(!parseVarUInt32(elementCount), "can't get Element section's count"); >- WASM_PARSER_FAIL_IF(elementCount == std::numeric_limits<uint32_t>::max(), "Element section's count is too big ", elementCount); >+ WASM_PARSER_FAIL_IF(elementCount > maxTableEntries, "Element section's count is too big ", elementCount, " maximum ", maxTableEntries); > WASM_PARSER_FAIL_IF(!m_info->elements.tryReserveCapacity(elementCount), "can't allocate memory for ", elementCount, " Elements"); > for (unsigned elementNum = 0; elementNum < elementCount; ++elementNum) { > uint32_t tableIndex; >@@ -471,6 +473,7 @@ auto ModuleParser::parseCode() -> PartialResult > WASM_PARSER_FAIL_IF(!parseVarUInt32(functionSize), "can't get ", i, "th Code function's size"); > WASM_PARSER_FAIL_IF(functionSize > length(), "Code function's size ", functionSize, " exceeds the module's size ", length()); > WASM_PARSER_FAIL_IF(functionSize > length() - m_offset, "Code function's size ", functionSize, " exceeds the module's remaining size", length() - m_offset); >+ WASM_PARSER_FAIL_IF(functionSize > std::numeric_limits<uint32_t>::max(), "Code function's size ", functionSize, " is too big"); > > m_info->functionLocationInBinary[i].start = m_offset; > m_info->functionLocationInBinary[i].end = m_offset + functionSize; >@@ -555,7 +558,7 @@ auto ModuleParser::parseData() -> PartialResult > uint32_t segmentCount; > WASM_PARSER_FAIL_IF(!m_info->memory, "Data section cannot exist without a Memory section or Import"); > WASM_PARSER_FAIL_IF(!parseVarUInt32(segmentCount), "can't get Data section's count"); >- WASM_PARSER_FAIL_IF(segmentCount == std::numeric_limits<uint32_t>::max(), "Data section's count is too big ", segmentCount); >+ WASM_PARSER_FAIL_IF(segmentCount > maxDataSegments, "Data section's count is too big ", segmentCount, " maximum ", maxDataSegments); > WASM_PARSER_FAIL_IF(!m_info->data.tryReserveCapacity(segmentCount), "can't allocate enough memory for Data section's ", segmentCount, " segments"); > > for (uint32_t segmentNumber = 0; segmentNumber < segmentCount; ++segmentNumber) { >diff --git a/Source/JavaScriptCore/wasm/WasmParser.h b/Source/JavaScriptCore/wasm/WasmParser.h >index c2a8c14..b8bf0c6 100644 >--- a/Source/JavaScriptCore/wasm/WasmParser.h >+++ b/Source/JavaScriptCore/wasm/WasmParser.h >@@ -30,6 +30,7 @@ > #include "B3Compilation.h" > #include "B3Procedure.h" > #include "WasmFormat.h" >+#include "WasmLimits.h" > #include "WasmModuleInformation.h" > #include "WasmOps.h" > #include "WasmSections.h" >@@ -146,6 +147,8 @@ ALWAYS_INLINE bool Parser<SuccessType>::consumeUTF8String(Name& result, size_t s > { > if (length() < stringLength || m_offset > length() - stringLength) > return false; >+ if (stringLength > maxStringSize) >+ return false; > if (!result.tryReserveCapacity(stringLength)) > return false; > >-- >2.9.3 >

Comment on attachment 310240 [details] patch Rejecting attachment 310240 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 310240, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in Source/JavaScriptCore/ChangeLog contains OOPS!. Full output: http://webkit-queues.webkit.org/results/3749203

Created attachment 310242 [details] patch Fix oops properly.

Comment on attachment 310242 [details] patch Clearing flags on attachment: 310242 Committed r216921: <http://trac.webkit.org/changeset/216921>

All reviewed patches have been landed. Closing bug.

Comment on attachment 310242 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=310242&action=review > Source/JavaScriptCore/wasm/WasmLimits.h:45 > +constexpr size_t maxTypes = 1000000; > +constexpr size_t maxFunctions = 1000000; > +constexpr size_t maxImports = 100000; > +constexpr size_t maxExports = 100000; > +constexpr size_t maxGlobals = 1000000; > +constexpr size_t maxDataSegments = 100000; Some of these don't seem that big, are we sure we want to introduce this artificial limitation?

(In reply to Saam Barati from comment #14) > Comment on attachment 310242 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=310242&action=review > > > Source/JavaScriptCore/wasm/WasmLimits.h:45 > > +constexpr size_t maxTypes = 1000000; > > +constexpr size_t maxFunctions = 1000000; > > +constexpr size_t maxImports = 100000; > > +constexpr size_t maxExports = 100000; > > +constexpr size_t maxGlobals = 1000000; > > +constexpr size_t maxDataSegments = 100000; > > Some of these don't seem that big, are we sure we want to introduce this > artificial limitation? I'm happy to revisit, but we need to reach out to other browser folks who we agreed on these limits with. It's easy to bump them up for them I guess, but presumably they're not hitting the limits?