Skip to content

Commit ca7ac96

Browse files
tlivelytlively
authored
Fix feature set checking for GC RMWs (#8480)
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.
1 parent 7c4a428 commit ca7ac96

3 files changed

Lines changed: 82 additions & 22 deletions

File tree

‎src/wasm-features.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -217,12 +217,12 @@ struct FeatureSet {
217217
}
218218
}
219219

220-
bool operator<=(const FeatureSet& other) const {
220+
bool isSubsetOf(const FeatureSet& other) const {
221221
return !(features & ~other.features);
222222
}
223223

224224
bool operator==(const FeatureSet& other) const {
225-
return *this <= other && other <= *this;
225+
return isSubsetOf(other) && other.isSubsetOf(*this);
226226
}
227227

228228
bool operator!=(const FeatureSet& other) const { return !(*this == other); }

‎src/wasm/wasm-validator.cpp‎

Lines changed: 21 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -748,7 +748,7 @@ void FunctionValidator::validatePoppyExpression(Expression* curr) {
748748

749749
void FunctionValidator::visitBlock(Block* curr) {
750750
auto feats = curr->type.getFeatures();
751-
if (!shouldBeTrue(feats <= getModule()->features,
751+
if (!shouldBeTrue(feats.isSubsetOf(getModule()->features),
752752
curr,
753753
"Block type requires additional features")) {
754754
getStream() << getMissingFeaturesList(*getModule(), feats) << '\n';
@@ -1133,7 +1133,7 @@ void FunctionValidator::visitCallIndirect(CallIndirect* curr) {
11331133
}
11341134

11351135
void FunctionValidator::visitConst(Const* curr) {
1136-
shouldBeTrue(curr->type.getFeatures() <= getModule()->features,
1136+
shouldBeTrue(curr->type.getFeatures().isSubsetOf(getModule()->features),
11371137
curr,
11381138
"all used features should be allowed");
11391139
}
@@ -1592,7 +1592,7 @@ void FunctionValidator::visitSIMDTernary(SIMDTernary* curr) {
15921592
required |= FeatureSet::SIMD;
15931593
break;
15941594
}
1595-
if (!shouldBeTrue(required <= getModule()->features,
1595+
if (!shouldBeTrue(required.isSubsetOf(getModule()->features),
15961596
curr,
15971597
"SIMD ternary operation requires additional features")) {
15981598
getStream() << getMissingFeaturesList(*getModule(), required) << '\n';
@@ -2099,7 +2099,7 @@ void FunctionValidator::visitBinary(Binary* curr) {
20992099
case InvalidBinary:
21002100
WASM_UNREACHABLE("invliad binary op");
21012101
}
2102-
shouldBeTrue(Features::get(curr->op) <= getModule()->features,
2102+
shouldBeTrue(Features::get(curr->op).isSubsetOf(getModule()->features),
21032103
curr,
21042104
"all used features should be allowed");
21052105
}
@@ -2406,7 +2406,7 @@ void FunctionValidator::visitUnary(Unary* curr) {
24062406
case InvalidUnary:
24072407
WASM_UNREACHABLE("invalid unary op");
24082408
}
2409-
shouldBeTrue(Features::get(curr->op) <= getModule()->features,
2409+
shouldBeTrue(Features::get(curr->op).isSubsetOf(getModule()->features),
24102410
curr,
24112411
"all used features should be allowed");
24122412
}
@@ -2490,7 +2490,7 @@ void FunctionValidator::visitRefNull(RefNull* curr) {
24902490
// allow RefNull there as we represent tables that way regardless of what
24912491
// features are enabled.
24922492
auto feats = curr->type.getFeatures();
2493-
if (!shouldBeTrue(!getFunction() || feats <= getModule()->features,
2493+
if (!shouldBeTrue(!getFunction() || feats.isSubsetOf(getModule()->features),
24942494
curr,
24952495
"ref.null requires additional features ")) {
24962496
getStream() << getMissingFeaturesList(*getModule(), feats) << '\n';
@@ -3490,9 +3490,9 @@ void FunctionValidator::visitStructSet(StructSet* curr) {
34903490
}
34913491

34923492
void FunctionValidator::visitStructRMW(StructRMW* curr) {
3493-
auto expected =
3493+
FeatureSet expected =
34943494
FeatureSet::GC | FeatureSet::Atomics | FeatureSet::SharedEverything;
3495-
if (!shouldBeTrue(expected <= getModule()->features,
3495+
if (!shouldBeTrue(expected.isSubsetOf(getModule()->features),
34963496
curr,
34973497
"struct.atomic.rmw requires additional features ")) {
34983498
getStream() << getMissingFeaturesList(*getModule(), expected) << '\n';
@@ -3542,9 +3542,9 @@ void FunctionValidator::visitStructRMW(StructRMW* curr) {
35423542
}
35433543

35443544
void FunctionValidator::visitStructCmpxchg(StructCmpxchg* curr) {
3545-
auto expected =
3545+
FeatureSet expected =
35463546
FeatureSet::GC | FeatureSet::Atomics | FeatureSet::SharedEverything;
3547-
if (!shouldBeTrue(expected <= getModule()->features,
3547+
if (!shouldBeTrue(expected.isSubsetOf(getModule()->features),
35483548
curr,
35493549
"struct.atomic.rmw requires additional features ")) {
35503550
getStream() << getMissingFeaturesList(*getModule(), expected) << '\n';
@@ -3998,9 +3998,9 @@ void FunctionValidator::visitArrayInitElem(ArrayInitElem* curr) {
39983998
}
39993999

40004000
void FunctionValidator::visitArrayRMW(ArrayRMW* curr) {
4001-
auto expected =
4001+
FeatureSet expected =
40024002
FeatureSet::GC | FeatureSet::Atomics | FeatureSet::SharedEverything;
4003-
if (!shouldBeTrue(expected <= getModule()->features,
4003+
if (!shouldBeTrue(expected.isSubsetOf(getModule()->features),
40044004
curr,
40054005
"array.atomic.rmw requires additional features ")) {
40064006
getStream() << getMissingFeaturesList(*getModule(), expected) << '\n';
@@ -4047,9 +4047,9 @@ void FunctionValidator::visitArrayRMW(ArrayRMW* curr) {
40474047
}
40484048

40494049
void FunctionValidator::visitArrayCmpxchg(ArrayCmpxchg* curr) {
4050-
auto expected =
4050+
FeatureSet expected =
40514051
FeatureSet::GC | FeatureSet::Atomics | FeatureSet::SharedEverything;
4052-
if (!shouldBeTrue(expected <= getModule()->features,
4052+
if (!shouldBeTrue(expected.isSubsetOf(getModule()->features),
40534053
curr,
40544054
"array.atomic.rmw requires additional features ")) {
40554055
getStream() << getMissingFeaturesList(*getModule(), expected) << '\n';
@@ -4681,7 +4681,7 @@ void FunctionValidator::visitFunction(Function* curr) {
46814681
for (const auto& var : curr->vars) {
46824682
features |= var.getFeatures();
46834683
}
4684-
shouldBeTrue(features <= getModule()->features,
4684+
shouldBeTrue(features.isSubsetOf(getModule()->features),
46854685
curr->name,
46864686
"all used types should be allowed");
46874687

@@ -4958,7 +4958,7 @@ void validateExports(Module& module, ValidationInfo& info) {
49584958
void validateGlobals(Module& module, ValidationInfo& info) {
49594959
std::unordered_set<Global*> seen;
49604960
ModuleUtils::iterDefinedGlobals(module, [&](Global* curr) {
4961-
info.shouldBeTrue(curr->type.getFeatures() <= module.features,
4961+
info.shouldBeTrue(curr->type.getFeatures().isSubsetOf(module.features),
49624962
curr->name,
49634963
"all used types should be allowed");
49644964
info.shouldBeTrue(
@@ -4993,7 +4993,8 @@ void validateGlobals(Module& module, ValidationInfo& info) {
49934993
// Check that globals have allowed types.
49944994
for (auto& g : module.globals) {
49954995
auto globalFeats = g->type.getFeatures();
4996-
if (!info.shouldBeTrue(globalFeats <= module.features, g->name, "")) {
4996+
if (!info.shouldBeTrue(
4997+
globalFeats.isSubsetOf(module.features), g->name, "")) {
49974998
info.getStream(nullptr)
49984999
<< "global type requires additional features "
49995000
<< getMissingFeaturesList(module, globalFeats) << '\n';
@@ -5129,7 +5130,7 @@ void validateTables(Module& module, ValidationInfo& info) {
51295130
"Non-nullable reference types are not yet supported for tables");
51305131
auto typeFeats = table->type.getFeatures();
51315132
if (!info.shouldBeTrue(table->type == funcref ||
5132-
typeFeats <= module.features,
5133+
typeFeats.isSubsetOf(module.features),
51335134
"table",
51345135
"table type requires additional features ")) {
51355136
info.getStream(nullptr)
@@ -5152,7 +5153,7 @@ void validateTables(Module& module, ValidationInfo& info) {
51525153
"Non-nullable reference types are not yet supported for tables");
51535154
auto typeFeats = segment->type.getFeatures();
51545155
if (!info.shouldBeTrue(
5155-
segment->type == funcref || typeFeats <= module.features,
5156+
segment->type == funcref || typeFeats.isSubsetOf(module.features),
51565157
"elem",
51575158
"element segment type requires additional features ")) {
51585159
info.getStream(nullptr)
@@ -5224,7 +5225,7 @@ void validateTags(Module& module, ValidationInfo& info) {
52245225
curr->name,
52255226
"Values in a tag should have concrete types");
52265227
}
5227-
info.shouldBeTrue(features <= module.features,
5228+
info.shouldBeTrue(features.isSubsetOf(module.features),
52285229
curr->name,
52295230
"all param types in tags should be allowed");
52305231
}

‎test/lit/validation/struct-atomic-threads.wast‎

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
;; RUN: not wasm-opt %s --enable-gc --enable-shared-everything --enable-reference-types --disable-threads 2>&1 | filecheck %s
2+
;; RUN: wasm-opt %s --enable-gc --enable-shared-everything --enable-reference-types --enable-threads
3+
4+
(module
5+
(type $struct (struct (field (mut i32))))
6+
(type $array (array (mut i32)))
7+
8+
(func $struct-cmpxchg (param $ref (ref $struct)) (param $expected i32) (param $replacement i32)
9+
(drop
10+
(struct.atomic.rmw.cmpxchg $struct 0
11+
(local.get $ref)
12+
(local.get $expected)
13+
(local.get $replacement)
14+
)
15+
)
16+
)
17+
18+
(func $struct-rmw (param $ref (ref $struct)) (param $value i32)
19+
(drop
20+
(struct.atomic.rmw.add $struct 0
21+
(local.get $ref)
22+
(local.get $value)
23+
)
24+
)
25+
)
26+
27+
(func $array-cmpxchg (param $ref (ref $array)) (param $expected i32) (param $replacement i32)
28+
(drop
29+
(array.atomic.rmw.cmpxchg $array
30+
(local.get $ref)
31+
(i32.const 0)
32+
(local.get $expected)
33+
(local.get $replacement)
34+
)
35+
)
36+
)
37+
38+
(func $array-rmw (param $ref (ref $array)) (param $value i32)
39+
(drop
40+
(array.atomic.rmw.add $array
41+
(local.get $ref)
42+
(i32.const 0)
43+
(local.get $value)
44+
)
45+
)
46+
)
47+
)
48+
49+
;; CHECK: [wasm-validator error in function struct-cmpxchg] unexpected false: struct.atomic.rmw requires additional features , on
50+
;; CHECK: [--enable-threads]
51+
52+
;; CHECK: [wasm-validator error in function struct-rmw] unexpected false: struct.atomic.rmw requires additional features , on
53+
;; CHECK: [--enable-threads]
54+
55+
;; CHECK: [wasm-validator error in function array-cmpxchg] unexpected false: array.atomic.rmw requires additional features , on
56+
;; CHECK: [--enable-threads]
57+
58+
;; CHECK: [wasm-validator error in function array-rmw] unexpected false: array.atomic.rmw requires additional features , on
59+
;; CHECK: [--enable-threads]

0 commit comments

Comments
 (0)