resolveTableIdx gives wrong function index when using multiple wasm modules

[doehyunbaek]

Repository reproducing the problem: https://github.com/doehyunbaek/wasabi_multimodule_fidx

Currently, resolveTableIdx relies on name property of the function to resolve function index.

wasabi/crates/wasabi/js/runtime.js

Lines 45 to 51 in 21a322b

	// NOTE We want to get the _index_ of the resolved function to the analysis code, but the
	// WebAssembly API only gives us a _function object_.
	// HACK We can abuse the `.name` property of the function object to get the index.
	// See the MDN, which says the "name property is the toString() result of the function's
	// index in the wasm module".
	// https://developer.mozilla.org/en-US/docs/WebAssembly/Exported_functions
	const resolvedFunctionIdx = parseInt(resolvedFunction.name);

This does not work when the function is imported from other WebAssembly instance and the index of the function in its imported module and the index of the function in its declared module is different, e.g.

module1.wat

(module
    (func $private_func_0 (;0;) )
    (func $private_func_1 (;1;) )
    (func $shared_func (;2;) (param $0 i32) (result i32)
        local.get $0
        i32.const 1
        i32.add
    )
    (export "shared_func" (func $shared_func))
)

module2.wat

(module
    (import "env" "shared_func" (func $shared_func (;0;) ))
)

For module2, resolveTableIdx would resolve shared_func to 2, which is different from expected behavior, which is 0.

[danleh]

Thanks, great analysis and very concise reproducer! :) This is indeed a bug.

Besides the multi-module case you layed out here, I'm wondering what will happen for functions that were created with the WebAssembly.Function API (still a proposal, but already implemented in some engines/browsers). I bet it's wrong there too.

How do we solve this? Basically, how to map back from a table entry to a function index in the local module? Even more problematic: Can we even always map back? E.g., what happens if we have a Wasm module that exports its table and the host inserts (via WebAssembly.Table.set) a function that is not even declared in the module (neither as import nor with code)? I guess this is possible host behavior, and in that case we cannot return a meaningful index, just null.

So the proper solution is probably:

• Expose the function reference directly to the hook (i.e., add a func_ref argument to after indirectTableIdx in the call_pre hook), and return targetFunc == undefined if we could not map the reference back to a local function index.
• For those cases, where the call target is a function from another module, but is also properly imported into the local module, we should somehow return the local index. I'm not quite sure what to do, brainstorming ideas:
  a) Compare the function reference against all functions in the module (which we could export as part of the instrumentation). Obvious downside: each call would do O(n) comparisons :(
  b) Maybe we can have a fastpath: We keep a map from indices to function references, look up the reference from the index of the Function object, and if that is the same object, we return the index, otherwise we do the slowpath? I.e., something like

  const func_idx_to_ref = { 0: instance.exports._wasabi_func_0, 1: instance.exports._wasabi_func_1, ... }
  // some helper to get the function index in the call_pre hook:
  let func_ref = table.get(table_index);
  let maybe_correct_func_idx = parseInt(func_ref.name);
  if (func_ref == func_idx_to_ref(maybe_correct_func_idx)) {
    // local function, works:
    return maybe_correct_func_idx;
  } else {
    for (let [idx, ref] of Object.entries(func_idx_to_ref)) {
      if (ref == func_ref) return idx;
    }
    return undefined;
  }

  c) Somehow track the state of the table with a "shadow table" that contains function indices instead of references. Downside: breaks if the host mutates the table (or with Wasm extension that adds table.get/table.set). On the other hand, host code that mutates the table is also wrong today already...

WDYT?

[danleh]

Mhm, maybe even simpler: one can just have a Map from function references to index, and either prefill at initialization time with all references in the table or lazily initialize it with the "slowpath" code from above.

[doehyunbaek]

b) Maybe we can have a fastpath

I like this approach best, will tinker around with this in mind.

Map from function references to index

So it's a funcref --> fidx instead of fidx --> funcref as above? I think this is better, although I'm not sure we can construct such a map having funcref as a key. Will try and update.

[doehyunbaek]

Map from function references to index

This works! I played around with the sola-st/wasm-r3@1004e65 commit and it seems it works well at least for the Wasm-R3's use case.

I want to generalize this solution and upstream to Wasabi but I think it is better to merge any outstanding changes(esp. #41) and build on top of that, WDYT?

[danleh]

Yes, sounds good (merging first, unless fixing this is urgent)