Changeset 249221 in webkit

Timestamp:

Aug 28, 2019, 2:24:47 PM (7 years ago)

Author:

mark.lam@apple.com

Message:

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):

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• wasm/WasmAirIRGenerator.cpp (modified) (1 diff)
• wasm/WasmB3IRGenerator.cpp (modified) (1 diff)
• wasm/WasmValidate.cpp (modified) (1 diff)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+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-28  Keith Rollin  <krollin@apple.com>
 

trunk/Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp

@@ -915 +915 @@
 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) {

trunk/Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp

@@ -659 +659 @@
 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) {

trunk/Source/JavaScriptCore/wasm/WasmValidate.cpp

@@ -250 +250 @@
 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)