Merge #638

638: Make aio, chdir, and wait tests thread safe r=Susurrus

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: bors[bot]

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,43 +16,27 @@ use nix::Error::Sys;
 
 #[test]
 fn test_mq_send_and_receive() {
-
     const MSG_SIZE: c_long =  32;
     let attr =  MqAttr::new(0, 10, MSG_SIZE, 0);
-    let mq_name_in_parent = &CString::new(b"/a_nix_test_queue".as_ref()).unwrap();
-    let mqd_in_parent = mq_open(mq_name_in_parent, O_CREAT | O_WRONLY, S_IWUSR | S_IRUSR | S_IRGRP | S_IROTH, Some(&attr)).unwrap();
-    let msg_to_send = "msg_1".as_bytes();
-
-    mq_send(mqd_in_parent, msg_to_send, 1).unwrap();
-
-    let (reader, writer) = pipe().unwrap();
-
-    let pid = fork();
-    match pid {
-        Ok(Child) => {
-            let mq_name_in_child =  &CString::new(b"/a_nix_test_queue".as_ref()).unwrap();
-            let mqd_in_child = mq_open(mq_name_in_child, O_CREAT | O_RDONLY, S_IWUSR | S_IRUSR | S_IRGRP | S_IROTH, Some(&attr)).unwrap();
-            let mut buf = [0u8; 32];
-            let mut prio = 0u32;
-            mq_receive(mqd_in_child, &mut buf, &mut prio).unwrap();
-            assert!(prio == 1);
-            write(writer, &buf).unwrap();  // pipe result to parent process. Otherwise cargo does not report test failures correctly
-            mq_close(mqd_in_child).unwrap();
-      }
-      Ok(Parent { child }) => {
-          mq_close(mqd_in_parent).unwrap();
-
-          // Wait for the child to exit.
-          waitpid(child, None).unwrap();
-          // Read 1024 bytes.
-          let mut read_buf = [0u8; 32];
-          read(reader, &mut read_buf).unwrap();
-          let message_str = str::from_utf8(&read_buf).unwrap();
-          assert_eq!(&message_str[.. message_str.char_indices().nth(5).unwrap().0], "msg_1");
-      },
-      // panic, fork should never fail unless there is a serious problem with the OS
-      Err(_) => panic!("Error: Fork Failed")
-    }
+    let mq_name= &CString::new(b"/a_nix_test_queue".as_ref()).unwrap();
+
+    let mqd0 = mq_open(mq_name, O_CREAT | O_WRONLY,
+                       S_IWUSR | S_IRUSR | S_IRGRP | S_IROTH,
+                       Some(&attr)).unwrap();
+    let msg_to_send = "msg_1";
+    mq_send(mqd0, msg_to_send.as_bytes(), 1).unwrap();
+
+    let mqd1 = mq_open(mq_name, O_CREAT | O_RDONLY,
+                       S_IWUSR | S_IRUSR | S_IRGRP | S_IROTH,
+                       Some(&attr)).unwrap();
+    let mut buf = [0u8; 32];
+    let mut prio = 0u32;
+    let len = mq_receive(mqd1, &mut buf, &mut prio).unwrap();
+    assert!(prio == 1);
+
+    mq_close(mqd1).unwrap();
+    mq_close(mqd0).unwrap();
+    assert_eq!(msg_to_send, str::from_utf8(&buf[0..len]).unwrap());
 }
 
 

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]