Bug 201006 – Wasm's AirIRGenerator::addLocal() and B3IRGenerator::addLocal() are doing unnecessary overflow checks.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

<rdar://problem/52053991>

[Mark Lam]

Created attachment 376946 [details]
proposed patch.

[Keith Miller]

Comment on attachment 376946 [details]
proposed patch.

r=me.

[Mark Lam]

Comment on attachment 376946 [details]
proposed patch.

Actually, this change is not needed.  Details to follow.

[Mark Lam]

Validate::addLocal() can never overflow because:

1. It is only called from m_context.addArguments() and FunctionParser<Context>::parse().

2. m_context.addArguments() will add the number of arguments in the signature passed to it.
   In SectionParser::parseType(), we already ensure that the number of arguments do not exceed maxFunctionParams, which is 1000 (see WasmLimits.h).

3. FunctionParser<Context>::parse() will also call addLocal() to add numberOfLocals for each local group.  numberOfLocals is capped to maxFunctionLocals, and the number of local groups is currently capped to maxFunctionLocals also.  maxFunctionLocals is 50000 (see WasmLimits.h).

As a result, the max possible number of locals added = 1000 + (50000 * 50000) = 2500001000 = 0x9502FCE8, which is less that UINT_MAX.

There is no chance of an overflow here.

[Mark Lam]

Actually, AirIRGenerator::addLocal() and B3IRGenerator::addLocal() are both doing unnecessary overflow checks.  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.

I'll use this bug to change these unnecessary checks to assertions instead.

[Mark Lam]

Created attachment 377483 [details]
proposed patch.

[Yusuke Suzuki]

Comment on attachment 377483 [details]
proposed patch.

r=me. These checks are redundant to WasmFunctionParser and the responsibility of this check should belong to WasmFunctionParser. Not to each client.

[Mark Lam]

Thanks for the review.  Landed in r249221: <http://trac.webkit.org/r249221>.