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

Soundness bug with Send+Sync implementations in the browser #4158

Open
Michael-F-Bryan opened this issue Aug 17, 2023 · 0 comments
Open

Soundness bug with Send+Sync implementations in the browser #4158

Michael-F-Bryan opened this issue Aug 17, 2023 · 0 comments
Labels
🚨 breaking change This Issue or PR involves a breaking change 📦 lib-api About wasmer priority-high High priority issue 🔈soundness Bugs causing an unsound API
Milestone

Comments

@Michael-F-Bryan
Copy link
Contributor

Describe the bug

It looks like we accidentally introduced a soundness bug in #3556 when adding Send+Sync bounds to things like wasmer::js::Module and wasmer::js::Memory.

Here's one example:

// Module implements `structuredClone` in js, so it's safe it to make it Send.
// https://developer.mozilla.org/en-US/docs/Web/API/structuredClone
// ```js
// const module = new WebAssembly.Module(new Uint8Array([
// 0x00, 0x61, 0x73, 0x6d, 0x01, 0x00, 0x00, 0x00
// ]));
// structuredClone(module)
// ```
unsafe impl Send for Module {}
unsafe impl Sync for Module {}

The underlying assumption is that because a type implements structuredClone it's fine to implement Send and Sync.

However, structuredClone only applies when sending values to a worker using postMessage.

If you use any other method to transfer a value to another thread (e.g. channels) then the underlying JsValue will point to the wrong object on that thread's wasm-bindgen "heap" and you'll have a bad time.

Additional context

See the conversation on Slack for more

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🚨 breaking change This Issue or PR involves a breaking change 📦 lib-api About wasmer priority-high High priority issue 🔈soundness Bugs causing an unsound API
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants