Skip to content

Commit 8498027

Browse files
kripkenkripken
authored
RefFunc: Validate that the type is non-nullable, and avoid possible bugs in the builder (WebAssembly#3790)
The builder can receive a HeapType so that callers don't need to set non-nullability themselves. Not NFC as some of the callers were in fact still making it nullable.
1 parent 8e20e49 commit 8498027

9 files changed

Lines changed: 23 additions & 27 deletions

File tree

‎src/binaryen-c.cpp‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1283,9 +1283,10 @@ BinaryenExpressionRef BinaryenRefAs(BinaryenModuleRef module,
12831283

12841284
BinaryenExpressionRef
12851285
BinaryenRefFunc(BinaryenModuleRef module, const char* func, BinaryenType type) {
1286+
// TODO: consider changing the C API to receive a heap type
12861287
Type type_(type);
12871288
return static_cast<Expression*>(
1288-
Builder(*(Module*)module).makeRefFunc(func, type_));
1289+
Builder(*(Module*)module).makeRefFunc(func, type_.getHeapType()));
12891290
}
12901291

12911292
BinaryenExpressionRef BinaryenRefEq(BinaryenModuleRef module,
@@ -3485,9 +3486,8 @@ BinaryenAddActiveElementSegment(BinaryenModuleRef module,
34853486
if (func == nullptr) {
34863487
Fatal() << "invalid function '" << funcNames[i] << "'.";
34873488
}
3488-
Type type(HeapType(func->sig), Nullable);
34893489
segment->data.push_back(
3490-
Builder(*(Module*)module).makeRefFunc(funcNames[i], type));
3490+
Builder(*(Module*)module).makeRefFunc(funcNames[i], func->sig));
34913491
}
34923492
return ((Module*)module)->addElementSegment(std::move(segment));
34933493
}
@@ -3503,9 +3503,8 @@ BinaryenAddPassiveElementSegment(BinaryenModuleRef module,
35033503
if (func == nullptr) {
35043504
Fatal() << "invalid function '" << funcNames[i] << "'.";
35053505
}
3506-
Type type(HeapType(func->sig), Nullable);
35073506
segment->data.push_back(
3508-
Builder(*(Module*)module).makeRefFunc(funcNames[i], type));
3507+
Builder(*(Module*)module).makeRefFunc(funcNames[i], func->sig));
35093508
}
35103509
return ((Module*)module)->addElementSegment(std::move(segment));
35113510
}

‎src/ir/module-splitting.cpp‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -105,8 +105,7 @@ template<class F> void forEachElement(Module& module, F f) {
105105

106106
static RefFunc* makeRefFunc(Module& wasm, Function* func) {
107107
// FIXME: make the type NonNullable when we support it!
108-
return Builder(wasm).makeRefFunc(func->name,
109-
Type(HeapType(func->sig), Nullable));
108+
return Builder(wasm).makeRefFunc(func->name, func->sig);
110109
}
111110

112111
struct TableSlotManager {

‎src/ir/table-utils.h‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -87,9 +87,7 @@ inline Index append(Table& table, Name name, Module& wasm) {
8787

8888
auto* func = wasm.getFunctionOrNull(name);
8989
assert(func != nullptr && "Cannot append non-existing function to a table.");
90-
// FIXME: make the type NonNullable when we support it!
91-
auto type = Type(HeapType(func->sig), Nullable);
92-
segment->data.push_back(Builder(wasm).makeRefFunc(name, type));
90+
segment->data.push_back(Builder(wasm).makeRefFunc(name, func->sig));
9391
table.initial = table.initial + 1;
9492
return tableIndex;
9593
}

‎src/tools/fuzzing.h‎

Lines changed: 6 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -738,7 +738,7 @@ class TranslateToFuzzReader {
738738
}
739739
// add some to an elem segment
740740
while (oneIn(3) && !finishedInput) {
741-
auto type = Type(HeapType(func->sig), Nullable);
741+
auto type = Type(HeapType(func->sig), NonNullable);
742742
std::vector<ElementSegment*> compatibleSegments;
743743
ModuleUtils::iterActiveElementSegments(
744744
wasm, [&](ElementSegment* segment) {
@@ -747,8 +747,7 @@ class TranslateToFuzzReader {
747747
}
748748
});
749749
auto& randomElem = compatibleSegments[upTo(compatibleSegments.size())];
750-
// FIXME: make the type NonNullable when we support it!
751-
randomElem->data.push_back(builder.makeRefFunc(func->name, type));
750+
randomElem->data.push_back(builder.makeRefFunc(func->name, func->sig));
752751
}
753752
numAddedFunctions++;
754753
return func;
@@ -1523,10 +1522,9 @@ class TranslateToFuzzReader {
15231522
for (const auto& type : target->sig.params) {
15241523
args.push_back(make(type));
15251524
}
1526-
auto targetType = Type(HeapType(target->sig), NonNullable);
15271525
// TODO: half the time make a completely random item with that type.
15281526
return builder.makeCallRef(
1529-
builder.makeRefFunc(target->name, targetType), args, type, isReturn);
1527+
builder.makeRefFunc(target->name, target->sig), args, type, isReturn);
15301528
}
15311529

15321530
Expression* makeLocalGet(Type type) {
@@ -2115,8 +2113,7 @@ class TranslateToFuzzReader {
21152113
if (!wasm.functions.empty() && !oneIn(wasm.functions.size())) {
21162114
target = pick(wasm.functions).get();
21172115
}
2118-
auto type = Type(HeapType(target->sig), Nullable);
2119-
return builder.makeRefFunc(target->name, type);
2116+
return builder.makeRefFunc(target->name, target->sig);
21202117
}
21212118
if (type == Type::i31ref) {
21222119
return builder.makeI31New(makeConst(Type::i32));
@@ -2138,7 +2135,7 @@ class TranslateToFuzzReader {
21382135
// TODO: randomize the order
21392136
for (auto& func : wasm.functions) {
21402137
if (type == Type(HeapType(func->sig), NonNullable)) {
2141-
return builder.makeRefFunc(func->name, type);
2138+
return builder.makeRefFunc(func->name, func->sig);
21422139
}
21432140
}
21442141
// We failed to find a function, so create a null reference if we can.
@@ -2160,7 +2157,7 @@ class TranslateToFuzzReader {
21602157
sig,
21612158
{},
21622159
builder.makeUnreachable()));
2163-
return builder.makeRefFunc(func->name, type);
2160+
return builder.makeRefFunc(func->name, heapType);
21642161
}
21652162
if (type.isRtt()) {
21662163
return builder.makeRtt(type);

‎src/wasm-builder.h‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -618,10 +618,10 @@ class Builder {
618618
ret->finalize();
619619
return ret;
620620
}
621-
RefFunc* makeRefFunc(Name func, Type type) {
621+
RefFunc* makeRefFunc(Name func, HeapType heapType) {
622622
auto* ret = wasm.allocator.alloc<RefFunc>();
623623
ret->func = func;
624-
ret->finalize(type);
624+
ret->finalize(Type(heapType, NonNullable));
625625
return ret;
626626
}
627627
RefEq* makeRefEq(Expression* left, Expression* right) {
@@ -870,7 +870,7 @@ class Builder {
870870
return makeRefNull(type);
871871
}
872872
if (type.isFunction()) {
873-
return makeRefFunc(value.getFunc(), type);
873+
return makeRefFunc(value.getFunc(), type.getHeapType());
874874
}
875875
if (type.isRtt()) {
876876
return makeRtt(value.type);

‎src/wasm/wasm-binary.cpp‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2856,8 +2856,7 @@ void WasmBinaryBuilder::readElementSegments() {
28562856
Index index = getU32LEB();
28572857
auto sig = getSignatureByFunctionIndex(index);
28582858
// Use a placeholder name for now
2859-
auto* refFunc = Builder(wasm).makeRefFunc(
2860-
Name::fromInt(index), Type(HeapType(sig), Nullable));
2859+
auto* refFunc = Builder(wasm).makeRefFunc(Name::fromInt(index), sig);
28612860
functionRefs[index].push_back(refFunc);
28622861
segmentData.push_back(refFunc);
28632862
}

‎src/wasm/wasm-s-parser.cpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3354,8 +3354,8 @@ ElementSegment* SExpressionWasmBuilder::parseElemFinish(
33543354
} else {
33553355
for (; i < s.size(); i++) {
33563356
auto func = getFunctionName(*s[i]);
3357-
segment->data.push_back(Builder(wasm).makeRefFunc(
3358-
func, Type(HeapType(functionSignatures[func]), Nullable)));
3357+
segment->data.push_back(
3358+
Builder(wasm).makeRefFunc(func, functionSignatures[func]));
33593359
}
33603360
}
33613361
return wasm.addElementSegment(std::move(segment));

‎src/wasm/wasm-validator.cpp‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2001,6 +2001,8 @@ void FunctionValidator::visitRefFunc(RefFunc* curr) {
20012001
shouldBeTrue(curr->type.isFunction(),
20022002
curr,
20032003
"ref.func must have a function reference type");
2004+
shouldBeTrue(
2005+
!curr->type.isNullable(), curr, "ref.func must have non-nullable type");
20042006
// TODO: verify it also has a typed function references type, and the right
20052007
// one,
20062008
// curr->type.getHeapType().getSignature()

‎test/binaryen.js/expressions.js‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1448,7 +1448,9 @@ console.log("# RefFunc");
14481448
assert(theRefFunc instanceof binaryen.RefFunc);
14491449
assert(theRefFunc instanceof binaryen.Expression);
14501450
assert(theRefFunc.func === func);
1451-
assert(theRefFunc.type === binaryen.funcref);
1451+
// TODO: check the type. the type is (ref func), that is, a non-nullable func,
1452+
// which differs from funcref. we don't have the ability to create such
1453+
// a type in the C/JS APIs yet.
14521454

14531455
theRefFunc.func = func = "b";
14541456
assert(theRefFunc.func === func);

0 commit comments

Comments
 (0)