bytecodealliance / wizer

The WebAssembly Pre-Initializer
Apache License 2.0
939 stars 55 forks source link

Allow reference types as long as not used by the initialization function? #98

Open amesgen opened 7 months ago

amesgen commented 7 months ago

I have a WASM module that uses reference types, but the initialization function (some pure precomputation) doesn't use them. Currently, wizer categorically fails with

Error: reference types support is not enabled (at offset 0x31ef)

However, when I apply this patch

diff --git a/src/lib.rs b/src/lib.rs
index f2ceb94..7f5adc9 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -623,7 +623,7 @@ impl Wizer {
         config.wasm_simd(self.wasm_simd.unwrap_or(DEFAULT_WASM_SIMD));

         // Proposals that we should add support for.
-        config.wasm_reference_types(false);
+        config.wasm_reference_types(true);
         config.wasm_threads(false);

         Ok(config)
@@ -642,7 +642,7 @@ impl Wizer {
             multi_value: self.wasm_multi_value.unwrap_or(DEFAULT_WASM_MULTI_VALUE),

             // Proposals that we should add support for.
-            reference_types: false,
+            reference_types: true,
             simd: self.wasm_simd.unwrap_or(DEFAULT_WASM_SIMD),
             threads: false,
             tail_call: false,

everything works fine, both initialization and later usage of the WASM module (including reference types).

Would it make sense to allow reference types a priori, and instead error when instructions related to reference types are encountered during initialization? I could take a stab at a PR if this makes sense.

fitzgen commented 7 months ago

We do this sort of thing for bulk memory already. We do an extra validation pass over the Wasm and fail if we statically see any table.set instruction or anything like that. We could extend this for reference types as well.

Adding support for doing these checks dynamically is a little more involved. I think we could do this by inserting calls to a new dynamic_fail_wizer_init imported function, or something like that, before each forbidden instruction during the instrumentation phase. That function would always trap, making initialization fail if it was dynamically executed.

tschneidereit commented 7 months ago

Could we perhaps check if there are any live references at the end of wizening? It seems like everything else wouldn't really matter, right?

fitzgen commented 7 months ago

If we limit this to extenrerfs, I think that works, so long as we also record the initialized sizes of externref tables (which would all be full of null elements if we enforce the proposed property). In fact, I don't think we need to even track the live externrefs since there won't be any: wasm can't create an externref, only the host can, and we simply won't.

So basically, I think allowing support for extenrefs is fine because there will only ever be null externrefs. And we can avoid the table-size issue for now by keeping the existing bulk-memory validation that forbids table.grow (or turn it into a dynamic check as described earlier (or just add support for growing externref tables with null references)).

But funcrefs present more problems:

See https://github.com/bytecodealliance/wizer/issues/29 for more ideas about full table support.