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

Add a check to command modules to ensure that they're only started once. #329

Merged
merged 2 commits into from
Oct 14, 2022

Conversation

sunfishcode
Copy link
Member

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.

@@ -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.
Copy link
Member

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?

Copy link
Member Author

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.

if (started != 0) {
__builtin_trap();
}
started = 0;
Copy link
Member

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

Copy link
Member Author

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.

Copy link
Member

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.
@sunfishcode sunfishcode force-pushed the sunfishcode/command-called-once branch from 8f56960 to 2323070 Compare September 30, 2022 21:26
Copy link
Member

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm with comment

if (started != 0) {
__builtin_trap();
}
started = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

started = 1; here

@sunfishcode
Copy link
Member Author

Oops; fixed.

@sunfishcode sunfishcode merged commit 21d93b9 into main Oct 14, 2022
@sunfishcode sunfishcode deleted the sunfishcode/command-called-once branch October 14, 2022 00:58
sunfishcode pushed a commit that referenced this pull request Jan 30, 2023

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
…nce (#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 #329.
john-sharratt pushed a commit to john-sharratt/wasix-libc that referenced this pull request Mar 6, 2023
…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.
john-sharratt pushed a commit to john-sharratt/wasix-libc that referenced this pull request Mar 6, 2023
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants