From 7128f155df302edef0173d68c4eaa139fedcc020 Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Thu, 20 Jul 2017 19:55:13 +0200 Subject: Document invariants of fork and fix tests 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. --- src/unistd.rs | 16 ++++++++++++++-- test/sys/test_wait.rs | 8 +++++--- test/test_unistd.rs | 14 ++++++++++---- 3 files changed, 29 insertions(+), 9 deletions(-) diff --git a/src/unistd.rs b/src/unistd.rs index 25237a07..19f82ef6 100644 --- a/src/unistd.rs +++ b/src/unistd.rs @@ -191,6 +191,18 @@ impl ForkResult { /// Continuing execution in parent process, new child has pid: 1234 /// I'm a new child process /// ``` +/// +/// # Safety +/// +/// 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 +/// 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 #[inline] pub fn fork() -> Result { use self::ForkResult::*; @@ -687,7 +699,7 @@ pub fn gethostname<'a>(buffer: &'a mut [u8]) -> Result<&'a CStr> { /// use nix::unistd::close; /// /// fn main() { -/// let mut f = tempfile::tempfile().unwrap(); +/// let f = tempfile::tempfile().unwrap(); /// close(f.as_raw_fd()).unwrap(); // Bad! f will also close on drop! /// } /// ``` @@ -700,7 +712,7 @@ pub fn gethostname<'a>(buffer: &'a mut [u8]) -> Result<&'a CStr> { /// use nix::unistd::close; /// /// fn main() { -/// let mut f = tempfile::tempfile().unwrap(); +/// let f = tempfile::tempfile().unwrap(); /// close(f.into_raw_fd()).unwrap(); // Good. into_raw_fd consumes f /// } /// ``` diff --git a/test/sys/test_wait.rs b/test/sys/test_wait.rs index 5f6c9231..e1d3464b 100644 --- a/test/sys/test_wait.rs +++ b/test/sys/test_wait.rs @@ -2,15 +2,16 @@ use nix::unistd::*; use nix::unistd::ForkResult::*; use nix::sys::signal::*; use nix::sys::wait::*; -use libc::exit; +use libc::_exit; #[test] fn test_wait_signal() { #[allow(unused_variables)] let m = ::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() { - Ok(Child) => pause().unwrap_or(()), + Ok(Child) => pause().unwrap_or_else(|_| unsafe { _exit(123) }), Ok(Parent { child }) => { kill(child, Some(SIGKILL)).ok().expect("Error: Kill Failed"); assert_eq!(waitpid(child, None), Ok(WaitStatus::Signaled(child, SIGKILL, false))); @@ -25,8 +26,9 @@ fn test_wait_exit() { #[allow(unused_variables)] let m = ::FORK_MTX.lock().expect("Mutex got poisoned by another test"); + // Safe: Child only calls `_exit`, which is async-signal-safe. match fork() { - Ok(Child) => unsafe { exit(12); }, + Ok(Child) => unsafe { _exit(12); }, Ok(Parent { child }) => { assert_eq!(waitpid(child, None), Ok(WaitStatus::Exited(child, 12))); }, diff --git a/test/test_unistd.rs b/test/test_unistd.rs index fd4e327b..14826b9e 100644 --- a/test/test_unistd.rs +++ b/test/test_unistd.rs @@ -11,16 +11,17 @@ use std::io::Write; use std::os::unix::prelude::*; use tempfile::tempfile; use tempdir::TempDir; -use libc::off_t; -use std::process::exit; +use libc::{_exit, off_t}; #[test] fn test_fork_and_waitpid() { #[allow(unused_variables)] let m = ::FORK_MTX.lock().expect("Mutex got poisoned by another test"); + + // Safe: Child only calls `_exit`, which is signal-safe let pid = fork(); match pid { - Ok(Child) => exit(0), + Ok(Child) => unsafe { _exit(0) }, Ok(Parent { child }) => { // assert that child was created and pid > 0 let child_raw: ::libc::pid_t = child.into(); @@ -48,9 +49,11 @@ fn test_wait() { // Grab FORK_MTX so wait doesn't reap a different test's child process #[allow(unused_variables)] let m = ::FORK_MTX.lock().expect("Mutex got poisoned by another test"); + + // Safe: Child only calls `_exit`, which is signal-safe let pid = fork(); match pid { - Ok(Child) => exit(0), + Ok(Child) => unsafe { _exit(0) }, Ok(Parent { child }) => { let wait_status = wait(); @@ -111,6 +114,9 @@ macro_rules! execve_test_factory( // data from `reader`. let (reader, writer) = pipe().unwrap(); + // 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() { Child => { #[cfg(not(target_os = "android"))] -- cgit v1.2.3