From 07913baf5138b2b18dd20282cfdfd0d88157feff Mon Sep 17 00:00:00 2001 From: jr Date: Tue, 23 Apr 2019 20:56:26 +0200 Subject: [PATCH 1/2] mark fork as unsafe 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. --- CHANGELOG.md | 4 +++- src/unistd.rs | 6 +++--- test/sys/test_ptrace.rs | 2 +- test/sys/test_uio.rs | 2 +- test/sys/test_wait.rs | 8 ++++---- test/test_unistd.rs | 6 +++--- 6 files changed, 15 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c52fd5271c..0020b54779 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,7 +41,9 @@ This project adheres to [Semantic Versioning](http://semver.org/). ([#1020](https://github.com/nix-rust/nix/pull/1020)) - Replaced `CmsgSpace` with the `cmsg_space` macro. ([#1020](https://github.com/nix-rust/nix/pull/1020)) - +- Marked `fork` as unsafe function + ([#1047](https://github.com/nix-rust/nix/pull/1047)) + ### Fixed - Fixed multiple bugs when using `sendmsg` and `recvmsg` with ancillary control messages ([#1020](https://github.com/nix-rust/nix/pull/1020)) diff --git a/src/unistd.rs b/src/unistd.rs index 96d8ace78c..d3e33070ac 100644 --- a/src/unistd.rs +++ b/src/unistd.rs @@ -192,7 +192,7 @@ impl ForkResult { /// ```no_run /// use nix::unistd::{fork, ForkResult}; /// -/// match fork() { +/// match unsafe {fork()} { /// Ok(ForkResult::Parent { child, .. }) => { /// println!("Continuing execution in parent process, new child has pid: {}", child); /// } @@ -222,9 +222,9 @@ impl ForkResult { /// /// [async-signal-safe]: http://man7.org/linux/man-pages/man7/signal-safety.7.html #[inline] -pub fn fork() -> Result { +pub unsafe fn fork() -> Result { use self::ForkResult::*; - let res = unsafe { libc::fork() }; + let res = libc::fork(); Errno::result(res).map(|res| match res { 0 => Child, diff --git a/test/sys/test_ptrace.rs b/test/sys/test_ptrace.rs index 24d9b522ee..5311202f17 100644 --- a/test/sys/test_ptrace.rs +++ b/test/sys/test_ptrace.rs @@ -74,7 +74,7 @@ fn test_ptrace_cont() { return; } - match fork().expect("Error: Fork Failed") { + match unsafe {fork()}.expect("Error: Fork Failed") { Child => { ptrace::traceme().unwrap(); // As recommended by ptrace(2), raise SIGTRAP to pause the child diff --git a/test/sys/test_uio.rs b/test/sys/test_uio.rs index 3e4fc28ceb..58da573bcc 100644 --- a/test/sys/test_uio.rs +++ b/test/sys/test_uio.rs @@ -207,7 +207,7 @@ fn test_process_vm_readv() { let mut vector = vec![1u8, 2, 3, 4, 5]; let (r, w) = pipe().unwrap(); - match fork().expect("Error: Fork Failed") { + match unsafe {fork()}.expect("Error: Fork Failed") { Parent { child } => { close(w).unwrap(); // wait for child diff --git a/test/sys/test_wait.rs b/test/sys/test_wait.rs index d07d82f0d9..e92c48b6f1 100644 --- a/test/sys/test_wait.rs +++ b/test/sys/test_wait.rs @@ -10,7 +10,7 @@ fn test_wait_signal() { let _ = ::FORK_MTX.lock().expect("Mutex got poisoned by another test"); // Safe: The child only calls `pause` and/or `_exit`, which are async-signal-safe. - match fork().expect("Error: Fork Failed") { + match unsafe {fork().expect("Error: Fork Failed")} { Child => { pause(); unsafe { _exit(123) } @@ -27,7 +27,7 @@ fn test_wait_exit() { let _m = ::FORK_MTX.lock().expect("Mutex got poisoned by another test"); // Safe: Child only calls `_exit`, which is async-signal-safe. - match fork().expect("Error: Fork Failed") { + match unsafe { fork()}.expect("Error: Fork Failed") { Child => unsafe { _exit(12); }, Parent { child } => { assert_eq!(waitpid(child, None), Ok(WaitStatus::Exited(child, 12))); @@ -47,7 +47,7 @@ fn test_waitstatus_from_raw() { fn test_waitstatus_pid() { let _m = ::FORK_MTX.lock().expect("Mutex got poisoned by another test"); - match fork().unwrap() { + match unsafe {fork()}.unwrap() { Child => unsafe { _exit(0) }, Parent { child } => { let status = waitpid(child, None).unwrap(); @@ -96,7 +96,7 @@ mod ptrace { fn test_wait_ptrace() { let _m = ::FORK_MTX.lock().expect("Mutex got poisoned by another test"); - match fork().expect("Error: Fork Failed") { + match unsafe {fork()}.expect("Error: Fork Failed") { Child => ptrace_child(), Parent { child } => ptrace_parent(child), } diff --git a/test/test_unistd.rs b/test/test_unistd.rs index 3f13883c7e..2f9990a12a 100644 --- a/test/test_unistd.rs +++ b/test/test_unistd.rs @@ -19,7 +19,7 @@ fn test_fork_and_waitpid() { let _m = ::FORK_MTX.lock().expect("Mutex got poisoned by another test"); // Safe: Child only calls `_exit`, which is signal-safe - match fork().expect("Error: Fork Failed") { + match unsafe {fork()}.expect("Error: Fork Failed") { Child => unsafe { _exit(0) }, Parent { child } => { // assert that child was created and pid > 0 @@ -47,7 +47,7 @@ fn test_wait() { let _m = ::FORK_MTX.lock().expect("Mutex got poisoned by another test"); // Safe: Child only calls `_exit`, which is signal-safe - match fork().expect("Error: Fork Failed") { + match unsafe {fork()}.expect("Error: Fork Failed") { Child => unsafe { _exit(0) }, Parent { child } => { let wait_status = wait(); @@ -191,7 +191,7 @@ macro_rules! execve_test_factory( // Safe: Child calls `exit`, `dup`, `close` and the provided `exec*` family function. // NOTE: Technically, this makes the macro unsafe to use because you could pass anything. // The tests make sure not to do that, though. - match fork().unwrap() { + match unsafe {fork()}.unwrap() { Child => { // Close stdout. close(1).unwrap(); From 6494e6a114d4d33d264f95ba6f441179f4afd589 Mon Sep 17 00:00:00 2001 From: jr Date: Mon, 29 Apr 2019 21:26:52 +0200 Subject: [PATCH 2/2] Make documentation for fork(2) a bit more clear --- src/unistd.rs | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/src/unistd.rs b/src/unistd.rs index d3e33070ac..d2dc69c7c6 100644 --- a/src/unistd.rs +++ b/src/unistd.rs @@ -185,9 +185,14 @@ impl ForkResult { /// Create a new child process duplicating the parent process ([see /// fork(2)](http://pubs.opengroup.org/onlinepubs/9699919799/functions/fork.html)). /// -/// After calling the fork system call (successfully) two processes will -/// be created that are identical with the exception of their pid and the -/// return value of this function. As an example: +/// After calling the fork system call (successfully) a new process (child) will +/// be created that is a copy of the calling process (parent). If the parent process is +/// multithreaded, only the calling thread will be cloned. +/// The child process differs in a few aspects from the parent process, most notably the pid and the +/// return value of this function. For a list of all differences in the child and parent +/// see the [man page]. +/// +/// As an example: /// /// ```no_run /// use nix::unistd::{fork, ForkResult}; @@ -210,17 +215,22 @@ impl ForkResult { /// I'm a new child process /// ``` /// +/// /// # 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. /// /// 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). +/// Note that memory allocation may **not** be async-signal-safe and thus must be /// 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 #[inline] pub unsafe fn fork() -> Result { use self::ForkResult::*;