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

The go! macro is unsound #109

Open
jmaargh opened this issue Jun 1, 2024 · 3 comments
Open

The go! macro is unsound #109

jmaargh opened this issue Jun 1, 2024 · 3 comments

Comments

@jmaargh
Copy link

jmaargh commented Jun 1, 2024

The go! macro is unsound and should require an unsafe block to use, with proper "Safety" documentation for the macro.

As discussed in #6, this crate can be unsound when thread-local storage is used. My understanding is that this is an unavoidable property of stackful coroutines when there isn't language-level support for them. Nonetheless, APIs that can lead to UB should only be accessible within an unsafe block and have documentation for the conditions under which their use is sound (that's what unsafe is for).

In #8 the spawn API was correctly marked as unsafe because of this. However, then the go! macro was introduced which simply silently inserts the unsafe block -- this only serves to hide the issue from users and obscure the safety documentation.

Personally, I'd remove the go! macro altogether. But if it is to stay, then I think it should definitely require an unsafe block and its safety requirements should be properly documented as any unsafe function should.

@Xudong-Huang
Copy link
Owner

This is designed intentially. If you need to make unsafe block explicitly, you could use the raw spawn API directcly.

@jmaargh
Copy link
Author

jmaargh commented Jun 3, 2024

That's not how unsafe works. It is possible to cause UB by using the go! macro and therefore the go! macro should require an unsafe block to use every time for all users (and should come with documentation specifying exactly when it is safe or unsafe to use)

@DavidLee18
Copy link

Hi, I just saw caveats, and became curious, so here I ask. can we detect when accessing the TLS in coroutine? If so, what about just panic!ing instead of UB? The same goes for when exceeding the coroutine stack. If implemented, it would be much better(safe and sound actually) than undefined behavior or segfault.

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

No branches or pull requests

3 participants