Propagate parse errors out of instruction and expression lists - #2824
Open
Nishuuzz wants to merge 1 commit into
Open
Propagate parse errors out of instruction and expression lists#2824Nishuuzz wants to merge 1 commit into
Nishuuzz wants to merge 1 commit into
Conversation
ParseInstrList and ParseExprList synchronise past a bad sub-expression and then return Result::Ok unconditionally, so a field that failed to parse looks to its caller like it succeeded. ParseModuleField therefore never fails, the rollback added in WebAssembly#2805 never runs, and the deferred reference-type resolutions registered by a discarded block are left pointing at freed memory. Pass the error back instead. Both loops still recover and carry on, so every error is still reported; they only remember that something was dropped. This also stops a second, redundant error being reported after the real one, which is why the expected output of a number of tests gets shorter. Diagnosed by @zherczeg on the issue. Fixes WebAssembly#2820.
Collaborator
|
I support this change. @sbc100 what do you think? Do you know what was the reason of the original concept? |
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
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.
wat2wasm,wast2jsonandwat-desugarread freed memory on malformed text input — #2820. The parser holds rawTypeVector*pointers to block signatures inresolve_type_vectors_and only dereferences them once the whole module has parsed, so a block destroyed on an error path in between leaves a dangling entry behind.The diagnosis is @zherczeg's, from the issue thread:
ParseModuleFieldwas returning Ok for a field that had in fact failed. Instrumenting it shows exactly that — the field records 8 errors and still succeeds:ParseInstrListandParseExprListsynchronise past a bad sub-expression and then returnResult::Okunconditionally, so the failure never reachesParseModuleFieldand the rollback added in #2805 never runs.So this just passes the error back. Both loops still recover and carry on, so every error is still reported — they only remember that something was dropped.
ParseModuleFieldthen fails as it should, and #2805's rollback discards the stale resolutions.The test churn
20 expected outputs get shorter, which is most of the diff: 470 lines removed against 43 added.
Previously, having reported the real error, the parser would carry on and emit a second redundant one — nearly always
unexpected token ), expected EOFpointing at the end of the module. That no longer appears:I checked the direction of every one of those diffs: they only ever remove lines. No test gains an error and no valid module starts being rejected. Still, it does touch spec test expectations, so shout if you would rather it were done differently.
Test
test/parse/bad-block-ref-result-discarded.txtis the reproducer from the issue. It is deliberately malformed, and it only aborts under a sanitizer, so realistically it is the asan CI jobs that would catch a regression rather than a normal run.Full suite is green locally against a debug ASAN/UBSAN build with all submodules checked out: 2495 tests, 0 failures.
Fixes #2820.