From 3b47356704bccea0cc92b356426b844ac97c46dd Mon Sep 17 00:00:00 2001 From: Thomas Lively Date: Tue, 17 Mar 2026 16:55:50 -0700 Subject: [PATCH] Fix feature set checking for GC RMWs The GC RMW instructions all used the same pattern of checking whether the expected features were a subset of the expected features. Unfortunately, this pattern was wrong. It looked this this: ``` auto expected = FeatureSet::GC | FeatureSet::Atomics ... shouldBeTrue(expected <= getModule()->features, ... ``` The problem is that the binary operator `|` caused the feature enums to be converted to int, so `expected` ended up being an int. So the `<=` that was supposed to be overloaded to do a subset check on the features was actually checking whether the integer values of the expected feature set was less than the enabled feature set. This incorrect feature checking let the fuzzer use initial contents with the affected instructions without all the expected features being enabled. Later optimizations could replace these instructions with other instructions that also required shared-everything, but checked for it a different way, causing (correct but late) validation errors. Fix the feature validation validation and remove the overloading of <= to eliminiate this class of bugs in the future. --- src/wasm-features.h | 4 +- src/wasm/wasm-validator.cpp | 41 ++++++------- .../lit/validation/struct-atomic-threads.wast | 59 +++++++++++++++++++ 3 files changed, 82 insertions(+), 22 deletions(-) create mode 100644 test/lit/validation/struct-atomic-threads.wast diff --git a/src/wasm-features.h b/src/wasm-features.h index adfa1e1a30b..3ec63ebc7c6 100644 --- a/src/wasm-features.h +++ b/src/wasm-features.h @@ -217,12 +217,12 @@ struct FeatureSet { } } - bool operator<=(const FeatureSet& other) const { + bool isSubsetOf(const FeatureSet& other) const { return !(features & ~other.features); } bool operator==(const FeatureSet& other) const { - return *this <= other && other <= *this; + return isSubsetOf(other) && other.isSubsetOf(*this); } bool operator!=(const FeatureSet& other) const { return !(*this == other); } diff --git a/src/wasm/wasm-validator.cpp b/src/wasm/wasm-validator.cpp index 47fd1de0736..aef68ec1aa0 100644 --- a/src/wasm/wasm-validator.cpp +++ b/src/wasm/wasm-validator.cpp @@ -748,7 +748,7 @@ void FunctionValidator::validatePoppyExpression(Expression* curr) { void FunctionValidator::visitBlock(Block* curr) { auto feats = curr->type.getFeatures(); - if (!shouldBeTrue(feats <= getModule()->features, + if (!shouldBeTrue(feats.isSubsetOf(getModule()->features), curr, "Block type requires additional features")) { getStream() << getMissingFeaturesList(*getModule(), feats) << '\n'; @@ -1133,7 +1133,7 @@ void FunctionValidator::visitCallIndirect(CallIndirect* curr) { } void FunctionValidator::visitConst(Const* curr) { - shouldBeTrue(curr->type.getFeatures() <= getModule()->features, + shouldBeTrue(curr->type.getFeatures().isSubsetOf(getModule()->features), curr, "all used features should be allowed"); } @@ -1592,7 +1592,7 @@ void FunctionValidator::visitSIMDTernary(SIMDTernary* curr) { required |= FeatureSet::SIMD; break; } - if (!shouldBeTrue(required <= getModule()->features, + if (!shouldBeTrue(required.isSubsetOf(getModule()->features), curr, "SIMD ternary operation requires additional features")) { getStream() << getMissingFeaturesList(*getModule(), required) << '\n'; @@ -2099,7 +2099,7 @@ void FunctionValidator::visitBinary(Binary* curr) { case InvalidBinary: WASM_UNREACHABLE("invliad binary op"); } - shouldBeTrue(Features::get(curr->op) <= getModule()->features, + shouldBeTrue(Features::get(curr->op).isSubsetOf(getModule()->features), curr, "all used features should be allowed"); } @@ -2406,7 +2406,7 @@ void FunctionValidator::visitUnary(Unary* curr) { case InvalidUnary: WASM_UNREACHABLE("invalid unary op"); } - shouldBeTrue(Features::get(curr->op) <= getModule()->features, + shouldBeTrue(Features::get(curr->op).isSubsetOf(getModule()->features), curr, "all used features should be allowed"); } @@ -2490,7 +2490,7 @@ void FunctionValidator::visitRefNull(RefNull* curr) { // allow RefNull there as we represent tables that way regardless of what // features are enabled. auto feats = curr->type.getFeatures(); - if (!shouldBeTrue(!getFunction() || feats <= getModule()->features, + if (!shouldBeTrue(!getFunction() || feats.isSubsetOf(getModule()->features), curr, "ref.null requires additional features ")) { getStream() << getMissingFeaturesList(*getModule(), feats) << '\n'; @@ -3490,9 +3490,9 @@ void FunctionValidator::visitStructSet(StructSet* curr) { } void FunctionValidator::visitStructRMW(StructRMW* curr) { - auto expected = + FeatureSet expected = FeatureSet::GC | FeatureSet::Atomics | FeatureSet::SharedEverything; - if (!shouldBeTrue(expected <= getModule()->features, + if (!shouldBeTrue(expected.isSubsetOf(getModule()->features), curr, "struct.atomic.rmw requires additional features ")) { getStream() << getMissingFeaturesList(*getModule(), expected) << '\n'; @@ -3542,9 +3542,9 @@ void FunctionValidator::visitStructRMW(StructRMW* curr) { } void FunctionValidator::visitStructCmpxchg(StructCmpxchg* curr) { - auto expected = + FeatureSet expected = FeatureSet::GC | FeatureSet::Atomics | FeatureSet::SharedEverything; - if (!shouldBeTrue(expected <= getModule()->features, + if (!shouldBeTrue(expected.isSubsetOf(getModule()->features), curr, "struct.atomic.rmw requires additional features ")) { getStream() << getMissingFeaturesList(*getModule(), expected) << '\n'; @@ -3998,9 +3998,9 @@ void FunctionValidator::visitArrayInitElem(ArrayInitElem* curr) { } void FunctionValidator::visitArrayRMW(ArrayRMW* curr) { - auto expected = + FeatureSet expected = FeatureSet::GC | FeatureSet::Atomics | FeatureSet::SharedEverything; - if (!shouldBeTrue(expected <= getModule()->features, + if (!shouldBeTrue(expected.isSubsetOf(getModule()->features), curr, "array.atomic.rmw requires additional features ")) { getStream() << getMissingFeaturesList(*getModule(), expected) << '\n'; @@ -4047,9 +4047,9 @@ void FunctionValidator::visitArrayRMW(ArrayRMW* curr) { } void FunctionValidator::visitArrayCmpxchg(ArrayCmpxchg* curr) { - auto expected = + FeatureSet expected = FeatureSet::GC | FeatureSet::Atomics | FeatureSet::SharedEverything; - if (!shouldBeTrue(expected <= getModule()->features, + if (!shouldBeTrue(expected.isSubsetOf(getModule()->features), curr, "array.atomic.rmw requires additional features ")) { getStream() << getMissingFeaturesList(*getModule(), expected) << '\n'; @@ -4681,7 +4681,7 @@ void FunctionValidator::visitFunction(Function* curr) { for (const auto& var : curr->vars) { features |= var.getFeatures(); } - shouldBeTrue(features <= getModule()->features, + shouldBeTrue(features.isSubsetOf(getModule()->features), curr->name, "all used types should be allowed"); @@ -4958,7 +4958,7 @@ void validateExports(Module& module, ValidationInfo& info) { void validateGlobals(Module& module, ValidationInfo& info) { std::unordered_set seen; ModuleUtils::iterDefinedGlobals(module, [&](Global* curr) { - info.shouldBeTrue(curr->type.getFeatures() <= module.features, + info.shouldBeTrue(curr->type.getFeatures().isSubsetOf(module.features), curr->name, "all used types should be allowed"); info.shouldBeTrue( @@ -4993,7 +4993,8 @@ void validateGlobals(Module& module, ValidationInfo& info) { // Check that globals have allowed types. for (auto& g : module.globals) { auto globalFeats = g->type.getFeatures(); - if (!info.shouldBeTrue(globalFeats <= module.features, g->name, "")) { + if (!info.shouldBeTrue( + globalFeats.isSubsetOf(module.features), g->name, "")) { info.getStream(nullptr) << "global type requires additional features " << getMissingFeaturesList(module, globalFeats) << '\n'; @@ -5129,7 +5130,7 @@ void validateTables(Module& module, ValidationInfo& info) { "Non-nullable reference types are not yet supported for tables"); auto typeFeats = table->type.getFeatures(); if (!info.shouldBeTrue(table->type == funcref || - typeFeats <= module.features, + typeFeats.isSubsetOf(module.features), "table", "table type requires additional features ")) { info.getStream(nullptr) @@ -5152,7 +5153,7 @@ void validateTables(Module& module, ValidationInfo& info) { "Non-nullable reference types are not yet supported for tables"); auto typeFeats = segment->type.getFeatures(); if (!info.shouldBeTrue( - segment->type == funcref || typeFeats <= module.features, + segment->type == funcref || typeFeats.isSubsetOf(module.features), "elem", "element segment type requires additional features ")) { info.getStream(nullptr) @@ -5224,7 +5225,7 @@ void validateTags(Module& module, ValidationInfo& info) { curr->name, "Values in a tag should have concrete types"); } - info.shouldBeTrue(features <= module.features, + info.shouldBeTrue(features.isSubsetOf(module.features), curr->name, "all param types in tags should be allowed"); } diff --git a/test/lit/validation/struct-atomic-threads.wast b/test/lit/validation/struct-atomic-threads.wast new file mode 100644 index 00000000000..37130a62518 --- /dev/null +++ b/test/lit/validation/struct-atomic-threads.wast @@ -0,0 +1,59 @@ +;; RUN: not wasm-opt %s --enable-gc --enable-shared-everything --enable-reference-types --disable-threads 2>&1 | filecheck %s +;; RUN: wasm-opt %s --enable-gc --enable-shared-everything --enable-reference-types --enable-threads + +(module + (type $struct (struct (field (mut i32)))) + (type $array (array (mut i32))) + + (func $struct-cmpxchg (param $ref (ref $struct)) (param $expected i32) (param $replacement i32) + (drop + (struct.atomic.rmw.cmpxchg $struct 0 + (local.get $ref) + (local.get $expected) + (local.get $replacement) + ) + ) + ) + + (func $struct-rmw (param $ref (ref $struct)) (param $value i32) + (drop + (struct.atomic.rmw.add $struct 0 + (local.get $ref) + (local.get $value) + ) + ) + ) + + (func $array-cmpxchg (param $ref (ref $array)) (param $expected i32) (param $replacement i32) + (drop + (array.atomic.rmw.cmpxchg $array + (local.get $ref) + (i32.const 0) + (local.get $expected) + (local.get $replacement) + ) + ) + ) + + (func $array-rmw (param $ref (ref $array)) (param $value i32) + (drop + (array.atomic.rmw.add $array + (local.get $ref) + (i32.const 0) + (local.get $value) + ) + ) + ) +) + +;; CHECK: [wasm-validator error in function struct-cmpxchg] unexpected false: struct.atomic.rmw requires additional features , on +;; CHECK: [--enable-threads] + +;; CHECK: [wasm-validator error in function struct-rmw] unexpected false: struct.atomic.rmw requires additional features , on +;; CHECK: [--enable-threads] + +;; CHECK: [wasm-validator error in function array-cmpxchg] unexpected false: array.atomic.rmw requires additional features , on +;; CHECK: [--enable-threads] + +;; CHECK: [wasm-validator error in function array-rmw] unexpected false: array.atomic.rmw requires additional features , on +;; CHECK: [--enable-threads]