Limit instruction nesting depth in the wast parser - #2815
Conversation
The wast parser is recursive descent, so each level of nested instructions costs several stack frames: ParseInstrList -> ParseInstr -> ParseExpr -> ParseBlock -> ParseInstrList Deeply nested but otherwise well-formed text therefore exhausts the stack and crashes instead of producing a diagnostic. A file of ~3000 nested blocks is enough to fault wat2wasm on Windows (0xC00000FD STATUS_STACK_OVERFLOW), and ~2000 is enough under ASAN on Linux. The binary reader already bounds this with BinaryReaderIR's kMaxNestingDepth, but it tracks nesting in an explicit label stack, so its limit can be much higher than a recursive parser can afford. Add the equivalent bound to the text parser, chosen to stay within the smallest default stack we build against (1MB on MSVC). Exceeding the limit is reported once and is not recoverable: without that, the callers resynchronize and walk straight back into the same too-deep input, reporting the same error once per level. On a 200k-deep input that was ~600k diagnostics and 7.8s; it is now 6 lines and 114ms. Fixes WebAssembly#2377.
sbc100
left a comment
There was a problem hiding this comment.
LGTM, I wonder if real world module will blow through this limit or not?
CI showed 1000 was still too high: windows-latest exited 0xC00000FD (STATUS_STACK_OVERFLOW) before the limit was reached, because msvc debug frames are considerably larger than the ones I measured against. Measured cost is ~600 bytes per nesting level for a gcc debug build (a 256KB stack faults between depth 400 and 500), and each level is four frames. Set the limit to 128, which leaves room for msvc debug and is still well clear of real input: across the spec testsuite and wabt's own tests, 18952 modules, the deepest nesting is 80 and the 99th percentile is 3. Also make NestingGuard a struct and have it hold a reference rather than a pointer. It can't be a const reference, since the guard exists to adjust the parser's nesting depth.
|
Thanks for the review. Pushed 505d3fa addressing all three comments — and CI caught a real problem with the original limit, so the value has changed. 1000 was too high. > I wonder if real world module will blow through this limit or not? I measured instead of guessing. Decoding the code section of every module in the spec testsuite plus wabt's own tests — 18952 modules:
Real control flow is very shallow; the only deep thing in the tree is a test written to be deep. The outlier in the wild is the generated code in #2377, which was thousands deep — that one still gets an error, but an error rather than a segfault. So 128 sits ~1.6x above the deepest module I can find anywhere, and roughly 4x under where msvc debug falls over (~600 bytes/level measured on gcc debug, and msvc is worse). If you would rather trade margin for headroom, raising it is a one-line change — I just would not go near 1000 again without also growing the stack, which is what #2748 does. One consequence worth naming: Full test suite is green against a debug ASAN/UBSAN build with all submodules checked out, and |
I regenerated the test for the lower limit but committed it without the STDERR block, so every test job failed on the unexpected output.
| // the default 1MB stack. This leaves room for that and is still well above | ||
| // what real modules use: across the spec testsuite and wabt's own tests | ||
| // (~19k modules) the deepest is 80 and the 99th percentile is 3. | ||
| static constexpr int kMaxNestingDepth = 128; |
There was a problem hiding this comment.
The spec tests are not really real world modules though.
I wonder what kind of nesting can be produced, for example, by wasm-opt on large real-world projects.
I'm also curious of real world engines enforce any kind of nesting limit like this. It looks like a limit is defined in https://www.w3.org/TR/wasm-core-2/#a3-implementation-limitations but I can't see what the value is.
128 was based on the spec testsuite, which turns out not to say much about real code. Measuring modules built by actual toolchains: webp_enc (emscripten) 29 resvg (wasm-pack) 180 sqlite (emscripten) 282 esbuild (go) 3457 Round-tripping sqlite through wasm2wat needs a limit of 320 and resvg needs 192, so 128 would have rejected both. 512 covers them with room and is still half of the 1000 that overflowed msvc debug's 1MB stack. esbuild is beyond anything a recursive parser can do here -- 3457 levels is several MB of frames -- but it could not be read before this change either, it just faulted rather than saying so.
|
You were right to push on that, the spec tests turn out to say almost nothing useful here. I went and measured real toolchain output instead, taking the max control depth out of wasm2wat's own label annotations: So 128 was too low. Round-tripping sqlite through wasm2wat needs a limit of 320 and resvg needs 192, both of which I would have broken. I have raised it to 512, which covers those with some room and is still half of the 1000 that overflowed msvc debug in CI on this PR. esbuild is the one worth dwelling on. At 3457 it is deeper than any build I have survives, so wat2wasm already cannot read wasm2wat's own output for it — it faults on Windows and under ASAN. That is this bug on a real module rather than a synthetic one, which is nice motivation, but it also means no limit can rescue that case: 3457 levels is several MB of frames and the tightest thing we build for is a 1MB stack. All the limit does there is turn the segfault into a message. On the spec link: A3 does list "the nesting depth of structured control instructions" as a dimension implementations may restrict, but it also says up front that where restrictions take the form of numeric limits, "no minimum requirements are given, nor are the limits assumed to be concrete, fixed numbers". So there is deliberately no value in there to copy. Engines do not seem to have one either. V8's wasm-limits.h has the ones wabt already mirrors (function size, 50000 locals, 1000 params/returns, br_table 65520) but nothing for control depth. That follows from their decoders being iterative — nesting costs heap rather than stack, so there is nothing to defend against. It is specifically recursive text parsers that need this. wasm-tools is the closest comparison and they went structural instead: the wast crate deliberately avoids call-stack recursion when parsing expressions, on the grounds that it is parsing user input that risks blowing the stack, and exposes That is probably the honest long-term answer for wabt too, if deeply nested modules should actually parse rather than just fail cleanly. Short of that, the limit is capped by the smallest stack we build for, so I would not go much above 512 without raising the stack as well — which is what #2748 is doing on the Windows side. Happy to pick a different number if you have a preference. |
CI showed 512 still faults on windows: msvc's default stack is 1MB, which is not enough to reach the limit, so deeply nested input dies before the parser can report anything. Picking a limit that does fit in 1MB is not really an option. It would have to be somewhere under 256, and real modules need more than that -- round-tripping sqlite needs 320 -- so it would start rejecting input that parses fine today on Linux and macOS. Asking for the 8MB those platforms already give us instead. This is the same change WebAssembly#2748 makes for the same reason; happy to drop it here if that lands first.
|
CI came back and 512 still faults on windows, so there is a second half to this that I had wrongly assumed was optional. msvc's default stack is 1MB. That is not enough to reach the limit, so deeply nested input dies before the parser gets to say anything — the check never runs. Picking a limit that does fit in 1MB is not really a way out either: it would have to be somewhere under 256, and real modules need more than that, so wat2wasm would start rejecting input that parses fine today on Linux and macOS. Trading a crash on Windows for a regression everywhere else seemed like the wrong deal. So I have added the linker flag to ask for the 8MB stack that Linux and macOS already give us. That is the same change #2748 makes, for the same reason — I have said so in the commit and in the comment next to it. If you would rather land #2748 first I will happily drop it from here and rebase; it is one That does mean the two pieces are less independent than I claimed earlier: the limit stops the recursion running off the end of the stack, but on msvc it needs the bigger stack to be reachable at all. Sorry for the earlier framing, I did not have the Windows data then. For reference, where the numbers landed: 512 covers sqlite (needs 320) and resvg (needs 192) with room, and sits well under the ~13000 levels an 8MB stack allows at the ~600 bytes per level I measured. macOS passed 512 before it got cancelled by the windows failure, so it was only ever msvc that objected. |
| # The wast parser is recursive descent, and msvc's default 1MB stack is | ||
| # not enough to reach WastParser::kMaxNestingDepth, so deeply nested | ||
| # input faults before the parser can report it. Ask for the 8MB that | ||
| # Linux and macOS give us by default. #2748 does the same thing. |
There was a problem hiding this comment.
No need to mention the other PR number here.
|
I think its fine to land the stack size bump as a part of this change since #2748 includes other changes that we still working through |
|
Can you suggest a more concise message to be used for the final squashed commit? |
|
Reference dropped, thanks — and good to know about #2748. For the squash message: |
|
After chatting with @tlively about about it does seems like real world module can have a very deep level nesting dure the br_table instruction. Apparently a swith/case in C can lower to a So I'm not sure what to do here.. maybe the real solution is to refactor to avoid the use of the native stack? But that seems like a lot of work. I'm temped to maybe just do nothing (aside from maybe increate the windows stack size). Is there much point exiting early before full stack exhaustion is reached? |
Problem
Every tool that goes through the wast parser (
wat2wasm,wast2json,wat-desugar) crashes on deeply nested — but otherwise well-formed — text input, instead of reporting an error:0xC00000FD(STATUS_STACK_OVERFLOW)ERROR: AddressSanitizer: stack-overflowThis is #2377, where the reporter hit it on machine-generated WAT containing a long chain of nested
ifs. It needs no unusual input — just depth.Cause
The parser is recursive descent, so each nesting level costs several stack frames:
Measured depth at which
(block …)nesting faults:Every syntactic form that nests is affected: folded
(block …)/(loop …), unfoldedblock … end, folded(if … (then …)), and folded operands such as(i32.eqz (i32.eqz …)).The binary reader is already bounded here —
BinaryReaderIR::kMaxNestingDepth(16384) makeswasm2watreportlabel stack exceeds max nesting depthrather than fault. It can afford a far higher limit because it tracks nesting in an explicit label stack instead of on the C++ stack. So today wabt accepts a 16383-deep module in binary but faults on the equivalent text.Fix
Bound nesting in the text parser as well, using an RAII guard on the two list parsers that every nesting cycle passes through (
ParseInstrListandParseExprList). The limit is 1000, picked to stay inside the smallest default stack we build against (1 MB on MSVC) — hence much lower than the binary reader's.Exceeding it is reported once and is deliberately not recoverable. Without that, the callers resynchronize and walk straight back into the same too-deep input, reporting the error once per level: on the 200k-deep file above that was ~600k diagnostics in 7.8 s, versus 6 lines in 0.11 s now.
Testing
test/parse/expr/bad-nesting-depth.txt(expected output generated withrun-tests.py --rebase).test/run-tests.pysuite run against a Debug + ASAN/UBSAN build, with all submodules checked out.scripts/clang-format-diff.sh mainis clean.Relationship to #2748
#2748 touches the same symptom on Windows by raising the linker stack reserve. The two are complementary rather than competing: a bigger stack raises the threshold, while this bounds the recursion so the tools report an error instead of faulting on every platform and build configuration.
The constant is a judgement call — happy to change the value, or to move the guard, if you'd prefer it somewhere else.
Fixes #2377.