Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

reusing a Module may trigger invalid memory access #4949

Open
tzemanovic opened this issue Jul 23, 2024 · 3 comments
Open

reusing a Module may trigger invalid memory access #4949

tzemanovic opened this issue Jul 23, 2024 · 3 comments
Assignees
Labels
bug Something isn't working priority-high High priority issue

Comments

@tzemanovic
Copy link

Describe the bug

Related to #4377 (see comment #4377 (comment)) re-running the same Module that's created with a Store which is dropped after the first run and new Store created for the second run causes a segfault with invalid memory access.

We manage to workaround this issue by keeping a Store that's used to compile cached Modules alive and instantiate a new Store when we want to run them.

wasmer -vV; rustc -vV

Steps to reproduce

I've added a small example that reproduces this here https://github.com/heliaxdev/wasmer/blob/24024841309bff268115ff25cdbb86b1b2db0d52/examples/invalid_memory_access.rs

Run with e.g. cargo run --example invalid-memory-access --features=singlepass

Expected behavior

Module can be re-used for multiple runs

Actual behavior

segfaults on a second loop

@syrusakbary
Copy link
Member

syrusakbary commented Jul 23, 2024

This is indeed a bug. We need to check that modules are run within the same store Engine.

Note that modules should not be reused across different stores Engines.

Edit: I had a typo, sorry for the misdirection here

@maminrayej maminrayej added the bug Something isn't working label Jul 25, 2024
@syrusakbary
Copy link
Member

What's happening is that the engine is different each time. We should error when this happens.

At the same time, something like this should work:

    let compiler = Singlepass::default();
    let engine: Engine = compiler.into();
    for i in 0..2 {
        println!("Iteration {i}...");

        let mut store = Store::new(&engine);
        let module = match &module {
            Some(module) => module.clone(),
            None => {
                module = Some(Module::new(&engine, &wasm_bytes).unwrap());
                module.as_ref().unwrap().clone()
            }
        };
        let import_object = imports! {};

        println!("Instantiating module...");
        // Let's instantiate the Wasm module.
        let instance = Instance::new(&mut store, &module, &import_object)?;

        let sum = instance.exports.get_function("sum")?;

        println!("Calling `sum` function...");
        // Let's call the `sum` exported function. The parameters are a
        // slice of `Value`s. The results are a boxed slice of `Value`s.
        // let _results = sum.call(&mut store, 0_i32, 0_i32);

        if i == 1 {
            println!("The next call will segfault");
        }
        let _results = sum.call(&mut store, &[Value::I32(1), Value::I32(2)]);

        println!("Iteration {i} done.");
        println!();
    }

@theduke
Copy link
Contributor

theduke commented Jul 25, 2024

@tzemanovic to clarify:

A module is not tied to a store, it is just tied to an Engine.

You can re-use a module just fine if you stick with one engine instead.
So in your example, just move the engine out of the loop.

Our code should of course fail if you try to re-use a module with the wrong engine, and I actually thought we had added that.
We might have only added it to cranelift, or the change might have been lost.
We'll treat this as a bug and make sure that correctness check happens.

@maminrayej maminrayej added the priority-high High priority issue label Jul 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working priority-high High priority issue
Projects
None yet
Development

No branches or pull requests

5 participants