Segfault when running linking0.wast

[stevenfontanella]

I ran an adapted version of linking0.wast (removing some assertions in the middle that aren't needed) from the current main with ./binaryen/bin/wasm-shell test/spec/testsuite/linking0.wast:

(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))

Running this gives a segfault during the last assert_return statement: backtrace.

This test is basically testing that a partially-instantiated module that fails to instantiate can still mutate an imported table. If we remove the error that causes it to fail to instantiate, then the test 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)

;; Now instantiation completes successfully
;; (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))

I suspect that when the module instantiation fails, some memory gets freed that is still referenced in the table of the imported function which causes the segfault. Strangely though, the backtrace doesn't confirm this. Instead the backtrace seems to show that getCurrContinuationOrNull() is returning a garbage value (continuations is empty but we return continuations.back()):

(lldb) f 6
frame #6: 0x0000555555657a57 wasm-shell`wasm::ExpressionRunner<wasm::ModuleRunner>::getCurrContinuationOrNull(this=0x0000555555756900) at wasm-interpreter.h:451:50
   448      if (!continuationStore || continuationStore->continuations.empty()) {
   449        return {};
   450      }
-> 451      return continuationStore->continuations.back();
   452    }
   453
   454    std::shared_ptr<ContData> getCurrContinuation() {
(lldb) p continuationStore->continuations
(std::vector<std::shared_ptr<wasm::ContData> >) size=0 {}
(lldb) p !continuationStore || continuationStore->continuations.empty()
(bool) false

Based on this frame, it seems like continuationStore was mutated by another thread in between the check and returning continuations.back(), but I'm not sure if we even have concurrent threads here and this doesn't make as much sense in context of the test.

[stevenfontanella]

Looks like the issue is that the lambda in makeFuncData captures this but outlives it in the case that it's written to another instance's table and the module that the function comes from fails to instantiate. Will send a fix soon.

[redianthus]

This test is basically testing that a partially-instantiated module that fails to instantiate can still mutate an imported table. If we remove the error that causes it to fail to instantiate, then the test passes:

The test is more subtle than that. Actually it is also testing the evaluation order of active data and active elements. A VM should first evaluate the active elem and then the active data. If it does the contrary, then, it will stop evaluating after the out of bounds in the active data, without updating the table with the active element, which will make the next assertion fail!

[stevenfontanella]

Right, I guess you're saying that the test is about the evaluation order of modules. In this case it looks like we're evaluating it in the correct order since the mentioned test now passes after #8111 (if we weren't evaluating in the correct order, then the module instantiation would fail before we ever write to the table). In general, I can't say for sure whether we implement the correct evaluation order since I haven't seen that code yet. From a quick look, I'm not aware of any failing spec tests related to the module evaluation order.

[tlively]

It might be an interesting exercise to compare our instantiation code with the spec for instantiation, even if all the tests are passing. I would easily believe that this is under-tested in the test suite.