Cranelift: x64: fix user-controlled recursion in cmp emission. #12333
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We had a set of rules introduced in bytecodealliance#11097 that attempted to optimize the case of testing the result of an `icmp` for a nonzero value. This allowed optimization of, for example, `(((x == 0) == 0) == 0 ...)` to a single level, either `x == 0` or `x != 0` depending on even/odd nesting depth. Unfortunately this kind of recursion in the backend has a depth bounded only by the user input, hence creates a DoS vulnerability: the wrong kind of compiler input can cause a stack overflow in Cranelift at compilation time. This case is reachable from Wasmtime's Wasm frontend via the `i32.eqz` operator (for example) as well. Ideally, this kind of deep rewrite is best done in our mid-end optimizer, where we think carefully about bounds for recursive rewrites. The left-hand sides for the backend rules should really be fixed shapes that correspond to machine instructions, rather than ad-hoc peephole optimizations in their own right. This fix thus simply removes the recursion case that causes the blowup. The patch includes two tests: one with optimizations disabled, showing correct compilation (without the fix, this case fails to compile with a stack overflow), and one with optimizations enabled, showing that the mid-end properly cleans up the nested expression and we get the expected one-level result anyway.
fitzgen
approved these changes
Jan 13, 2026
Member
|
I haven't dug deeply into the fallout here, but it looks like just removing these rules isn't enough to retain the codegen we have from before. A rough hunch is that there's a number of entrypoints for using these ISLE helpers and while the wasm test case here is one which is also cleaned up by the optimizer I think that these rules might be load-bearing for other codegen patterns unrelated to the optimizer. It might need to be the case that some entrypoints use one rule, which includes these deleted rules, but then these rules don't recurse on themselves? |
Member
|
Although, from another angle, this seems fine as a thing to backport since it's clearly such low impact. I'm not aware of these codegen patterns being load bearing in terms of performance so a temporary minor regression while a "full fix" is developed for |
Member
Author
|
This broke some existing lowerings; playing a bit more with various ways to break the cycle. |
Member
Author
|
(On refresh, what Alex said -- somehow those comments didn't appear for me.) |
Member
Author
|
OK, I've reworked things a bit to preserve exactly the same codegen on branches -- I effectively peeled one layer off of |
Member
Author
|
Or, well, spoke too soon -- no Cranelift filetest changes. A few disas tests that do FP compares do regress to materializing the bool. |
48b4f6c to
96a2b7e
Compare
Member
Author
|
OK, I've gotten full equivalence on the disas tests too -- this needed one new mid-end rule to see through a pattern ( @fitzgen mind giving this another look, since it's a little heavier than before? Re: backport, I could certainly backport the rule deletions only, but I'd be a little concerned about performance regressions in that case because this affects many branches. If we feel ok about these rewrites I think the full patch should be applicable back to v36 (LTS). |
Member
|
Also, logistically, @cfallin are you ok running point on backporting/point releases? And 24.0.0, while supported, is unaffected right? |
Member
Author
|
Yes, I'm happy to do the backports too. (Sorry, very slow today taking scraps of time between meetings and other interrupts; can hopefully do backports by end of tomorrow at latest.) |
Member
|
Oh no worries! Just want to make sure there's explicitly an owner. Tomorrow is totally fine and I can be around to hit approvals and help babysit CI. We've had some CI changes recently like trusted publishing which raises the risk of broken CI on other branches, so something to watch out for. |
This change works by splitting a rule so that the entry point used by `brif` lowering can still peel off one layer of `icmp` and emit it directly, without entering the unbounded structural recursion. It also adds a mid-end rule to catch one case that we were previously catching in the backend only: `fcmp(...) != 0`.
96a2b7e to
8453886
Compare
Member
Author
|
OK, this should be good to go -- if r+ here (on new changes) then I can put up backports momentarily. |
alexcrichton
approved these changes
Jan 13, 2026
cfallin
added a commit
to cfallin/wasmtime
that referenced
this pull request
Jan 13, 2026
…odealliance#12333) * Cranelift: x64: fix user-controlled recursion in cmp emission. We had a set of rules introduced in bytecodealliance#11097 that attempted to optimize the case of testing the result of an `icmp` for a nonzero value. This allowed optimization of, for example, `(((x == 0) == 0) == 0 ...)` to a single level, either `x == 0` or `x != 0` depending on even/odd nesting depth. Unfortunately this kind of recursion in the backend has a depth bounded only by the user input, hence creates a DoS vulnerability: the wrong kind of compiler input can cause a stack overflow in Cranelift at compilation time. This case is reachable from Wasmtime's Wasm frontend via the `i32.eqz` operator (for example) as well. Ideally, this kind of deep rewrite is best done in our mid-end optimizer, where we think carefully about bounds for recursive rewrites. The left-hand sides for the backend rules should really be fixed shapes that correspond to machine instructions, rather than ad-hoc peephole optimizations in their own right. This fix thus simply removes the recursion case that causes the blowup. The patch includes two tests: one with optimizations disabled, showing correct compilation (without the fix, this case fails to compile with a stack overflow), and one with optimizations enabled, showing that the mid-end properly cleans up the nested expression and we get the expected one-level result anyway. * Preserve codegen on branches. This change works by splitting a rule so that the entry point used by `brif` lowering can still peel off one layer of `icmp` and emit it directly, without entering the unbounded structural recursion. It also adds a mid-end rule to catch one case that we were previously catching in the backend only: `fcmp(...) != 0`.
cfallin
added a commit
to cfallin/wasmtime
that referenced
this pull request
Jan 13, 2026
…odealliance#12333) * Cranelift: x64: fix user-controlled recursion in cmp emission. We had a set of rules introduced in bytecodealliance#11097 that attempted to optimize the case of testing the result of an `icmp` for a nonzero value. This allowed optimization of, for example, `(((x == 0) == 0) == 0 ...)` to a single level, either `x == 0` or `x != 0` depending on even/odd nesting depth. Unfortunately this kind of recursion in the backend has a depth bounded only by the user input, hence creates a DoS vulnerability: the wrong kind of compiler input can cause a stack overflow in Cranelift at compilation time. This case is reachable from Wasmtime's Wasm frontend via the `i32.eqz` operator (for example) as well. Ideally, this kind of deep rewrite is best done in our mid-end optimizer, where we think carefully about bounds for recursive rewrites. The left-hand sides for the backend rules should really be fixed shapes that correspond to machine instructions, rather than ad-hoc peephole optimizations in their own right. This fix thus simply removes the recursion case that causes the blowup. The patch includes two tests: one with optimizations disabled, showing correct compilation (without the fix, this case fails to compile with a stack overflow), and one with optimizations enabled, showing that the mid-end properly cleans up the nested expression and we get the expected one-level result anyway. * Preserve codegen on branches. This change works by splitting a rule so that the entry point used by `brif` lowering can still peel off one layer of `icmp` and emit it directly, without entering the unbounded structural recursion. It also adds a mid-end rule to catch one case that we were previously catching in the backend only: `fcmp(...) != 0`.
cfallin
added a commit
to cfallin/wasmtime
that referenced
this pull request
Jan 13, 2026
…odealliance#12333) * Cranelift: x64: fix user-controlled recursion in cmp emission. We had a set of rules introduced in bytecodealliance#11097 that attempted to optimize the case of testing the result of an `icmp` for a nonzero value. This allowed optimization of, for example, `(((x == 0) == 0) == 0 ...)` to a single level, either `x == 0` or `x != 0` depending on even/odd nesting depth. Unfortunately this kind of recursion in the backend has a depth bounded only by the user input, hence creates a DoS vulnerability: the wrong kind of compiler input can cause a stack overflow in Cranelift at compilation time. This case is reachable from Wasmtime's Wasm frontend via the `i32.eqz` operator (for example) as well. Ideally, this kind of deep rewrite is best done in our mid-end optimizer, where we think carefully about bounds for recursive rewrites. The left-hand sides for the backend rules should really be fixed shapes that correspond to machine instructions, rather than ad-hoc peephole optimizations in their own right. This fix thus simply removes the recursion case that causes the blowup. The patch includes two tests: one with optimizations disabled, showing correct compilation (without the fix, this case fails to compile with a stack overflow), and one with optimizations enabled, showing that the mid-end properly cleans up the nested expression and we get the expected one-level result anyway. * Preserve codegen on branches. This change works by splitting a rule so that the entry point used by `brif` lowering can still peel off one layer of `icmp` and emit it directly, without entering the unbounded structural recursion. It also adds a mid-end rule to catch one case that we were previously catching in the backend only: `fcmp(...) != 0`.
cfallin
added a commit
to cfallin/wasmtime
that referenced
this pull request
Jan 13, 2026
…odealliance#12333) * Cranelift: x64: fix user-controlled recursion in cmp emission. We had a set of rules introduced in bytecodealliance#11097 that attempted to optimize the case of testing the result of an `icmp` for a nonzero value. This allowed optimization of, for example, `(((x == 0) == 0) == 0 ...)` to a single level, either `x == 0` or `x != 0` depending on even/odd nesting depth. Unfortunately this kind of recursion in the backend has a depth bounded only by the user input, hence creates a DoS vulnerability: the wrong kind of compiler input can cause a stack overflow in Cranelift at compilation time. This case is reachable from Wasmtime's Wasm frontend via the `i32.eqz` operator (for example) as well. Ideally, this kind of deep rewrite is best done in our mid-end optimizer, where we think carefully about bounds for recursive rewrites. The left-hand sides for the backend rules should really be fixed shapes that correspond to machine instructions, rather than ad-hoc peephole optimizations in their own right. This fix thus simply removes the recursion case that causes the blowup. The patch includes two tests: one with optimizations disabled, showing correct compilation (without the fix, this case fails to compile with a stack overflow), and one with optimizations enabled, showing that the mid-end properly cleans up the nested expression and we get the expected one-level result anyway. * Preserve codegen on branches. This change works by splitting a rule so that the entry point used by `brif` lowering can still peel off one layer of `icmp` and emit it directly, without entering the unbounded structural recursion. It also adds a mid-end rule to catch one case that we were previously catching in the backend only: `fcmp(...) != 0`.
cfallin
added a commit
that referenced
this pull request
Jan 14, 2026
… (#12338) * Cranelift: x64: fix user-controlled recursion in cmp emission. We had a set of rules introduced in #11097 that attempted to optimize the case of testing the result of an `icmp` for a nonzero value. This allowed optimization of, for example, `(((x == 0) == 0) == 0 ...)` to a single level, either `x == 0` or `x != 0` depending on even/odd nesting depth. Unfortunately this kind of recursion in the backend has a depth bounded only by the user input, hence creates a DoS vulnerability: the wrong kind of compiler input can cause a stack overflow in Cranelift at compilation time. This case is reachable from Wasmtime's Wasm frontend via the `i32.eqz` operator (for example) as well. Ideally, this kind of deep rewrite is best done in our mid-end optimizer, where we think carefully about bounds for recursive rewrites. The left-hand sides for the backend rules should really be fixed shapes that correspond to machine instructions, rather than ad-hoc peephole optimizations in their own right. This fix thus simply removes the recursion case that causes the blowup. The patch includes two tests: one with optimizations disabled, showing correct compilation (without the fix, this case fails to compile with a stack overflow), and one with optimizations enabled, showing that the mid-end properly cleans up the nested expression and we get the expected one-level result anyway. * Preserve codegen on branches. This change works by splitting a rule so that the entry point used by `brif` lowering can still peel off one layer of `icmp` and emit it directly, without entering the unbounded structural recursion. It also adds a mid-end rule to catch one case that we were previously catching in the backend only: `fcmp(...) != 0`.
cfallin
added a commit
that referenced
this pull request
Jan 14, 2026
… (#12341) * Cranelift: x64: fix user-controlled recursion in cmp emission. We had a set of rules introduced in #11097 that attempted to optimize the case of testing the result of an `icmp` for a nonzero value. This allowed optimization of, for example, `(((x == 0) == 0) == 0 ...)` to a single level, either `x == 0` or `x != 0` depending on even/odd nesting depth. Unfortunately this kind of recursion in the backend has a depth bounded only by the user input, hence creates a DoS vulnerability: the wrong kind of compiler input can cause a stack overflow in Cranelift at compilation time. This case is reachable from Wasmtime's Wasm frontend via the `i32.eqz` operator (for example) as well. Ideally, this kind of deep rewrite is best done in our mid-end optimizer, where we think carefully about bounds for recursive rewrites. The left-hand sides for the backend rules should really be fixed shapes that correspond to machine instructions, rather than ad-hoc peephole optimizations in their own right. This fix thus simply removes the recursion case that causes the blowup. The patch includes two tests: one with optimizations disabled, showing correct compilation (without the fix, this case fails to compile with a stack overflow), and one with optimizations enabled, showing that the mid-end properly cleans up the nested expression and we get the expected one-level result anyway. * Preserve codegen on branches. This change works by splitting a rule so that the entry point used by `brif` lowering can still peel off one layer of `icmp` and emit it directly, without entering the unbounded structural recursion. It also adds a mid-end rule to catch one case that we were previously catching in the backend only: `fcmp(...) != 0`.
cfallin
added a commit
that referenced
this pull request
Jan 14, 2026
… (#12339) * Cranelift: x64: fix user-controlled recursion in cmp emission. We had a set of rules introduced in #11097 that attempted to optimize the case of testing the result of an `icmp` for a nonzero value. This allowed optimization of, for example, `(((x == 0) == 0) == 0 ...)` to a single level, either `x == 0` or `x != 0` depending on even/odd nesting depth. Unfortunately this kind of recursion in the backend has a depth bounded only by the user input, hence creates a DoS vulnerability: the wrong kind of compiler input can cause a stack overflow in Cranelift at compilation time. This case is reachable from Wasmtime's Wasm frontend via the `i32.eqz` operator (for example) as well. Ideally, this kind of deep rewrite is best done in our mid-end optimizer, where we think carefully about bounds for recursive rewrites. The left-hand sides for the backend rules should really be fixed shapes that correspond to machine instructions, rather than ad-hoc peephole optimizations in their own right. This fix thus simply removes the recursion case that causes the blowup. The patch includes two tests: one with optimizations disabled, showing correct compilation (without the fix, this case fails to compile with a stack overflow), and one with optimizations enabled, showing that the mid-end properly cleans up the nested expression and we get the expected one-level result anyway. * Preserve codegen on branches. This change works by splitting a rule so that the entry point used by `brif` lowering can still peel off one layer of `icmp` and emit it directly, without entering the unbounded structural recursion. It also adds a mid-end rule to catch one case that we were previously catching in the backend only: `fcmp(...) != 0`.
cfallin
added a commit
that referenced
this pull request
Jan 14, 2026
… (#12342) * Cranelift: x64: fix user-controlled recursion in cmp emission. We had a set of rules introduced in #11097 that attempted to optimize the case of testing the result of an `icmp` for a nonzero value. This allowed optimization of, for example, `(((x == 0) == 0) == 0 ...)` to a single level, either `x == 0` or `x != 0` depending on even/odd nesting depth. Unfortunately this kind of recursion in the backend has a depth bounded only by the user input, hence creates a DoS vulnerability: the wrong kind of compiler input can cause a stack overflow in Cranelift at compilation time. This case is reachable from Wasmtime's Wasm frontend via the `i32.eqz` operator (for example) as well. Ideally, this kind of deep rewrite is best done in our mid-end optimizer, where we think carefully about bounds for recursive rewrites. The left-hand sides for the backend rules should really be fixed shapes that correspond to machine instructions, rather than ad-hoc peephole optimizations in their own right. This fix thus simply removes the recursion case that causes the blowup. The patch includes two tests: one with optimizations disabled, showing correct compilation (without the fix, this case fails to compile with a stack overflow), and one with optimizations enabled, showing that the mid-end properly cleans up the nested expression and we get the expected one-level result anyway. * Preserve codegen on branches. This change works by splitting a rule so that the entry point used by `brif` lowering can still peel off one layer of `icmp` and emit it directly, without entering the unbounded structural recursion. It also adds a mid-end rule to catch one case that we were previously catching in the backend only: `fcmp(...) != 0`.
Contributor
|
Thanks for taking care of this @cfallin, @alexcrichton, and @fitzgen! |
github-merge-queue bot
pushed a commit
to dfinity/ic
that referenced
this pull request
Jan 21, 2026
This is a patch version bump of wasmtime which contains the backport fix for the compiler bug reported in bytecodealliance/wasmtime#12333 --------- Co-authored-by: IDX GitHub Automation <infra+github-automation@dfinity.org>
github-merge-queue bot
pushed a commit
to dfinity/ic
that referenced
this pull request
Jan 21, 2026
This is a patch version bump of wasmtime which contains the backport fix for the compiler bug reported in bytecodealliance/wasmtime#12333 --------- Co-authored-by: IDX GitHub Automation <infra+github-automation@dfinity.org>
github-merge-queue bot
pushed a commit
to dfinity/ic
that referenced
this pull request
Jan 21, 2026
This is a patch version bump of wasmtime which contains the backport fix for the compiler bug reported in bytecodealliance/wasmtime#12333 --------- Co-authored-by: IDX GitHub Automation <infra+github-automation@dfinity.org>
github-merge-queue bot
pushed a commit
to dfinity/ic
that referenced
this pull request
Jan 21, 2026
This is a patch version bump of wasmtime which contains the backport fix for the compiler bug reported in bytecodealliance/wasmtime#12333 --------- Co-authored-by: IDX GitHub Automation <infra+github-automation@dfinity.org>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We had a set of rules introduced in #11097 that attempted to optimize the case of testing the result of an
icmpfor a nonzero value. This allowed optimization of, for example,(((x == 0) == 0) == 0 ...)to a single level, eitherx == 0orx != 0depending on even/odd nesting depth.Unfortunately this kind of recursion in the backend has a depth bounded only by the user input, hence creates a compiler DoS vulnerability: the wrong kind of compiler input can cause a stack overflow in Cranelift at compilation time. This case is reachable from Wasmtime's Wasm frontend via the
i32.eqzoperator (for example) as well.Ideally, this kind of deep rewrite is best done in our mid-end optimizer, where we think carefully about bounds for recursive rewrites. The left-hand sides for the backend rules should really be fixed shapes that correspond to machine instructions, rather than ad-hoc peephole optimizations in their own right.
This fix thus simply removes the recursion case that causes the blowup. The patch includes two tests: one with optimizations disabled, showing correct compilation (without the fix, this case fails to compile with a stack overflow), and one with optimizations enabled, showing that the mid-end properly cleans up the nested expression and we get the expected one-level result anyway.
Note: this was reported as a security issue by @venkkatesh-sekar (thanks!); per our security policy, compilation DoS is not covered, so this is being fixed in a public PR. I will subsequently make a patch release to our currently supported versions (v36, v39, v40, 41).