-
Notifications
You must be signed in to change notification settings - Fork 824
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
Remove Send
and Sync
from wasmer
types backed by JavaScript objects
#4157
Conversation
@@ -0,0 +1,32 @@ | |||
/// Assert that a type does **not** implement a set of traits. | |||
macro_rules! assert_not_implemented { | |||
($t:ty : !$first:ident $(+ !$rest:ident)*) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a small nit: assert_not_implemented!(Instance: !Send + !Sync);
sounds like a double negative. so one might initially expect Instance
to not implement !Send
, i.e. that Instance
should be Send
. I don't have suggestions for alternate syntax though.
d0a4923
to
f56664d
Compare
// structuredClone(memory) | ||
// ``` | ||
unsafe impl Send for Memory {} | ||
unsafe impl Sync for Memory {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Side note: should't clippy have required a Safety: ... comment here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Although: this is technically a breaking change, and would require a 5.0
...
Although we can probably get by with just ignoring this here, since it is basically a bug fix.
@@ -11,9 +11,7 @@ pub struct Table { | |||
pub(crate) handle: VMTable, | |||
} | |||
|
|||
// Table can't be Send in js because it dosen't support `structuredClone` | |||
// https://developer.mozilla.org/en-US/docs/Web/API/structuredClone | |||
// unsafe impl Send for Table {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we keep a comment on all assert_not_implemented
usage points so future readers know what's going on here?
This PR looks great, but it will require further rework on WASIX to make it work with this changes |
This fixes a soundness bug introduced in #3556 which added
Send+Sync
bounds to things likewasmer::js::Module
andwasmer::js::Memory
.Here's one example:
wasmer/lib/api/src/js/module.rs
Lines 48 to 57 in d53b9e2
The underlying assumption is that because a type implements
structuredClone
it's fine to implementSend
andSync
.However,
structuredClone
only applies when sending values to a worker usingpostMessage
.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.Fixes #4158.