-
Notifications
You must be signed in to change notification settings - Fork 206
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
Add a check to command modules to ensure that they're only started once. #329
Conversation
libc-bottom-half/crt/crt1-command.c
Outdated
@@ -3,8 +3,20 @@ extern void __wasm_call_ctors(void); | |||
extern int __main_void(void); | |||
extern void __wasm_call_dtors(void); | |||
|
|||
// Commands should only be called once per instance. This simple check ensures | |||
// that the `_start` function isn't started more than once. `address_space(1)` | |||
// tells clang to put this variable in a wasm global. |
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.
Does that actually work? It seems a little risky to depend on this now. Why not just is a simple static?
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.
No big reason; and depending on it breaks LLVM 11, which we still support for now, so I'll switch to a static.
libc-bottom-half/crt/crt1-command.c
Outdated
if (started != 0) { | ||
__builtin_trap(); | ||
} | ||
started = 0; |
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.
Seems like it would be good place for #ifndef NDEBUG
but we don't have a debug version of libc yet IIUC
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.
I care about code size, but this is just a few bytes. And this hazard comes up in both component use cases and JS embedding use cases, where creating an instance is separated from running the command and there's no other way to prevent the command from being run multiple times, causing UB.
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.
started = 1;
here
Wasm command modules should only be called once per instance, because the programming model doesn't leave linear memory in a reusable state when the program exits. As use cases arise for loading wasm modules in environments that want to treat them like reactors, add a safety check to ensure that command modules are used according to their expectations.
8f56960
to
2323070
Compare
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 with comment
libc-bottom-half/crt/crt1-command.c
Outdated
if (started != 0) { | ||
__builtin_trap(); | ||
} | ||
started = 0; |
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.
started = 1;
here
Oops; fixed. |
…ce. (WebAssembly#329) * Add a check to command modules to ensure that they're only started once. Wasm command modules should only be called once per instance, because the programming model doesn't leave linear memory in a reusable state when the program exits. As use cases arise for loading wasm modules in environments that want to treat them like reactors, add a safety check to ensure that command modules are used according to their expectations.
…nce (WebAssembly#388) Calling _initialize multiple times is undefined behavior, since the ctors are not guaranteed to be idempotent. We should have this safety check which is similar to WebAssembly#329.
Wasm command modules should only be called once per instance, because the programming model doesn't leave linear memory in a reusable state when the program exits. As use cases arise for loading wasm modules in environments that want to treat them like reactors, add a safety check to ensure that command modules are used according to their expectations.