Merge r249221 - Wasm's AirIRGenerator::addLocal() and B3IRGenerator::…

…addLocal() are doing unnecessary overflow checks. https://bugs.webkit.org/show_bug.cgi?id=201006 <rdar://problem/52053991> Reviewed by Yusuke Suzuki. We already ensured that it is not possible to overflow in Wasm::FunctionParser's parse(). It is unnecessary and misleading to do those overflow checks in AirIRGenerator and B3IRGenerator. The only check that is necessary is that m_locals.tryReserveCapacity() is successful, otherwise, we have an out of memory situation. This patch changes these unnecessary checks to assertions instead. * wasm/WasmAirIRGenerator.cpp: (JSC::Wasm::AirIRGenerator::addLocal): * wasm/WasmB3IRGenerator.cpp: (JSC::Wasm::B3IRGenerator::addLocal): * wasm/WasmValidate.cpp: (JSC::Wasm::Validate::addLocal):
WebKit · Aug 29, 2019 · 121c8d7 · 121c8d7
1 parent 764bce3
commit 121c8d7
Show file tree

Hide file tree

Showing 4 changed files with 35 additions and 10 deletions.
diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
@@ -1,3 +1,26 @@
+2019-08-28  Mark Lam  <mark.lam@apple.com>
+
+        Wasm's AirIRGenerator::addLocal() and B3IRGenerator::addLocal() are doing unnecessary overflow checks.
+        https://bugs.webkit.org/show_bug.cgi?id=201006
+        <rdar://problem/52053991>
+
+        Reviewed by Yusuke Suzuki.
+
+        We already ensured that it is not possible to overflow in Wasm::FunctionParser's
+        parse().  It is unnecessary and misleading to do those overflow checks in
+        AirIRGenerator and B3IRGenerator.  The only check that is necessary is that
+        m_locals.tryReserveCapacity() is successful, otherwise, we have an out of memory
+        situation.
+
+        This patch changes these unnecessary checks to assertions instead.
+
+        * wasm/WasmAirIRGenerator.cpp:
+        (JSC::Wasm::AirIRGenerator::addLocal):
+        * wasm/WasmB3IRGenerator.cpp:
+        (JSC::Wasm::B3IRGenerator::addLocal):
+        * wasm/WasmValidate.cpp:
+        (JSC::Wasm::Validate::addLocal):
+
 2019-08-27  Michael Saboff  <msaboff@apple.com>
 
         Update PACCage changes for builds without Gigacage, but with signed pointers

diff --git a/Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp b/Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp
@@ -914,10 +914,10 @@ void AirIRGenerator::emitThrowException(CCallHelpers& jit, ExceptionType type)
 
 auto AirIRGenerator::addLocal(Type type, uint32_t count) -> PartialResult
 {
-    Checked<uint32_t, RecordOverflow> totalBytesChecked = count;
-    totalBytesChecked += m_locals.size();
-    uint32_t totalBytes;
-    WASM_COMPILE_FAIL_IF((totalBytesChecked.safeGet(totalBytes) == CheckedState::DidOverflow) || !m_locals.tryReserveCapacity(totalBytes), "can't allocate memory for ", totalBytes, " locals");
+    size_t newSize = m_locals.size() + count;
+    ASSERT(!(CheckedUint32(count) + m_locals.size()).hasOverflowed());
+    ASSERT(newSize <= maxFunctionLocals);
+    WASM_COMPILE_FAIL_IF(!m_locals.tryReserveCapacity(newSize), "can't allocate memory for ", newSize, " locals");
 
     for (uint32_t i = 0; i < count; ++i) {
         auto local = tmpForType(type);

diff --git a/Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp b/Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp
@@ -658,10 +658,10 @@ void B3IRGenerator::insertConstants()
 
 auto B3IRGenerator::addLocal(Type type, uint32_t count) -> PartialResult
 {
-    Checked<uint32_t, RecordOverflow> totalBytesChecked = count;
-    totalBytesChecked += m_locals.size();
-    uint32_t totalBytes;
-    WASM_COMPILE_FAIL_IF((totalBytesChecked.safeGet(totalBytes) == CheckedState::DidOverflow) || !m_locals.tryReserveCapacity(totalBytes), "can't allocate memory for ", totalBytes, " locals");
+    size_t newSize = m_locals.size() + count;
+    ASSERT(!(CheckedUint32(count) + m_locals.size()).hasOverflowed());
+    ASSERT(newSize <= maxFunctionLocals);
+    WASM_COMPILE_FAIL_IF(!m_locals.tryReserveCapacity(newSize), "can't allocate memory for ", newSize, " locals");
 
     for (uint32_t i = 0; i < count; ++i) {
         Variable* local = m_proc.addVariable(toB3Type(type));

diff --git a/Source/JavaScriptCore/wasm/WasmValidate.cpp b/Source/JavaScriptCore/wasm/WasmValidate.cpp
@@ -249,8 +249,10 @@ auto Validate::addRefFunc(uint32_t index, ExpressionType& result) -> Result
 
 auto Validate::addLocal(Type type, uint32_t count) -> Result
 {
-    size_t size = m_locals.size() + count;
-    WASM_VALIDATOR_FAIL_IF(!m_locals.tryReserveCapacity(size), "can't allocate memory for ", size, " locals");
+    size_t newSize = m_locals.size() + count;
+    ASSERT(!(CheckedUint32(count) + m_locals.size()).hasOverflowed());
+    ASSERT(newSize <= maxFunctionLocals);
+    WASM_VALIDATOR_FAIL_IF(!m_locals.tryReserveCapacity(newSize), "can't allocate memory for ", newSize, " locals");
 
     for (uint32_t i = 0; i < count; ++i)
         m_locals.uncheckedAppend(type);