Merge #695

695: Document safety of `fork()` and fix tests r=asomers

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.

cc #586

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<ForkResult> {
     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
 /// }
 /// ```

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)));
       },
@@ -46,13 +48,14 @@ mod ptrace {
     use nix::unistd::*;
     use nix::unistd::ForkResult::*;
     use std::{ptr, process};
+    use libc::_exit;
 
     fn ptrace_child() -> ! {
         let _ = ptrace(PTRACE_TRACEME, Pid::from_raw(0), ptr::null_mut(), ptr::null_mut());
         // As recommended by ptrace(2), raise SIGTRAP to pause the child
         // until the parent is ready to continue
         let _ = raise(SIGTRAP);
-        process::exit(0)
+        unsafe { _exit(0) }
     }
 
     fn ptrace_parent(child: Pid) {

test/test_unistd.rs

@@ -11,16 +11,16 @@ 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");
-    let pid = fork();
-    match pid {
-        Ok(Child) => exit(0),
+
+    // Safe: Child only calls `_exit`, which is signal-safe
+    match fork() {
+        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 +48,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 +113,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"))]