Validate heap types #5279
Validate heap types #5279
Conversation
Validate that GC is enabled if struct and array types are used by the module and validate that both GC and isorecursive types are enabled when nontrivial rec groups are used. This fixes a fuzz bug in #5239 where initial contents included a rec group but the fuzzer disabled GC. Since the resulting module passed validation, the rec groups made it into the binary output, making the type section malformed.
|
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
src/wasm/wasm-validator.cpp
Outdated
| @@ -3461,6 +3461,25 @@ static void validateTags(Module& module, ValidationInfo& info) { | |||
| } | |||
| } | |||
|
|
|||
| static void validateTypes(Module& module, ValidationInfo& info) { | |||
| for (auto type : ModuleUtils::collectHeapTypes(module)) { | |||
There was a problem hiding this comment.
I am somewhat worried about scanning the entire module for this validation - it will basically double validation times, I think.
We already validate GC instructions, which are most of the places GC types can appear. Perhaps we can add validation in the other places, like function params and locals?
There was a problem hiding this comment.
Yeah, maybe. I'd be worried about missing places and duplicating the logic from ModuleUtils::collectHeapTypes (which we've had trouble remembering to update in just a single place), but for better or worse maybe the fuzzer will help find cases where we miss adding validation.
There was a problem hiding this comment.
Yeah, definitely a tradeoff. Not sure what's best.
Can we at least not do the extra pass when GC is not enabled? That is, if GC is not enabled then I'd expect to get lots of other validation errors if the code contains GC types. And the new check added here seems to mostly be important to differentiate things in the context of GC being enabled anyhow (nominal/isorecursive differences).
There was a problem hiding this comment.
Sure, that seems reasonable for now.
There was a problem hiding this comment.
I pushed a better solution just now. PTAL.
| @@ -2903,6 +2903,9 @@ void FunctionValidator::visitFunction(Function* curr) { | |||
| "Multivalue function results (multivalue is not enabled)"); | |||
| } | |||
| FeatureSet features; | |||
| // Check for things like having a rec group with GC enabled. | |||
| features |= | |||
There was a problem hiding this comment.
Why are reference types ignored here?
There was a problem hiding this comment.
Because a function reference type always requires ReferenceTypes as a baseline, but it's kind of a false positive in this situation. MVP functions will have some MVP signature type here, but getFeatures() will still return ReferenceTypes.
In practice the only thing this line checks for is rec groups.
There was a problem hiding this comment.
Ah, right, the type itself is a reference type. A comment perhaps could help?
There was a problem hiding this comment.
Oh oops, I saw this comment just after merging. Will add a comment in a follow-up.
This addresses feedback missed in #5279.
This addresses feedback missed in #5279.
Validate that GC is enabled if struct and array types are used by the module and
validate that both GC and isorecursive types are enabled when nontrivial rec
groups are used.
This fixes a fuzz bug in #5239 where initial contents included a rec group but
the fuzzer disabled GC. Since the resulting module passed validation, the rec
groups made it into the binary output, making the type section malformed.