Keep instantiations alive on failure to prevent use-after-free when a partially-instantiated module writes a function reference to an imported table (#8111)

Fixes #8108. In linking0.wast, a module writes a function reference to
an imported memory but fails to instantiate completely due to an out of
bounds memory access. In this case, the function reference is expected
to stay alive even though instantiation didn't complete, but our code
currently drops the last reference in this case which causes a segfault.
Change the code to keep the reference alive even when instantiation
fails.

This is pessimistic since some modules may fail to instantiate without
writing any references to imported tables, but it avoids dangling
pointers in those cases that do write references.

Author: stevenfontanella

src/tools/wasm-shell.cpp

@@ -158,12 +158,19 @@ struct Shell {
   }
 
   Result<> instantiate(Module& wasm, Name instanceName) {
-    std::shared_ptr<ShellExternalInterface> interface;
-    std::shared_ptr<ModuleRunner> instance;
+    auto interface = std::make_shared<ShellExternalInterface>(linkedInstances);
+    auto instance =
+      std::make_shared<ModuleRunner>(wasm, interface.get(), linkedInstances);
+
+    lastInstance = instanceName;
+
+    // Even if instantiation fails, the module may have partially instantiated
+    // and mutated an imported memory or table. Keep the references alive to
+    // ensure that function references stay alive.
+    interfaces[instanceName] = interface;
+    instances[instanceName] = instance;
+
     try {
-      interface = std::make_shared<ShellExternalInterface>(linkedInstances);
-      instance =
-        std::make_shared<ModuleRunner>(wasm, interface.get(), linkedInstances);
 
       // This is not an optimization: we want to execute anything, even relaxed
       // SIMD instructions.
@@ -173,10 +180,6 @@ struct Shell {
       return Err{"failed to instantiate module"};
     }
 
-    lastInstance = instanceName;
-
-    interfaces[instanceName] = std::move(interface);
-    instances[instanceName] = std::move(instance);
     return Ok{};
   }
 

test/spec/linking0.wast

@@ -0,0 +1,31 @@
+;; Adapted from test/spec/testsuite/linking0.wast with a failing assertion removed.
+;; TODO: Remove this once test/spec/testsuite/linking0.wast passes.
+
+(module $Mt
+  (type (func (result i32)))
+  (type (func))
+
+  (table (export "tab") 10 funcref)
+  (elem (i32.const 2) $g $g $g $g)
+  (func $g (result i32) (i32.const 4))
+  (func (export "h") (result i32) (i32.const -4))
+
+  (func (export "call") (param i32) (result i32)
+    (call_indirect (type 0) (local.get 0))
+  )
+)
+(register "Mt" $Mt)
+
+(assert_trap
+  (module
+    (table (import "Mt" "tab") 10 funcref)
+    (func $f (result i32) (i32.const 0))
+    (elem (i32.const 7) $f)
+    (memory 0)
+    (memory $m 1)
+    (memory 0)
+    (data $m (i32.const 0x10000) "d")  ;; out of bounds
+  )
+  "out of bounds memory access"
+)
+(assert_return (invoke $Mt "call" (i32.const 7)) (i32.const 0))