Fix thread safety issues in aio, chdir, and wait tests

They have four problems:

* The chdir tests change the process's cwd, which is global.  Protect them
  all with a mutex.

* The wait tests will reap any subprocess, and several tests create
  subprocesses.  Protect them all with a mutex so only one
  subprocess-creating test will run at a time.

* When a multithreaded test forks, the child process can sometimes block in
  the stack unwinding code.  It blocks on a mutex that was held by a
  different thread in the parent, but that thread doesn't exist in the
  child, so a deadlock results.  Fix this by immediately calling
  std::process:exit in the child processes.

* My previous attempt at thread safety in the aio tests didn't work, because
  anonymous MutexGuards drop immediately.  Fix this by naming the
  SIGUSR2_MTX MutexGuards.

Fixes #251

Author: asomers

test/sys/test_aio.rs

@@ -8,7 +8,6 @@ use std::io::{Write, Read, Seek, SeekFrom};
 use std::ops::Deref;
 use std::os::unix::io::AsRawFd;
 use std::rc::Rc;
-use std::sync::Mutex;
 use std::sync::atomic::{AtomicBool, Ordering};
 use std::{thread, time};
 use tempfile::tempfile;
@@ -233,8 +232,6 @@ fn test_write() {
 
 lazy_static! {
     pub static ref SIGNALED: AtomicBool = AtomicBool::new(false);
-    // protects access to SIGUSR2 handlers, not just SIGNALED
-    pub static ref SIGUSR2_MTX: Mutex<()> = Mutex::new(());
 }
 
 extern fn sigfunc(_: c_int) {
@@ -246,7 +243,8 @@ extern fn sigfunc(_: c_int) {
 #[test]
 #[cfg_attr(any(all(target_env = "musl", target_arch = "x86_64"), target_arch = "mips"), ignore)]
 fn test_write_sigev_signal() {
-    let _ = SIGUSR2_MTX.lock().expect("Mutex got poisoned by another test");
+    #[allow(unused_variables)]
+    let m = ::SIGUSR2_MTX.lock().expect("Mutex got poisoned by another test");
     let sa = SigAction::new(SigHandler::Handler(sigfunc),
                             SA_RESETHAND,
                             SigSet::empty());
@@ -376,7 +374,8 @@ fn test_lio_listio_nowait() {
 #[cfg(not(any(target_os = "ios", target_os = "macos")))]
 #[cfg_attr(any(target_arch = "mips", target_env = "musl"), ignore)]
 fn test_lio_listio_signal() {
-    let _ = SIGUSR2_MTX.lock().expect("Mutex got poisoned by another test");
+    #[allow(unused_variables)]
+    let m = ::SIGUSR2_MTX.lock().expect("Mutex got poisoned by another test");
     const INITIAL: &'static [u8] = b"abcdef123456";
     const WBUF: &'static [u8] = b"CDEF";
     let rbuf = Rc::new(vec![0; 4].into_boxed_slice());

test/sys/test_wait.rs

@@ -6,6 +6,9 @@ use libc::exit;
 
 #[test]
 fn test_wait_signal() {
+    #[allow(unused_variables)]
+    let m = ::FORK_MTX.lock().expect("Mutex got poisoned by another test");
+
     match fork() {
       Ok(Child) => pause().unwrap_or(()),
       Ok(Parent { child }) => {
@@ -19,6 +22,9 @@ fn test_wait_signal() {
 
 #[test]
 fn test_wait_exit() {
+    #[allow(unused_variables)]
+    let m = ::FORK_MTX.lock().expect("Mutex got poisoned by another test");
+
     match fork() {
       Ok(Child) => unsafe { exit(12); },
       Ok(Parent { child }) => {

test/test.rs

@@ -25,6 +25,7 @@ mod test_unistd;
 
 use nixtest::assert_size_of;
 use std::os::unix::io::RawFd;
+use std::sync::Mutex;
 use nix::unistd::read;
 
 /// Helper function analogous to std::io::Read::read_exact, but for `RawFD`s
@@ -37,6 +38,17 @@ fn read_exact(f: RawFd, buf: &mut  [u8]) {
     }
 }
 
+lazy_static! {
+    /// Any test that changes the process's current working directory must grab
+    /// this mutex
+    pub static ref CWD_MTX: Mutex<()> = Mutex::new(());
+    /// Any test that creates child processes must grab this mutex, regardless
+    /// of what it does with those children.
+    pub static ref FORK_MTX: Mutex<()> = Mutex::new(());
+    /// Any test that registers a SIGUSR2 handler must grab this mutex
+    pub static ref SIGUSR2_MTX: Mutex<()> = Mutex::new(());
+}
+
 #[test]
 pub fn test_size_of_long() {
     // This test is mostly here to ensure that 32bit CI is correctly

test/test_mq.rs

@@ -16,6 +16,8 @@ use nix::Error::Sys;
 
 #[test]
 fn test_mq_send_and_receive() {
+    #[allow(unused_variables)]
+    let m = ::FORK_MTX.lock().expect("Mutex got poisoned by another test");
 
     const MSG_SIZE: c_long =  32;
     let attr =  MqAttr::new(0, 10, MSG_SIZE, 0);

test/test_unistd.rs

@@ -9,16 +9,18 @@ use std::ffi::CString;
 use std::fs::File;
 use std::io::Write;
 use std::os::unix::prelude::*;
-use std::env::current_dir;
 use tempfile::tempfile;
 use tempdir::TempDir;
 use libc::off_t;
+use std::process::exit;
 
 #[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) => {} // ignore child here
+        Ok(Child) => exit(0),
         Ok(Parent { child }) => {
             // assert that child was created and pid > 0
             let child_raw: ::libc::pid_t = child.into();
@@ -29,10 +31,10 @@ fn test_fork_and_waitpid() {
                 Ok(WaitStatus::Exited(pid_t, _)) =>  assert!(pid_t == child),
 
                 // panic, must never happen
-                Ok(_) => panic!("Child still alive, should never happen"),
+                s @ Ok(_) => panic!("Child exited {:?}, should never happen", s),
 
                 // panic, waitpid should never fail
-                Err(_) => panic!("Error: waitpid Failed")
+                Err(s) => panic!("Error: waitpid returned Err({:?}", s)
             }
 
         },
@@ -43,9 +45,12 @@ fn test_fork_and_waitpid() {
 
 #[test]
 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");
     let pid = fork();
     match pid {
-        Ok(Child) => {} // ignore child here
+        Ok(Child) => exit(0),
         Ok(Parent { child }) => {
             let wait_status = wait();
 
@@ -100,6 +105,8 @@ macro_rules! execve_test_factory(
     ($test_name:ident, $syscall:ident, $unix_sh:expr, $android_sh:expr) => (
     #[test]
     fn $test_name() {
+        #[allow(unused_variables)]
+        let m = ::FORK_MTX.lock().expect("Mutex got poisoned by another test");
         // The `exec`d process will write to `writer`, and we'll read that
         // data from `reader`.
         let (reader, writer) = pipe().unwrap();
@@ -145,40 +152,43 @@ macro_rules! execve_test_factory(
 
 #[test]
 fn test_fchdir() {
+    // fchdir changes the process's cwd
+    #[allow(unused_variables)]
+    let m = ::CWD_MTX.lock().expect("Mutex got poisoned by another test");
+
     let tmpdir = TempDir::new("test_fchdir").unwrap();
     let tmpdir_path = tmpdir.path().canonicalize().unwrap();
     let tmpdir_fd = File::open(&tmpdir_path).unwrap().into_raw_fd();
-    let olddir_path = getcwd().unwrap();
-    let olddir_fd = File::open(&olddir_path).unwrap().into_raw_fd();
 
     assert!(fchdir(tmpdir_fd).is_ok());
     assert_eq!(getcwd().unwrap(), tmpdir_path);
 
-    assert!(fchdir(olddir_fd).is_ok());
-    assert_eq!(getcwd().unwrap(), olddir_path);
-
-    assert!(close(olddir_fd).is_ok());
     assert!(close(tmpdir_fd).is_ok());
 }
 
 #[test]
 fn test_getcwd() {
-    let tmp_dir = TempDir::new("test_getcwd").unwrap();
-    assert!(chdir(tmp_dir.path()).is_ok());
-    assert_eq!(getcwd().unwrap(), current_dir().unwrap());
-
-    // make path 500 chars longer so that buffer doubling in getcwd kicks in.
-    // Note: One path cannot be longer than 255 bytes (NAME_MAX)
-    // whole path cannot be longer than PATH_MAX (usually 4096 on linux, 1024 on macos)
-    let mut inner_tmp_dir = tmp_dir.path().to_path_buf();
+    // chdir changes the process's cwd
+    #[allow(unused_variables)]
+    let m = ::CWD_MTX.lock().expect("Mutex got poisoned by another test");
+
+    let tmpdir = TempDir::new("test_getcwd").unwrap();
+    let tmpdir_path = tmpdir.path().canonicalize().unwrap();
+    assert!(chdir(&tmpdir_path).is_ok());
+    assert_eq!(getcwd().unwrap(), tmpdir_path);
+
+    // make path 500 chars longer so that buffer doubling in getcwd
+    // kicks in.  Note: One path cannot be longer than 255 bytes
+    // (NAME_MAX) whole path cannot be longer than PATH_MAX (usually
+    // 4096 on linux, 1024 on macos)
+    let mut inner_tmp_dir = tmpdir_path.to_path_buf();
     for _ in 0..5 {
         let newdir = iter::repeat("a").take(100).collect::<String>();
-        //inner_tmp_dir = inner_tmp_dir.join(newdir).path();
         inner_tmp_dir.push(newdir);
         assert!(mkdir(inner_tmp_dir.as_path(), stat::S_IRWXU).is_ok());
     }
     assert!(chdir(inner_tmp_dir.as_path()).is_ok());
-    assert_eq!(getcwd().unwrap(), current_dir().unwrap());
+    assert_eq!(getcwd().unwrap(), inner_tmp_dir.as_path());
 }
 
 #[test]