Skip to content

Commit 0887c5f

Browse files
Fix validation errors for struct.wait (#8394)
Prior to #8378, any `assert_invalid` assertions that required a feature to be enabled spuriously succeeded even if the test would otherwise wrongly pass validation. The checks that were in wasm-validator.cpp didn't run if the ref was null or unreachable, which is wrong because the type + field index immediates could still be wrong. Move these assertions to IRBuilder where we have the type immediate available. Verified by removing the assert_invalid parts and checking that the error message matches what's in the test. Part of #8315.
1 parent 88aa87b commit 0887c5f

3 files changed

Lines changed: 18 additions & 21 deletions

File tree

src/wasm/wasm-ir-builder.cpp

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2250,6 +2250,16 @@ Result<> IRBuilder::makeStructWait(HeapType type, Index index) {
22502250
if (!type.isStruct()) {
22512251
return Err{"expected struct type annotation on struct.wait"};
22522252
}
2253+
// This is likely checked in the caller by the `fieldidx` parser.
2254+
if (index >= type.getStruct().fields.size()) {
2255+
return Err{"struct.wait field index out of bounds"};
2256+
}
2257+
2258+
if (type.getStruct().fields.at(index).packedType !=
2259+
Field::PackedType::WaitQueue) {
2260+
return Err{"struct.wait field index must contain a `waitqueue`"};
2261+
}
2262+
22532263
StructWait curr(wasm.allocator);
22542264
curr.index = index;
22552265
CHECK_ERR(ChildPopper{*this}.visitStructWait(&curr, type));

src/wasm/wasm-validator.cpp

Lines changed: 6 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -3517,25 +3517,12 @@ void FunctionValidator::visitStructWait(StructWait* curr) {
35173517
curr,
35183518
"struct.wait timeout must be an i64");
35193519

3520-
if (curr->ref->type == Type::unreachable || curr->ref->type.isNull()) {
3521-
return;
3522-
}
3523-
3524-
// In practice this likely fails during parsing instead.
3525-
if (!shouldBeTrue(curr->index <
3526-
curr->ref->type.getHeapType().getStruct().fields.size(),
3527-
curr,
3528-
"struct.wait index immediate should be less than the field "
3529-
"count of the struct")) {
3530-
return;
3531-
}
3532-
3533-
shouldBeTrue(curr->ref->type.getHeapType()
3534-
.getStruct()
3535-
.fields.at(curr->index)
3536-
.packedType == Field::WaitQueue,
3537-
curr,
3538-
"struct.wait struct field must be a waitqueue");
3520+
// Checks to the ref argument's type are done in IRBuilder where we have the
3521+
// type annotation immediate available. We check that
3522+
// * The reference arg is a subtype of the type immediate
3523+
// * The index immediate is a valid field index of the type immediate (and
3524+
// thus valid for the reference's type too)
3525+
// * The index points to a packed waitqueue field
35393526
}
35403527

35413528
void FunctionValidator::visitArrayNew(ArrayNew* curr) {

test/spec/waitqueue.wast

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
(func (param $expected i32) (param $timeout i64) (result i32)
55
(struct.wait $t 0 (ref.null $t) (local.get $expected) (local.get $timeout))
66
)
7-
) "struct.wait struct field must be a waitqueue"
7+
) "struct.wait struct field index must contain a `waitqueue`"
88
)
99

1010
(assert_invalid
@@ -38,7 +38,7 @@
3838

3939
;; unreachable is allowed
4040
(module
41-
(type $t (struct (field i32)))
41+
(type $t (struct (field waitqueue)))
4242
(func (param $expected i32) (param $timeout i64) (result i32)
4343
(struct.wait $t 0 (unreachable) (local.get $expected) (local.get $timeout))
4444
)

0 commit comments

Comments
 (0)