-
Notifications
You must be signed in to change notification settings - Fork 654
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
Document safety of fork()
and fix tests
#695
Conversation
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 think this makes sense, setting fork
to be unsafe. Just a few minor nits here to clean up. Also @asomers will need to take a look at this.
test/test_unistd.rs
Outdated
let pid = fork(); | ||
|
||
// Safe: Child only calls `_exit`, which is signal-safe | ||
let pid = unsafe { fork() }; | ||
match pid { |
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.
Let's get rid of the pid
variable completely here for consistency with other tests and since it isn't used anywhere else.
src/unistd.rs
Outdated
/// | ||
/// # Safety | ||
/// | ||
/// In a multithreaded program, only [async-signal-safe] functions may be called |
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.
Your comments talk about libc::exit
and libc::pause
being safe to use here. Let's explicitly list those here. We should also state that std::process:exit
is not safe.
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.
libc::exit
actually isn't safe, only _exit
is
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.
Sorry, that's what I meant. We should document a few of the safe and unsafe things for reference. The big thing about this is that how would the user figure out what is safe or not. I don't know if we have any guidance on that end, however.
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 like all of the changes that you've made to the tests. But I still think it's premature to mark fork
as unsafe
. It's not that I think fork
is safe; it's that I think consistency between nix and libstd is more important.
Nobody has yet come up with a POC that violates Rust's memory safety rules by fork
ing. Until that happens, I don't think libstd
is going to change.
test/test_unistd.rs
Outdated
@@ -111,7 +114,10 @@ macro_rules! execve_test_factory( | |||
// data from `reader`. | |||
let (reader, writer) = pipe().unwrap(); | |||
|
|||
match fork().unwrap() { | |||
// Safe: Child calls `exit`, `dup` and the provided `exec*` family function. |
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.
If you're going to list them all, you should add close
And this is a very bad thing IMO. Maybe it's hard to come up with a POC on the currently supported configurations, but what about other allocator implementations, other libcs, other operating systems, other architectures where this is a more serious issue? Even if those aren't supported by Rust today, even if those configurations don't even exist today - the fact that POSIX says "don't do this it's unsafe" should be more than enough of a reason to proceed with extreme caution. A language that aims to guarantee memory safety shouldn't do things like this. |
POSIX says it's "unsafe", but the libstd guys say it's not |
Yeah, seems like POSIX leaves the meaning of "safe" completely undefined. Depending on that, |
As an aside, from the docs of
I would be very surprised if "the behaviour is undefined" means something different than UB in C in this context, and the child handler is basically just code that is executed by the child after a fork, so there isn't much of a difference to what we're dealing with. (UB is strictly worse than memory unsafety, after all) |
I don't want to argue with you about the safety of |
I'd like to echo what @asomers says here. Rust has a pretty narrow definition of "unsafe" and whatever |
PR #566 just added a new |
Some tests were invoking non-async-signal-safe functions from the child process after a `fork`. Since they might be invoked in parallel, this could lead to problems.
Note that ptrace isn't documented as signal-safe, but it's supposed to just be a light syscall wrapper, so it should be fine.
fork()
unsafe and fix testsfork()
and fix tests
Thanks for the PR @jonas-schievink . Looks like we can sneak this one into 0.9.0 bors r+ |
Build succeeded |
Some tests were invoking non-async-signal-safe functions from the child
process after a
fork
. Since they might be invoked in parallel, thiscould lead to problems.
cc #586