-
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
mark fork as unsafe #1047
mark fork as unsafe #1047
Conversation
@@ -192,7 +192,7 @@ impl ForkResult { | |||
/// ```no_run | |||
/// use nix::unistd::{fork, ForkResult}; | |||
/// | |||
/// match fork() { | |||
/// match unsafe {fork()} { |
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.
You should add a section to the docs describing how to use fork
safely, like this:
/// # Safety
///
/// To use this function safely, you must do A, B, and C.
Reasoning: After a fork() in a multithreaded program, only async-signal-safe functions may be called by the child. Calling everything else (including malloc(), mutexes) is undefined behavior.
/// # Safety | ||
/// Make sure to read the [man page]. | ||
/// Fork has some properties that can lead to unintended behavior. | ||
/// Especially the handling of mutexes and open files can be tricky. |
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.
sentence fragment.
/// | ||
/// In a multithreaded program, only [async-signal-safe] functions like `pause` | ||
/// and `_exit` may be called by the child (the parent isn't restricted). Note | ||
/// that memory allocation may **not** be async-signal-safe and thus must be | ||
/// and `_exit` are safe to call by the child until it calls `execve`(the parent isn't restricted). |
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.
Technically, there are other functions that can take the place of execve
, like fexecve
. Better just to say "until it execs another program" or something like that.
/// prevented. | ||
/// | ||
/// Those functions are only a small subset of your operating system's API, so | ||
/// special care must be taken to only invoke code you can control and audit. | ||
/// | ||
/// [async-signal-safe]: http://man7.org/linux/man-pages/man7/signal-safety.7.html | ||
/// [man page]: http://man7.org/linux/man-pages/man2/fork.2.html |
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.
For standardized functions, it's better to use the open group's man page: http://pubs.opengroup.org/onlinepubs/9699919799/functions/fork.html
/// # Safety | ||
/// Make sure to read the [man page]. |
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 consider that RTFM goes without saying. You can remove this sentence.
Why did this get closed? |
Shall this pull request be resubmitted with the documentation changes proposed above addressed? |
Yeah, it should. |
Since fork() is definitely unsafe and requires care, it was marked unsafe back in 2019. Put the unsafe block around it to compile with newer versions of nix. nix-rust/nix#1047
Reasoning: After a fork() in a multithreaded program, only async-signal-safe functions may be called by the child.
Calling everything else (including malloc(), mutexes) is undefined behavior.
This is obviously a breaking change
References:
std discussion
std merge request
signal-safety(7)
fork(2)
Closes: #1030