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

Wasmer/js raises cannot read property 'buffer' when try to access memory via Env #2816

Closed
kwonoj opened this issue Mar 10, 2022 · 3 comments · Fixed by #2892
Closed

Wasmer/js raises cannot read property 'buffer' when try to access memory via Env #2816

kwonoj opened this issue Mar 10, 2022 · 3 comments · Fixed by #2892
Labels
bug Something isn't working priority-high High priority issue
Milestone

Comments

@kwonoj
Copy link
Contributor

kwonoj commented Mar 10, 2022

Describe the bug

Note: I'm filing this as a bug since I was not able to figure out why one passes and the other is not, but it is totally possible I made a mistakes in my code. Please change category if needed. 🙏

I'm trying to make existing wasm execution logic to be compatible on wasmer/js features as well, to allow run wasm binary from its executor / runner compiled into wasm itself. I made poc work, however in certain case wasmer raises TypeError: Cannot read property 'buffer' of undefined when it try to access memory from imported fn's env and I am not able to figure out what exactly triggers this.

Passing poc

fn run() {
    let wasmer_store = Store::default();
    let module = Module::new(&wasmer_store, &WASM_BINARY).unwrap();
    let transform_result: Arc<Mutex<Vec<u8>>> = Arc::new(Mutex::new(vec![]));

    let set_transform_result_fn_decl = Function::new_native_with_env(
        &wasmer_store,
        HostEnvironment {
            memory: LazyInit::default(),
            transform_result: transform_result.clone(),
        },
        set_transform_result,
    );

    let imports = imports! {
        "env" => {
            "__set_transform_result" => set_transform_result_fn_decl,
        }
    };

    let instance = Instance::new(&module, &imports).unwrap();

    let input_ptr = write_bytes_into_guest(&instance, &input_serialized);

    let transform_fn = instance
        .exports
        .get_native_function::<(i32, i32), i32>("__guest_transform")
        .unwrap();

    transform_fn.call(input_ptr.0, input_ptr.1).unwrap();
}

Failing POC

fn load_plugin() -> Result<(Instance, Arc<Mutex<Vec<u8>>>), Error> {
    let wasmer_store = Store::default();
    let module = Module::new(&wasmer_store, &WASM_BINARY);

    return match module {
        Ok(module) => {
            let transform_result: Arc<Mutex<Vec<u8>>> = Arc::new(Mutex::new(vec![]));

            let set_transform_result_fn_decl = Function::new_native_with_env(
                &wasmer_store,
                HostEnvironment {
                    memory: LazyInit::default(),
                    transform_result: transform_result.clone(),
                },
                set_transform_result,
            );

            let imports = imports! {
                "env" => {
                    "__set_transform_result" => set_transform_result_fn_decl,
                }
            };

            let instance = Instance::new(&module, &imports).unwrap();

            Ok((instance, transform_result))
        },
        Err(err) => panic!("should not be here"),
    }
}

fn run() {
    let (instance, transform_result) = load_plugin().unwrap();

    let input_ptr = write_bytes_into_guest(&instance, &input_serialized);

    let transform_fn = instance
        .exports
        .get_native_function::<(i32, i32), i32>("__guest_transform")
        .unwrap();


    transform_fn.call(input_ptr.0, input_ptr.1).unwrap();
}

Codes are exactly same, the only difference is failing cases have separate fn to load / instantiate module then return created instance (with cloned env properties).

  1. Is there something I did setup mistakenly cuases this?
  2. If so, what's the reason passing case works properly? Somethings gets dropped with load_plugin maybe?
  3. What is the correct / recommended pattern in this case?

Thanks!

Steps to reproduce

  1. Clone https://github.com/kwonoj/wasmer-js-test
  2. Run ./build.sh && node index.js
  3. See test_success() passes (console out JSON object), test_fail() raises exception

Expected behavior

Actual behavior

wasm-bindgen: imported JS function that was not marked as `catch` threw an error: Cannot read property 'buffer' of undefined

Stack:
TypeError: Cannot read property 'buffer' of undefined
    at /home/ojkwon/github_wsl/wasmer-js-test/host_runner/pkg/host_runner.js:460:30
    at logError (/home/ojkwon/github_wsl/wasmer-js-test/host_runner/pkg/host_runner.js:222:18)
    at module.exports.__wbg_buffer_5e74a88a1424a2e0 (/home/ojkwon/github_wsl/wasmer-js-test/host_runner/pkg/host_runner.js:459:68)
    at js_sys::WebAssembly::Memory::buffer::hf044a38285fed8aa (<anonymous>:wasm-function[8773]:0x2f99f6)
    at wasmer::js::externals::memory::Memory::size::hfd9a6285e7a4b573 (<anonymous>:wasm-function[1247]:0x1b53e1)
    at wasmer::js::ptr::WasmPtr<T,wasmer::js::ptr::Array>::deref::h310443bb52737f93 (<anonymous>:wasm-function[587]:0x14acbc)
    at host_runner::set_transform_result::hac06130700517b90 (<anonymous>:wasm-function[866]:0x17f185)
    at core::ops::function::Fn::call::h4ed7026923862d80 (<anonymous>:wasm-function[7064]:0x2d468f)
    at <Func as wasmer::js::externals::function::inner::HostFunction<(A1,A2),Rets,wasmer::js::externals::function::inner::WithEnv,Env>>::function_body_ptr::func_wrapper::{{closure}}::h558bdfb380e59924 (<anonymous>:wasm-function[4322]:0x28659d)
    at <core::panic::unwind_safe::AssertUnwindSafe<F> as core::ops::function::FnOnce<()>>::call_once::h5553e26885b27801 (<anonymous>:wasm-function[4896]:0x29a416)
@kwonoj kwonoj added the bug Something isn't working label Mar 10, 2022
@syrusakbary
Copy link
Member

Thanks for creating the issue!

We have identified an issue in wasmer-js raleted to this that basically frees the imports after instantiating (this doesn't happen when you use a js-only approach, but it happens in Rust).

This will be fixed in the next releases.

To verify this assumption, could you try storing the imports so they don't get freed and see if that fixes the issue?

fn load_plugin() -> Result<(Instance, ImportObject, Arc<Mutex<Vec<u8>>>), Error> {
    let wasmer_store = Store::default();
    let module = Module::new(&wasmer_store, &WASM_BINARY);

    return match module {
        Ok(module) => {
            let transform_result: Arc<Mutex<Vec<u8>>> = Arc::new(Mutex::new(vec![]));

            let set_transform_result_fn_decl = Function::new_native_with_env(
                &wasmer_store,
                HostEnvironment {
                    memory: LazyInit::default(),
                    transform_result: transform_result.clone(),
                },
                set_transform_result,
            );

            let imports = imports! {
                "env" => {
                    "__set_transform_result" => set_transform_result_fn_decl,
                }
            };

            let instance = Instance::new(&module, &imports).unwrap();

            Ok((instance, imports, transform_result))
        },
        Err(err) => panic!("should not be here"),
    }
}

fn run() {
    let (instance, imports, transform_result) = load_plugin().unwrap();

    let input_ptr = write_bytes_into_guest(&instance, &input_serialized);

    let transform_fn = instance
        .exports
        .get_native_function::<(i32, i32), i32>("__guest_transform")
        .unwrap();


    transform_fn.call(input_ptr.0, input_ptr.1).unwrap();
}

@kwonoj
Copy link
Contributor Author

kwonoj commented Mar 10, 2022

Oh wow, I spent nearly week to find out isolated case cause I never expected creating a fn might cause unexpectedly drop imports.

I tried suggested verification and it looks like correctly preserving imports. Guess I can wait for next release.

Thanks!

@syrusakbary
Copy link
Member

syrusakbary commented Apr 27, 2022

This issue is solved will be fully resolved on the next major Wasmer release 3.0:

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

Successfully merging a pull request may close this issue.

2 participants