[naga wgsl-in] Return error if wgsl parser recurses too deeply. #6885
Conversation
| self.recursion_depth += 1; | ||
| if self.recursion_depth >= 256 { |
There was a problem hiding this comment.
thought: We already have some recursion tracking for curly braces. I wonder how they might be reconciled.
There was a problem hiding this comment.
That is definitely something to consider, but I would like to treat it as a follow-up. We have particular reasons that #5757 is urgent to close despite being not very important to users, so we want to put this behind us quickly.
There was a problem hiding this comment.
I agree; generally, thoughts I offer are not blocking, so 👍🏻 if it's a problem, weeee'll figure it out!
There was a problem hiding this comment.
Okay, right - I should have checked. Thanks for the clarification.
WGSL explicitly allows us to impose ad-hoc limits like this:
|
|
And for reference Tint also imposes a similar limit (though tracking recursion at different places and a different value for the limit) |
| self.recursion_depth += 1; | ||
| if self.recursion_depth >= 256 { |
There was a problem hiding this comment.
Okay, right - I should have checked. Thanks for the clarification.
It's currently trivial to write a shader that causes the wgsl parser to recurse too deeply and overflow the stack. This patch makes the parser return an error when recursing too deeply, before the stack overflows. It makes use of a new function Parser::track_recursion(). This increments a counter returning an error if the value is too high, before calling the user-provided function and returning its return value after decrementing the counter again. Any recursively-called functions can simply be modified to call track_recursion(), providing their previous contents in a closure as the argument. All instances of recursion during parsing call through either Parser::statement(), Parser::unary_expression(), or Parser::type_decl(), so only these functions have been updated as described in order to keep the patch as unobtrusive as possible. A value of 256 has been chosen as the recursion limit, but can be later tweaked if required. This avoids the stack overflow in the testcase attached to issue gfx-rs#5757.
jamienicol
force-pushed
the
stack-overflow
branch
from
c8bda92 to
688c06b
Compare
Connections
Fixes #5757
Description
It's currently trivial to write a shader that causes the wgsl parser to recurse too deeply and overflow the stack. This patch makes the parser return an error when recursing too deeply, before the stack overflows.
It makes use of a new function Parser::track_recursion(). This increments a counter returning an error if the value is too high, before calling the user-provided function and returning its return value after decrementing the counter again.
Any recursively-called functions can simply be modified to call track_recursion(), providing their previous contents in a closure as the argument. All instances of recursion during parsing call through either Parser::statement(), Parser::unary_expression(), or Parser::type_decl(), so only these functions have been updated as described in order to keep the patch as unobtrusive as possible.
A value of 256 has been chosen as the recursion limit, but can be later tweaked if required. This avoids the stack overflow in the testcase attached to issue #5757.
Testing
Ran test suite
Checklist
cargo fmt.taplo format.cargo clippy. If applicable, add:--target wasm32-unknown-unknown--target wasm32-unknown-emscriptencargo xtask testto run tests.CHANGELOG.md. See simple instructions inside file.