WasmEdge / wasmedge-rust-sdk

Embed WasmEdge functions in a Rust host app
Apache License 2.0
30 stars 15 forks source link

Bug: Undefined Behavior When Registering Import #77

Closed StaticallyTypedAnxiety closed 1 year ago

StaticallyTypedAnxiety commented 1 year ago

Using the new wasmEdge-sdk for rust I am getting some undefined behavior when loading my particular module. Version: 13.4 wasmedge-sdk:0.12.2.

Case1:

behavior: instantiation failed: unknown import, Code: 0x62 expected: VM should be created and registered with the import module

Case2:

behavior: Segmentation fault expected: VM should be created and registered with the import module

behavior was working in previous versions of wasmedge-sdk

Platform WSL1 OS: Ubuntu 22.04.2 LTS

L-jasmine commented 1 year ago

@AshantiM Could you provide js_interpreter.wasm to help us identify the issue?

L-jasmine commented 1 year ago

@AshantiM I have obtained the js_interpreter.wasm. Inside, it imports a non-WASI function functiongraph_host:log. So, case1 is fine. However, case2 shouldn't be happening. Could you provide the Rust code for case2?

StaticallyTypedAnxiety commented 1 year ago

@AshantiM I have obtained the js_interpreter.wasm. Inside, it imports a non-WASI function functiongraph_host:log. So, case1 is fine. However, case2 shouldn't be happening. Could you provide the Rust code for case2?

Absolutely

`

[host_function]

fn my_add(_caller: Caller, input: Vec) -> Result<Vec, HostFuncError> {

Ok(vec![WasmValue::from_i32(c)])

}

fn main() -> Result<(), Box> { let args: Vec = std::env::args().collect(); println!("args: {:?}", args);

let wasm_lib_file = &args[1];

// create an import module
let import = ImportObjectBuilder::new()
    .with_func::<i32, (), NeverType>("log", my_add, None)?
    .build::<NeverType>("functiongraph_host", None)?;

// create a new Vm with default config
let config = ConfigBuilder::new(CommonConfigOptions::default())
    .with_host_registration_config(HostRegistrationConfigOptions::default().wasi(true))
    .build()?;
let wasi_context = WasiContext::new(None, None, None);

let mut vm = VmBuilder::new()
.with_config(config)
.with_wasi_context(wasi_context)
.build()?;
vm.register_import_module(&import)?;
let module = Module::from_file(None,&wasm_lib_file)?;
let res = vm
     .register_module(None,module)?;
Ok(())

} `

For case1 I get an unknown import even though I have registered my host function against it. It was working in previous versions of wasmedge-sdk not sure if maybe I missed something?

L-jasmine commented 1 year ago

@AshantiM I attempted to compile and run the source code you provided. Initially, I encountered a compilation error because the variable c in the my_add function was undefined. From the line with_func::<i32, (), NeverType>("log", my_add, None)?, I observed that "log" is a function with no return value. So, I made a fix to my_add by having it return Ok(vec![]). However, I was not able to reproduce any of the cases.

I noticed that it didn't execute any functions within the wasm. Could this affect the issue reproduction?

L-jasmine commented 1 year ago

Platform WSL2 OS: Ubuntu 20.04.1 LTS

[package]
name = "wasmedgex"
version = "0.1.0"
edition = "2021"

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]
wasmedge-sdk = "0.12.2"
use wasmedge_sdk::{
    config::{CommonConfigOptions, ConfigBuilder, HostRegistrationConfigOptions},
    error::HostFuncError,
    host_function, Caller, ImportObjectBuilder, Module, NeverType, VmBuilder, WasmValue,
};

#[host_function]
fn my_add(_caller: Caller, input: Vec<WasmValue>) -> Result<Vec<WasmValue>, HostFuncError> {
    Ok(vec![])
}

type Type = NeverType;

fn main() -> Result<(), Box<dyn std::error::Error>> {
    let args: Vec<String> = std::env::args().collect();
    println!("args: {:?}", args);

    let wasm_lib_file = &args[1];

    // create an import module
    let import = ImportObjectBuilder::new()
        .with_func::<i32, (), NeverType>("log", my_add, None)?
        .build::<Type>("functiongraph_host", None)?;

    // create a new Vm with default config
    let config = ConfigBuilder::new(CommonConfigOptions::default())
        .with_host_registration_config(HostRegistrationConfigOptions::default().wasi(true))
        .build()?;

    let mut vm = VmBuilder::new().with_config(config).build()?;
    if let Some(wasi) = vm.wasi_module_mut() {
        wasi.initialize(None, None, None);
    }
    vm.register_import_module(&import)?;
    let module = Module::from_file(None, &wasm_lib_file)?;
    let vm = vm.register_module(None, module)?;
    Ok(())
}
➜  wasmedgex git:(master) ✗ cargo run js_interpreter.wasm
   Compiling wasmedgex v0.1.0 (/home/csh/workspace/hw/wasmedgex)
warning: unused variable: `input`
 --> src/main.rs:8:28
  |
8 | fn my_add(_caller: Caller, input: Vec<WasmValue>) -> Result<Vec<WasmValue>, HostFuncError> {
  |                            ^^^^^ help: if this is intentional, prefix it with an underscore: `_input`
  |
  = note: `#[warn(unused_variables)]` on by default

warning: unused variable: `vm`
  --> src/main.rs:36:9
   |
36 |     let vm = vm.register_module(None, module)?;
   |         ^^ help: if this is intentional, prefix it with an underscore: `_vm`

warning: `wasmedgex` (bin "wasmedgex") generated 2 warnings (run `cargo fix --bin "wasmedgex"` to apply 2 suggestions)
    Finished dev [unoptimized + debuginfo] target(s) in 1.24s
     Running `target/debug/wasmedgex js_interpreter.wasm`
args: ["target/debug/wasmedgex", "js_interpreter.wasm"]
➜  wasmedgex git:(master) ✗
StaticallyTypedAnxiety commented 1 year ago

Platform WSL2 OS: Ubuntu 20.04.1 LTS

[package]
name = "wasmedgex"
version = "0.1.0"
edition = "2021"

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]
wasmedge-sdk = "0.12.2"
use wasmedge_sdk::{
    config::{CommonConfigOptions, ConfigBuilder, HostRegistrationConfigOptions},
    error::HostFuncError,
    host_function, Caller, ImportObjectBuilder, Module, NeverType, VmBuilder, WasmValue,
};

#[host_function]
fn my_add(_caller: Caller, input: Vec<WasmValue>) -> Result<Vec<WasmValue>, HostFuncError> {
    Ok(vec![])
}

type Type = NeverType;

fn main() -> Result<(), Box<dyn std::error::Error>> {
    let args: Vec<String> = std::env::args().collect();
    println!("args: {:?}", args);

    let wasm_lib_file = &args[1];

    // create an import module
    let import = ImportObjectBuilder::new()
        .with_func::<i32, (), NeverType>("log", my_add, None)?
        .build::<Type>("functiongraph_host", None)?;

    // create a new Vm with default config
    let config = ConfigBuilder::new(CommonConfigOptions::default())
        .with_host_registration_config(HostRegistrationConfigOptions::default().wasi(true))
        .build()?;

    let mut vm = VmBuilder::new().with_config(config).build()?;
    if let Some(wasi) = vm.wasi_module_mut() {
        wasi.initialize(None, None, None);
    }
    vm.register_import_module(&import)?;
    let module = Module::from_file(None, &wasm_lib_file)?;
    let vm = vm.register_module(None, module)?;
    Ok(())
}
➜  wasmedgex git:(master) ✗ cargo run js_interpreter.wasm
   Compiling wasmedgex v0.1.0 (/home/csh/workspace/hw/wasmedgex)
warning: unused variable: `input`
 --> src/main.rs:8:28
  |
8 | fn my_add(_caller: Caller, input: Vec<WasmValue>) -> Result<Vec<WasmValue>, HostFuncError> {
  |                            ^^^^^ help: if this is intentional, prefix it with an underscore: `_input`
  |
  = note: `#[warn(unused_variables)]` on by default

warning: unused variable: `vm`
  --> src/main.rs:36:9
   |
36 |     let vm = vm.register_module(None, module)?;
   |         ^^ help: if this is intentional, prefix it with an underscore: `_vm`

warning: `wasmedgex` (bin "wasmedgex") generated 2 warnings (run `cargo fix --bin "wasmedgex"` to apply 2 suggestions)
    Finished dev [unoptimized + debuginfo] target(s) in 1.24s
     Running `target/debug/wasmedgex js_interpreter.wasm`
args: ["target/debug/wasmedgex", "js_interpreter.wasm"]
➜  wasmedgex git:(master) ✗

Not at all for me it fails as soon as it registers the module. Maybe it is having a problem with WSL1? I could try to upgrade and see if that works

apepkuss commented 1 year ago

@AshantiM Please help close the issue if it has been resolved. Thanks a lot!

paulmchen commented 1 year ago

Richard is still able to reproduce this issue on an Ubuntu server. He is collecting the debugging info.

On Tue, Oct 24, 2023, 5:13 AM Xin Liu @.***> wrote:

@AshantiM https://github.com/AshantiM Please help close the issue if it has been resolved. Thanks a lot!

— Reply to this email directly, view it on GitHub https://github.com/WasmEdge/wasmedge-rust-sdk/issues/77#issuecomment-1776826379, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHYLRRGA6A2GMNSELCP4BZTYA6BDJAVCNFSM6AAAAAA6F66TEOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONZWHAZDMMZXHE . You are receiving this because you are subscribed to this thread.Message ID: @.***>

StaticallyTypedAnxiety commented 1 year ago

I attatched a Debugger to the code. In our code we call

vm.register_import_module(&import) and then we call vm.register_module(None,module).

My findings so far are that it is almost as if the functiongraph_host import module variable gets deleted after register_import_module is called.

one interesting thing I found is that during the creation of the Import we link the stores from the import module and then unlink the stores because the Module destructor is being called at the end of this function

Unlink Variables

The bug appears mostly here before we would try to obtain NamedMod it would contain functiongraph_host but it errors out at this moment because it can't find functiongraph_host because it has been unlinked and destroyed

Named Module Doesnt have functiongraph host Where the error happens

Additionally after register_import_module I can call vm.run_func("functiongraph_host","log",params) immedietly after and the function gets called succesfully.

L-jasmine commented 1 year ago

So, is it the store being unlinked that causes this bug? Indeed, the store's lifetime should be greater than the module that is linked into it. We've addressed this issue in the new version of the SDK, but it has led to significant API changes. Currently, it's in the testing phase.

paulmchen commented 1 year ago

@L-jasmine any workaround? and if it is going to be fixed in the next release of SDK, when would be the plan date? Thanks.

L-jasmine commented 1 year ago

@paulmchen I'm very sorry for the late response.

Currently, our new version is only in one branch. Due to the significant changes, we are in a transitional phase. If you have any suggestions, please feel free to provide feedback in this pull request. https://github.com/WasmEdge/wasmedge-rust-sdk/pull/83

If, for some reason, you cannot switch to the new SDK at the moment, please ensure that the lifetime of the store is greater than that of the VM when using the old SDK.

StaticallyTypedAnxiety commented 1 year ago

@paulmchen I'm very sorry for the late response.

Currently, our new version is only in one branch. Due to the significant changes, we are in a transitional phase. If you have any suggestions, please feel free to provide feedback in this pull request. https://github.com/WasmEdge/wasmedge-rust-sdk/pull/83

If, for some reason, you cannot switch to the new SDK at the moment, please ensure that the lifetime of the store is greater than that of the VM when using the old SDK.

We ended up removing the host function and migrating it to the wasm module which works pretty well. Through our regression we noticed that timeout is not working as expected on 12.2. We don't get an error when it goes beyond the provided timeout. Is there any insight you can provide?

L-jasmine commented 1 year ago

@AshantiM Could you provide a simple example for me to reproduce this Bug?

L-jasmine commented 1 year ago

I need to determine whether it's async or sync, as their logic differs somewhat.

paulmchen commented 1 year ago

@L-jasmine Richard will provide you a simple example. It looks like the Wasmedge timeout functions are not working in this version of the SDK.

L-jasmine commented 1 year ago

For call_func_async_with_timeout, the timeout behavior might seem counterintuitive. It only considers the time spent within the wasm, not the time spent in blocking operations. For example, if the wasm contains sleep(10s) and the timeout for call_func_async_with_timeout is set to 5s, the execution will continue for the full 10s and exit without returning an error after the wasm execution is completed.

In fact, it only measures the time spent during the execution of the poll function in the Future.

StaticallyTypedAnxiety commented 1 year ago

For call_func_async_with_timeout, the timeout behavior might seem counterintuitive. It only considers the time spent within the wasm, not the time spent in blocking operations. For example, if the wasm contains sleep(10s) and the timeout for call_func_async_with_timeout is set to 5s, the execution will continue for the full 10s and exit without returning an error after the wasm execution is completed.

In fact, it only measures the time spent during the execution of the poll function in the Future.

I was using sync timeout. I have the same code that uses a timeout method that uses this executor with run_func_timeout https://github.com/second-state/WasmEdge/blob/feat/async/bindings/rust/wasmedge-sys/src/executor.rs and run_func_with_timeout with this executor. https://github.com/WasmEdge/wasmedge-rust-sdk/blob/main/crates/wasmedge-sys/src/executor.rs

The code itself passes when using the feat/async method but fails when I switch to the new version. Im trying to create an example and hopefully will have it here soon.

StaticallyTypedAnxiety commented 1 year ago

Looks like the issue is only specific to wsl 1 I would guess. We tried it on another machine and it ended up working. I think this is okay for us, will be closing this issue. Thank you very much!