Repository navigation
Conversation
Previously, `ShellExternalInterface::init` only checked for 64-bit byte-size shift overflow and allocation failure, allowing modules with initial memories larger than the interpreter's 1 GiB cap (`MaxMemorySize = 1024 * 1024 * 1024`) to instantiate, while `ShellExternalInterface::growMemory` checked `newSize > 1024 * 1024 * 1024` and returned `false` (producing a Wasm `-1` result even for `memory.grow(0)`). When a module with an initial memory exceeding 1 GiB (or growing past 1 GiB while remaining within its declared maximum) was executed under differential fuzzing, the Binaryen interpreter returned `-1` from `memory.grow` while other VMs succeeded.
Check `bytes > MaxMemorySize` in `ShellExternalInterface::init` and call `hostLimit("memory too large")` in both `init` and `growMemory` when the interpreter's 1 GiB allocation cap is exceeded so that `ExecutionResults` and differential fuzzing treat it as a host limit rather than a semantic divergence.
| ;; Growing a memory past the interpreter's 1 GiB cap while remaining within the | ||
| ;; declared maximum size must also signal a host limit rather than returning -1. |
There was a problem hiding this comment.
Marked draft because I'm not convinced this makes sense. @kripken, wdyt?
I guess in fuzzing mode this kind of makes sense because it is non-determinism we want to be able to detect and discard, but for e.g. spec tests this is simply not correct.
There was a problem hiding this comment.
Yeah, I feel the same way, but I think it makes sense. We are basically a fragile interpreter that fails spectacularly in this situation... which is ok I think.
There was a problem hiding this comment.
But let's document that this is for fuzzing purposes, here.
Also maybe add a Precompute test that shows this limitation does not affect Precompute? (I guess it never precomputes growth, so just to confirm that, with a message there, so if we do ever Precompute growth, we'd not forget this)
| // bad_alloc or length_error from the underlying vector: the memory | ||
| // is representable but too large to actually allocate. | ||
| hostLimit("memory too large"); | ||
| } |
There was a problem hiding this comment.
Why remove this try-catch? Don't we want to handle runtime errors?
There was a problem hiding this comment.
I guess the assumption is that if we only allocate 1GB, then we can assume it will always succeed. But the try-catch seems harmless enough; I'll put it back in.
| if (newSize > 1024 * 1024 * 1024) { | ||
| return false; | ||
| if (newSize > MaxMemorySize) { | ||
| hostLimit("memory too large"); |
There was a problem hiding this comment.
Interesting, this is technically not spec-compliant, but I guess it is nice for fuzzing...
Maybe add a comment that this is mainly for fuzzing, and it is here and not in the main interpreter code so it does not affect Precompute?
This branch has not been deployed
2 participants
Previously,
ShellExternalInterface::initonly checked for 64-bit byte-size shift overflow and allocation failure, allowing modules with initial memories larger than the interpreter's 1 GiB cap (MaxMemorySize = 1024 * 1024 * 1024) to instantiate, whileShellExternalInterface::growMemorycheckednewSize > 1024 * 1024 * 1024and returnedfalse(producing a Wasm-1result even formemory.grow(0)). When a module with an initial memory exceeding 1 GiB (or growing past 1 GiB while remaining within its declared maximum) was executed under differential fuzzing, the Binaryen interpreter returned-1frommemory.growwhile other VMs succeeded.Check
bytes > MaxMemorySizeinShellExternalInterface::initand callhostLimit("memory too large")in bothinitandgrowMemorywhen the interpreter's 1 GiB allocation cap is exceeded so thatExecutionResultsand differential fuzzing treat it as a host limit rather than a semantic divergence.