Skip to content

[Types] Fix missing inverse check for described field in isValidSupertype#8378

Open
sumleo wants to merge 1 commit intoWebAssembly:mainfrom
sumleo:fix-supertype-described-check
Open

[Types] Fix missing inverse check for described field in isValidSupertype#8378
sumleo wants to merge 1 commit intoWebAssembly:mainfrom
sumleo:fix-supertype-described-check

Conversation

@sumleo
Copy link
Contributor

@sumleo sumleo commented Feb 25, 2026

Summary

  • Add the missing else branch in isValidSupertype for the described field check, making it symmetric with the existing descriptor field check.
  • Without this fix, a type without a describes clause can illegally subtype a type that has a describes clause (i.e., a descriptor type), as long as all features are enabled.
  • The descriptor check already handles both directions (if sub has no descriptor, super must also have no descriptor), but the described check was missing the inverse direction.

Details

In isValidSupertype (src/wasm/wasm-type.cpp), the validation for descriptor is fully symmetric:

if (sub.descriptor) {
    if (super.descriptor && sub.descriptor->supertype != super.descriptor) {
        return false;
    }
} else {
    if (super.descriptor) { return false; }
}

But the described check was missing the else branch:

if (sub.described) {
    if (!super.described || sub.described->supertype != super.described) {
        return false;
    }
}
// Missing: else { if (super.described) { return false; } }

The spec test at test/spec/descriptors.wast (lines 152-161) already covers this case with an assert_invalid for "supertype of non-descriptor type cannot be a descriptor". However, the test currently passes for the wrong reason: wasm-shell re-parses assert_invalid modules as quoted text modules without features enabled, so TypeBuilder::build() rejects the module with RequiresCustomDescriptors before ever reaching isValidSupertype. When the module is loaded through the normal inline parse path (with all features enabled), the invalid subtyping relationship is incorrectly accepted.

Test plan

  • Verified that wasm-shell test/spec/descriptors.wast passes all 14 checks (including the one at line 152)
  • Verified that all 309 GTest unit tests pass

…type

The descriptor field check in isValidSupertype was fully symmetric: if
the subtype has no descriptor, then the supertype must also have no
descriptor. However, the described field check was missing its else
branch, allowing a non-descriptor type to illegally subtype a descriptor
type when features are fully enabled.

Add the missing else branch so that if the subtype has no describes
clause, the supertype must also have no describes clause.
// describes clause.
if (super.described) {
return false;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is not correct, but the spec changed and I'm not sure - @tlively ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is correct! We do not allow this:

(type $foo (descriptor $super) (struct))
(type $super (sub (describes $foo) (struct)))
(type $sub (sub $super) (struct))

@tlively
Copy link
Member

tlively commented Feb 25, 2026

However, the test currently passes for the wrong reason: wasm-shell re-parses assert_invalid modules as quoted text modules without features enabled, so TypeBuilder::build() rejects the module with RequiresCustomDescriptors before ever reaching isValidSupertype.

That seems pretty bad. Would you be able to upload a fix for that as well?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants