Fix an accidental regression from #697 #708
Merged
Conversation
This file contains 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
This commit fixes a regression introduced in bytecodealliance#697 which could cause a panic when validating an invalid wasm module. The issue introduced was that a [check that the control stack is non-empty][check] was lost in the refactoring of the operator validator. This check ran for every single operator and verified that there was a frame on the control stack that the operator could be attached to, otherwise it means instructions were present after the end of the function. The current design of `VisitOperator` doesn't have an easy place to slot this in so I decided to fix this via a different route than was implemented before. Anything which operates on the control stack now checks to see if it's empty instead of asserting it's non-empty. Operators which don't touch the control stack are then checked by ensuring that the `end` opcode which emptied the control stack was the last operator processed in the function. This exposed a minor issue where when validating const expressions the offset that was passed in as the final offset of the expression was actually the first offset of the expression. Additionally this adds some tests to exercise this corner case (unsure why the spec test suite doesn't have them already!) [check]: https://github.com/bytecodealliance/wasm-tools/blob/8732e0bc8a579cd9f15d9134af997c5d3d95af5d/crates/wasmparser/src/validator/operators.rs#L581-L583
fitzgen
approved these changes
Aug 12, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This commit fixes a regression introduced in #697 which could cause a
panic when validating an invalid wasm module. The issue introduced was
that a check that the control stack is non-empty was lost in
the refactoring of the operator validator. This check ran for every
single operator and verified that there was a frame on the control stack
that the operator could be attached to, otherwise it means instructions
were present after the end of the function.
The current design of
VisitOperatordoesn't have an easy place to slotthis in so I decided to fix this via a different route than was
implemented before. Anything which operates on the control stack now
checks to see if it's empty instead of asserting it's non-empty.
Operators which don't touch the control stack are then checked by
ensuring that the
endopcode which emptied the control stack was thelast operator processed in the function.
This exposed a minor issue where when validating const expressions the
offset that was passed in as the final offset of the expression was
actually the first offset of the expression.
Additionally this adds some tests to exercise this corner case (unsure
why the spec test suite doesn't have them already!)