From 64f798492c5d7abfef8be9837ae0703cda43b48e Mon Sep 17 00:00:00 2001 From: Alan Somers Date: Fri, 30 Jun 2017 10:49:18 -0600 Subject: [PATCH 1/2] 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 --- test/sys/test_aio.rs | 9 ++++---- test/sys/test_wait.rs | 6 +++++ test/test.rs | 12 ++++++++++ test/test_mq.rs | 2 ++ test/test_unistd.rs | 52 ++++++++++++++++++++++++++----------------- 5 files changed, 55 insertions(+), 26 deletions(-) diff --git a/test/sys/test_aio.rs b/test/sys/test_aio.rs index 7945aaf840..7e2bef63fe 100644 --- a/test/sys/test_aio.rs +++ b/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()); diff --git a/test/sys/test_wait.rs b/test/sys/test_wait.rs index d8ac94e487..2e28d9e78f 100644 --- a/test/sys/test_wait.rs +++ b/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 }) => { diff --git a/test/test.rs b/test/test.rs index 2cf3036054..4c81aa2b73 100644 --- a/test/test.rs +++ b/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 diff --git a/test/test_mq.rs b/test/test_mq.rs index 82a1a388f7..36af3066af 100644 --- a/test/test_mq.rs +++ b/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); diff --git a/test/test_unistd.rs b/test/test_unistd.rs index 22aa5da887..d5e58fd5f7 100644 --- a/test/test_unistd.rs +++ b/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::(); - //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] From 2cd4420279fdf3f45ff443b8fe0f715eb0c18424 Mon Sep 17 00:00:00 2001 From: Alan Somers Date: Sun, 16 Jul 2017 15:51:19 -0600 Subject: [PATCH 2/2] Don't fork in test_mq_send_receive It isn't necessary, and can cause deadlocks in Rust's test harness --- test/test_mq.rs | 56 +++++++++++++++++-------------------------------- 1 file changed, 19 insertions(+), 37 deletions(-) diff --git a/test/test_mq.rs b/test/test_mq.rs index 36af3066af..60f5bd2e48 100644 --- a/test/test_mq.rs +++ b/test/test_mq.rs @@ -16,45 +16,27 @@ 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); - 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()); }