Calling a host function with environment in parallel results in data race

[caelunshun]

Describe the bug

Causing a host function to be called in parallel leads to aliased mutable references to the function's Env. This is a data race and is undefined behavior.

Here's a complete example:

use wasmer::{Cranelift, Function, Instance, Module, Store, Value, JIT};

fn main() {
    let mut compiler_config = Cranelift::new();
    let engine = JIT::new(&mut compiler_config).engine();
    let store = Store::new(&engine);

    let wasm_source = "
    (module
        (type (;0;) (func (param i32) (result i32)))
        (import \"env\" \"host_function\" (func $host_function (type 0)))
        (func $guest_function (type 0) (param i32) (result i32)
          local.get 0
          call $host_function)
        (table (;0;) 1 1 funcref)
        (memory (;0;) 16)
        (global (;0;) (mut i32) (i32.const 1048576))
        (global (;1;) i32 (i32.const 1048576))
        (global (;2;) i32 (i32.const 1048576))
        (export \"memory\" (memory 0))
        (export \"guest_function\" (func $guest_function))
        (export \"__data_end\" (global 1))
        (export \"__heap_base\" (global 2)))
    ";
    let module = Module::new(&store, wasm_source).unwrap();
    let import_object = wasmer::imports! {
        "env" => {
            // host_function has a `u32` environment
            "host_function" => Function::new_native_with_env(&store, 10u32, host_function),
        }
    };
    let instance = Instance::new(&module, &import_object).unwrap();

    // spawn 8 threads to call guest_function in parallel
    let mut threads = Vec::new();
    for _ in 0..8 {
        let instance = instance.clone();
        threads.push(std::thread::spawn(move || {
            for _ in 0..1_000_000 {
                let guest_function = instance.exports.get_function("guest_function").unwrap();
                let _result = guest_function.call(&[Value::I32(1000)]).unwrap();
            }
        }));
    }

    for thread in threads {
        thread.join().unwrap();
    }
}

fn host_function(env: &mut u32, x: u32) -> u32 {
    *env += 1;
    if *env % 1_000_000 == 0 {
        println!("{}", env);
    }
    x
}

Running with wasmer-1.0.0-alpha02.0 and cargo run --release gives me this output on Linux:

1000079
1000131
1000262
1001312
2000104
2000220
3000017
3000307

There's a pretty clear data race here—the correct output would be

1000000
2000000
3000000
4000000
5000000
6000000
7000000
8000000

Steps to reproduce

Run the above example with cargo run --release

Expected behavior

This looks like the API for host function environments needs to be changed so the function doesn't receive a mutable reference to its Env.

[MarkMcCaskey]

Thanks for the report! I've made a PR which keeps the Env mutable by default but synchronizes it.

This fixes the problem but is not an optimal solution, we'll want a &Env version too, probably. And possibly a &mut Env when used in a single threaded context if we can make the trait constraints for that work...

Open to feedback on this, I'll probably start writing up a design doc later today.