Skip to content

Commit 201149c

Browse files
stevenfontanellastevenfontanella
authored
Add validations for imports during instantiation (#8086)
* At instantiation time, check the types of exports against their corresponding imports, and trap if they don't match. * Exported memories and tables must be a subtype of the corresponding import. * Function types aren't checked yet because we don't have a way to check the exact type of function re-exports; i.e. [exact-func-imports.wast](https://github.com/WebAssembly/binaryen/blob/main/test/spec/exact-func-import.wast#L85) would fail to instantiate on line 85. * Copy `exact-func-import.wast` from the [testsuite](https://github.com/WebAssembly/testsuite/blob/main/proposals/custom-descriptors/exact-func-import.wast) repo, which now passes. * Fix `ref_func.wast` and `tags.wast` which also otherwise fail with the new validations. * Fix getMemoryInstanceInfo to follow import chains rather than just one import. * Also fix getMemorySize to read the runtime page size from imports when applicable. This is necessary to keep track of page sizes correctly when we import a memory that's later grown e.g. in test/spec/imports.wast. * Fixes `imports0`, `imports2`, `linking0`, `linking3` and partially fixes `imports`, `imports3` and `memory64`. As followups, we should improve the handling of the "spectest" module by implementing the [implicit definition](https://github.com/WebAssembly/spec/blob/main/interpreter/README.md#spectest-host-module) described by the spec, and we should add tracking for exact types of function re-exports so that we can enable import-time type checking for function types.
1 parent 4236103 commit 201149c

File tree

11 files changed

+155
-42
lines changed

11 files changed

+155
-42
lines changed

‎scripts/test/shared.py‎

Lines changed: 5 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -418,15 +418,14 @@ def get_tests(test_dir, extensions=[], recursive=False):
418418
'func.wast', # Duplicate parameter names not properly rejected
419419
'global.wast', # Fail to parse table
420420
'if.wast', # Requires more precise unreachable validation
421-
'imports.wast', # Missing validation of missing function on instantiation
421+
'imports.wast', # Requires fixing handling of mutation to imported globals
422422
'proposals/threads/imports.wast', # Missing memory type validation on instantiation
423423
'linking.wast', # Missing function type validation on instantiation
424424
'proposals/threads/memory.wast', # Missing memory type validation on instantiation
425-
'memory64-imports.wast', # Missing validation on instantiation
426425
'annotations.wast', # String annotations IDs should be allowed
427426
'id.wast', # Empty IDs should be disallowed
428-
# Requires correct handling of tag imports from different instances of the same module,
429-
# ref.null wast constants, and splitting for module instances
427+
# Requires correct handling of tag imports from different instances of the same module
428+
# and splitting for module instances
430429
'instance.wast',
431430
'table64.wast', # Requires validations for table size
432431
'table_grow.wast', # Incorrect table linking semantics in interpreter
@@ -451,12 +450,8 @@ def get_tests(test_dir, extensions=[], recursive=False):
451450
'type-rec.wast', # Missing function type validation on instantiation
452451
'type-subtyping.wast', # ShellExternalInterface::callTable does not handle subtyping
453452
'call_indirect.wast', # Bug with 64-bit inline element segment parsing
454-
'memory64.wast', # Requires validations for memory size
455-
'imports0.wast', # Missing memory type validation on instantiation
456-
'imports2.wast', # Missing memory type validation on instantiation
457-
'imports3.wast', # Missing memory type validation on instantiation
458-
'linking0.wast', # Missing memory type validation on instantiation
459-
'linking3.wast', # Fatal error on missing table.
453+
'memory64.wast', # Requires validations on the max memory size
454+
'imports3.wast', # Requires better checking of exports from the special "spectest" module
460455
'i16x8_relaxed_q15mulr_s.wast', # Requires wast `either` support
461456
'i32x4_relaxed_trunc.wast', # Requires wast `either` support
462457
'i8x16_relaxed_swizzle.wast', # Requires wast `either` support

‎src/ir/memory-utils.cpp‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,11 @@
2020

2121
namespace wasm::MemoryUtils {
2222

23+
bool isSubType(const Memory& a, const Memory& b) {
24+
return a.shared == b.shared && a.addressType == b.addressType &&
25+
a.initial >= b.initial && a.max <= b.max;
26+
}
27+
2328
bool flatten(Module& wasm) {
2429
// If there are no memories then they are already flat, in the empty sense.
2530
if (wasm.memories.empty()) {

‎src/ir/memory-utils.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,8 @@
2828

2929
namespace wasm::MemoryUtils {
3030

31+
bool isSubType(const Memory& a, const Memory& b);
32+
3133
// Flattens memory into a single data segment, or no segment. If there is
3234
// a segment, it starts at 0.
3335
// Returns true if successful (e.g. relocatable segments cannot be flattened).

‎src/ir/table-utils.cpp‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,11 @@
2121

2222
namespace wasm::TableUtils {
2323

24+
bool isSubType(const Table& a, const Table& b) {
25+
return a.addressType == b.addressType && Type::isSubType(a.type, b.type) &&
26+
a.initial >= b.initial && a.max <= b.max;
27+
}
28+
2429
std::set<Name> getFunctionsNeedingElemDeclare(Module& wasm) {
2530
// Without reference types there are no ref.funcs or elem declare.
2631
if (!wasm.features.hasReferenceTypes()) {

‎src/ir/table-utils.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,8 @@
2626

2727
namespace wasm::TableUtils {
2828

29+
bool isSubType(const Table& a, const Table& b);
30+
2931
struct FlatTable {
3032
std::vector<Name> names;
3133
bool valid;

‎src/shell-interface.h‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -134,8 +134,10 @@ struct ShellExternalInterface : ModuleRunner::ExternalInterface {
134134
auto inst = getImportInstance(import);
135135
auto* exportedGlobal = inst->wasm.getExportOrNull(import->base);
136136
if (!exportedGlobal || exportedGlobal->kind != ExternalKind::Global) {
137-
Fatal() << "importGlobals: unknown import: " << import->module.str
138-
<< "." << import->name.str;
137+
trap((std::stringstream()
138+
<< "importGlobals: unknown import: " << import->module.str << "."
139+
<< import->name.str)
140+
.str());
139141
}
140142
globals[import->name] = inst->globals[*exportedGlobal->getInternalName()];
141143
});

‎src/tools/wasm-shell.cpp‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -175,7 +175,9 @@ struct Shell {
175175
// This is not an optimization: we want to execute anything, even relaxed
176176
// SIMD instructions.
177177
instance->setRelaxedBehavior(ModuleRunner::RelaxedBehavior::Execute);
178-
instance->instantiate();
178+
instance->instantiate(/* validateImports_=*/true);
179+
} catch (const std::exception& e) {
180+
return Err{std::string("failed to instantiate module: ") + e.what()};
179181
} catch (...) {
180182
return Err{"failed to instantiate module"};
181183
}

‎src/wasm-interpreter.h‎

Lines changed: 100 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -29,15 +29,18 @@
2929
#define wasm_wasm_interpreter_h
3030

3131
#include <cmath>
32+
#include <iomanip>
3233
#include <limits.h>
3334
#include <sstream>
3435
#include <variant>
3536

3637
#include "fp16.h"
3738
#include "ir/intrinsics.h"
3839
#include "ir/iteration.h"
40+
#include "ir/memory-utils.h"
3941
#include "ir/module-utils.h"
4042
#include "ir/properties.h"
43+
#include "ir/table-utils.h"
4144
#include "support/bits.h"
4245
#include "support/safe_integer.h"
4346
#include "support/stdckdint.h"
@@ -3207,7 +3210,7 @@ class ModuleRunnerBase : public ExpressionRunner<SubType> {
32073210
// Start up this instance. This must be called before doing anything else.
32083211
// (This is separate from the constructor so that it does not occur
32093212
// synchronously, which makes some code patterns harder to write.)
3210-
void instantiate() {
3213+
void instantiate(bool validateImports_ = false) {
32113214
// import globals from the outside
32123215
externalInterface->importGlobals(globals, wasm);
32133216
// generate internal (non-imported) globals
@@ -3218,6 +3221,10 @@ class ModuleRunnerBase : public ExpressionRunner<SubType> {
32183221
// initialize the rest of the external interface
32193222
externalInterface->init(wasm, *self());
32203223

3224+
if (validateImports_) {
3225+
validateImports();
3226+
}
3227+
32213228
initializeTableContents();
32223229
initializeMemoryContents();
32233230

@@ -3309,6 +3316,81 @@ class ModuleRunnerBase : public ExpressionRunner<SubType> {
33093316
Name name;
33103317
};
33113318

3319+
// Validates that the export that provides `importable` exists and has the
3320+
// same kind that the import expects (`kind`).
3321+
void validateImportKindMatches(ExternalKind kind,
3322+
const Importable& importable) {
3323+
auto it = linkedInstances.find(importable.module);
3324+
if (it == linkedInstances.end()) {
3325+
trap((std::stringstream()
3326+
<< "Import module " << std::quoted(importable.module.toString())
3327+
<< " doesn't exist.")
3328+
.str());
3329+
}
3330+
auto* importedInstance = it->second.get();
3331+
3332+
Export* export_ = importedInstance->wasm.getExportOrNull(importable.base);
3333+
3334+
if (!export_) {
3335+
trap((std::stringstream()
3336+
<< "Export " << importable.base << " doesn't exist.")
3337+
.str());
3338+
}
3339+
if (export_->kind != kind) {
3340+
trap((std::stringstream() << "Exported kind: " << export_->kind
3341+
<< " doesn't match expected kind: " << kind)
3342+
.str());
3343+
}
3344+
}
3345+
3346+
// Trap if types don't match between all imports and their corresponding
3347+
// exports. Imported memories and tables must also be a subtype of their
3348+
// export.
3349+
void validateImports() {
3350+
ModuleUtils::iterImportable(
3351+
wasm,
3352+
[this](ExternalKind kind,
3353+
std::variant<Function*, Memory*, Tag*, Global*, Table*> import) {
3354+
Importable* importable = std::visit(
3355+
[](const auto& import) -> Importable* { return import; }, import);
3356+
3357+
// These two modules are injected implicitly to tests. We won't find any
3358+
// import information for them.
3359+
if (importable->module == "binaryen-intrinsics" ||
3360+
(importable->module == "spectest" &&
3361+
importable->base.startsWith("print")) ||
3362+
importable->module == "fuzzing-support") {
3363+
return;
3364+
}
3365+
3366+
validateImportKindMatches(kind, *importable);
3367+
3368+
SubType* importedInstance =
3369+
linkedInstances.at(importable->module).get();
3370+
Export* export_ =
3371+
importedInstance->wasm.getExportOrNull(importable->base);
3372+
3373+
if (auto** memory = std::get_if<Memory*>(&import)) {
3374+
Memory exportedMemory =
3375+
*importedInstance->wasm.getMemory(*export_->getInternalName());
3376+
exportedMemory.initial =
3377+
importedInstance->getMemorySize(*export_->getInternalName());
3378+
3379+
if (!MemoryUtils::isSubType(exportedMemory, **memory)) {
3380+
trap("Imported memory isn't compatible.");
3381+
}
3382+
}
3383+
3384+
if (auto** table = std::get_if<Table*>(&import)) {
3385+
Table* exportedTable =
3386+
importedInstance->wasm.getTable(*export_->getInternalName());
3387+
if (!TableUtils::isSubType(*exportedTable, **table)) {
3388+
trap("Imported table isn't compatible");
3389+
}
3390+
}
3391+
});
3392+
}
3393+
33123394
TableInstanceInfo getTableInstanceInfo(Name name) {
33133395
auto* table = wasm.getTable(name);
33143396
if (table->imported()) {
@@ -3366,12 +3448,16 @@ class ModuleRunnerBase : public ExpressionRunner<SubType> {
33663448
};
33673449

33683450
MemoryInstanceInfo getMemoryInstanceInfo(Name name) {
3369-
auto* memory = wasm.getMemory(name);
3370-
if (memory->imported()) {
3371-
auto& importedInstance = linkedInstances.at(memory->module);
3372-
auto* memoryExport = importedInstance->wasm.getExport(memory->base);
3373-
return importedInstance->getMemoryInstanceInfo(
3374-
*memoryExport->getInternalName());
3451+
auto* instance = self();
3452+
Export* memoryExport = nullptr;
3453+
for (auto* memory = instance->wasm.getMemory(name); memory->imported();
3454+
memory = instance->wasm.getMemory(*memoryExport->getInternalName())) {
3455+
instance = instance->linkedInstances.at(memory->module).get();
3456+
memoryExport = instance->wasm.getExport(memory->base);
3457+
}
3458+
3459+
if (memoryExport) {
3460+
return instance->getMemoryInstanceInfo(*memoryExport->getInternalName());
33753461
}
33763462

33773463
return MemoryInstanceInfo{self(), name};
@@ -3425,6 +3511,11 @@ class ModuleRunnerBase : public ExpressionRunner<SubType> {
34253511
}
34263512

34273513
Address getMemorySize(Name memory) {
3514+
auto info = getMemoryInstanceInfo(memory);
3515+
if (info.instance != self()) {
3516+
return info.instance->getMemorySize(info.name);
3517+
}
3518+
34283519
auto iter = memorySizes.find(memory);
34293520
if (iter == memorySizes.end()) {
34303521
externalInterface->trap("getMemorySize called on non-existing memory");
@@ -3437,7 +3528,7 @@ class ModuleRunnerBase : public ExpressionRunner<SubType> {
34373528
if (iter == memorySizes.end()) {
34383529
externalInterface->trap("setMemorySize called on non-existing memory");
34393530
}
3440-
memorySizes[memory] = size;
3531+
iter->second = size;
34413532
}
34423533

34433534
public:
@@ -4967,7 +5058,7 @@ class ModuleRunnerBase : public ExpressionRunner<SubType> {
49675058
if (lhs > rhs) {
49685059
std::stringstream ss;
49695060
ss << msg << ": " << lhs << " > " << rhs;
4970-
externalInterface->trap(ss.str().c_str());
5061+
externalInterface->trap(ss.str());
49715062
}
49725063
}
49735064

‎test/spec/exact-func-import.wast‎

Lines changed: 20 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1,29 +1,27 @@
11
;; TODO: Use the upstream version from the custom descriptors proposal.
22

3-
(module
3+
(module definition
44
(type $f (func))
55
(import "" "" (func $1 (exact (type 0))))
6-
(import "" "" (func $2 (exact (type $f) (param) (result))))
7-
(import "" "" (func $3 (exact (type $f))))
8-
(import "" "" (func $4 (exact (type 1)))) ;; Implicitly defined next
9-
(import "" "" (func $5 (exact (param i32) (result i64))))
6+
;; (import "" "" (func $2 (exact (type $f) (param) (result)))) ;; TODO: parser support
7+
(import "" "" (func $2 (exact (type $f))))
8+
(import "" "" (func $3 (exact (type 1)))) ;; Implicitly defined next
9+
(import "" "" (func $4 (exact (param i32) (result i64))))
1010

11-
(func $6 (import "" "") (exact (type 0)))
12-
(func $7 (import "" "") (exact (type $f) (param) (result)))
13-
(func $8 (import "" "") (exact (type $f)))
14-
(func $9 (import "" "") (exact (type 2))) ;; Implicitly defined next
15-
(func $10 (import "" "") (exact (param i64) (result i32)))
11+
(func $5 (import "" "") (exact (type 0)))
12+
;; (func $6 (import "" "") (exact (type $f) (param) (result))) ;; TODO: parser support
13+
(func $6 (import "" "") (exact (type $f)))
14+
;; (func $7 (import "" "") (exact (type 2))) ;; Implicitly defined next
15+
;; (func $8 (import "" "") (exact (param i64) (result i32))) ;; TODO: parser support
1616

1717
(global (ref (exact $f)) (ref.func $1))
1818
(global (ref (exact $f)) (ref.func $2))
19-
(global (ref (exact $f)) (ref.func $3))
19+
(global (ref (exact 1)) (ref.func $3))
2020
(global (ref (exact 1)) (ref.func $4))
21-
(global (ref (exact 1)) (ref.func $5))
21+
(global (ref (exact $f)) (ref.func $5))
2222
(global (ref (exact $f)) (ref.func $6))
23-
(global (ref (exact $f)) (ref.func $7))
24-
(global (ref (exact $f)) (ref.func $8))
25-
(global (ref (exact 2)) (ref.func $9))
26-
(global (ref (exact 2)) (ref.func $10))
23+
;; (global (ref (exact 2)) (ref.func $7))
24+
;; (global (ref (exact 2)) (ref.func $8))
2725
)
2826

2927
;; References to inexact imports are not exact.
@@ -51,7 +49,7 @@
5149

5250
;; Inexact imports can still be referenced inexactly, though.
5351

54-
(module
52+
(module definition
5553
(type $f (func))
5654
(import "" "" (func $1 (type $f)))
5755
(global (ref $f) (ref.func $1))
@@ -70,7 +68,9 @@
7068
;; Import and re-export inexactly.
7169
(module $B
7270
(type $f (func))
73-
(func (export "f") (import "A" "f") (type $f))
71+
;; (func (import "A" "f") (export "f") (type $f))
72+
(func (import "A" "f") (type $f))
73+
(export "f" (func 0))
7474
)
7575
(register "B")
7676

@@ -220,7 +220,7 @@
220220
;; Test the binary format
221221

222222
;; Exact function imports use 0x20.
223-
(module binary
223+
(module definition binary
224224
"\00asm" "\01\00\00\00"
225225
"\01" ;; Type section id
226226
"\04" ;; Type section length
@@ -265,4 +265,4 @@
265265
"\0b" ;; End
266266
)
267267
"malformed export kind"
268-
)
268+
)

‎test/spec/ref_func.wast‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,9 @@
1+
;; TODO: use test/spec/testsuite/ref_func.wast once it's fixed.
2+
13
(module
24
(func (export "f") (param $x i32) (result i32) (local.get $x))
35
)
6+
(register "M")
47
(module
58
(func $f (import "M" "f") (param i32) (result i32))
69
(func $g (param $x i32) (result i32) (i32.add (local.get $x) (i32.const 1)))

0 commit comments

Comments
 (0)