Skip to content

Commit

Permalink
Make sure panic after fork reliably causes the test to fail (#877)
Browse files Browse the repository at this point in the history
  • Loading branch information
squell authored Oct 15, 2024
2 parents 3900363 + 8643990 commit 55ec501
Show file tree
Hide file tree
Showing 2 changed files with 33 additions and 14 deletions.
43 changes: 31 additions & 12 deletions src/system/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -129,11 +129,9 @@ pub(crate) enum ForkResult {
///
/// # Safety
///
/// In a multithreaded program, only async-signal-safe functions are guaranteed to work in the
/// child process until a call to `execve` or a similar function is done.
/// Must not be called in multithreaded programs.
pub(crate) unsafe fn fork() -> io::Result<ForkResult> {
// FIXME use std::panic::always_abort() once it is stable if the current
// process is multi-threaded.
// FIXME add debug assertion that we are not currently using multiple threads.
let pid = cerr(unsafe { libc::fork() })?;
if pid == 0 {
Ok(ForkResult::Child)
Expand All @@ -142,6 +140,27 @@ pub(crate) unsafe fn fork() -> io::Result<ForkResult> {
}
}

/// Create a new process with extra precautions for usage in tests.
///
/// # Safety
///
/// In a multithreaded program, only async-signal-safe functions are guaranteed to work in the
/// child process until a call to `execve` or a similar function is done.
#[cfg(test)]
unsafe fn fork_for_test() -> ForkResult {
let result = fork().unwrap();
if let ForkResult::Child = result {
// Make sure that panics in the child always abort the process if it doesn't deadlock.
// FIXME use std::panic::always_abort() once it is stable
std::panic::set_hook(Box::new(|info| {
use std::io::Write;
let _ = writeln!(std::io::stderr(), "{info}");
std::process::exit(101);
}));
}
result
}

pub fn setsid() -> io::Result<ProcessId> {
// SAFETY: this function is memory-safe to call
cerr(unsafe { libc::setsid() })
Expand Down Expand Up @@ -704,7 +723,7 @@ mod tests {
use libc::SIGKILL;

use super::{
fork, getpgrp, setpgid,
fork_for_test, getpgrp, setpgid,
wait::{Wait, WaitOptions},
ForkResult, Group, User, WithProcess, ROOT_GROUP_NAME,
};
Expand Down Expand Up @@ -814,7 +833,7 @@ mod tests {

// FIXME fork will deadlock when this test panics if it forked while
// another test was panicking.
match unsafe { super::fork().unwrap() } {
match unsafe { super::fork_for_test() } {
ForkResult::Child => {
// wait for the parent.
std::thread::sleep(std::time::Duration::from_secs(1))
Expand Down Expand Up @@ -845,15 +864,15 @@ mod tests {

// FIXME fork will deadlock when this test panics if it forked while
// another test was panicking.
let ForkResult::Parent(pid1) = (unsafe { fork().unwrap() }) else {
let ForkResult::Parent(pid1) = (unsafe { fork_for_test() }) else {
std::thread::sleep(std::time::Duration::from_secs(1));
tx.write_all(&[42]).unwrap();
exit(0);
};

// FIXME fork will deadlock when this test panics if it forked while
// another test was panicking.
let ForkResult::Parent(pid2) = (unsafe { fork().unwrap() }) else {
let ForkResult::Parent(pid2) = (unsafe { fork_for_test() }) else {
std::thread::sleep(std::time::Duration::from_secs(1));
tx.write_all(&[42]).unwrap();
exit(0);
Expand Down Expand Up @@ -883,13 +902,13 @@ mod tests {
fn close_the_universe() {
// FIXME fork will deadlock when this test panics if it forked while
// another test was panicking.
let ForkResult::Parent(child_pid) = (unsafe { fork().unwrap() }) else {
let ForkResult::Parent(child_pid) = (unsafe { fork_for_test() }) else {
let should_close =
std::fs::File::open(std::env::temp_dir().join("should_close.txt")).unwrap();
std::fs::File::create(std::env::temp_dir().join("should_close.txt")).unwrap();
assert!(!is_closed(&should_close));

let should_not_close =
std::fs::File::open(std::env::temp_dir().join("should_not_close.txt")).unwrap();
std::fs::File::create(std::env::temp_dir().join("should_not_close.txt")).unwrap();
assert!(!is_closed(&should_not_close));

let mut closer = super::FileCloser::new();
Expand All @@ -916,7 +935,7 @@ mod tests {
fn except_stdio_is_fine() {
// FIXME fork will deadlock when this test panics if it forked while
// another test was panicking.
let ForkResult::Parent(child_pid) = (unsafe { fork().unwrap() }) else {
let ForkResult::Parent(child_pid) = (unsafe { fork_for_test() }) else {
let mut closer = super::FileCloser::new();

closer.except(&io::stdin());
Expand Down
4 changes: 2 additions & 2 deletions src/system/term/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,7 @@ mod tests {
process::exit,
};

use crate::system::{fork, getpgid, setsid, term::*, ForkResult};
use crate::system::{fork_for_test, getpgid, setsid, term::*, ForkResult};

#[test]
fn open_pty() {
Expand All @@ -238,7 +238,7 @@ mod tests {

// FIXME fork will deadlock when this test panics if it forked while
// another test was panicking.
let ForkResult::Parent(_) = (unsafe { fork().unwrap() }) else {
let ForkResult::Parent(_) = (unsafe { fork_for_test() }) else {
// Open a new pseudoterminal.
let leader = Pty::open().unwrap().leader;
// The pty leader should not have a foreground process group yet.
Expand Down

0 comments on commit 55ec501

Please sign in to comment.