diff --git a/AGENTS.md b/AGENTS.md index 2f013d49..b6994c8e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -40,6 +40,7 @@ - Never reach inside a poisoned lock. Use `expect` when the critical section cannot panic, or a fallback that leaves the data alone. Tests that serialize on process-wide state use `test_env::lock()` or a `test_env::Lease`. - `catch_unwind` is reserved for the reviewed worker-isolation sites that carry an `#[expect]` reason. Tests observe panics through a joined scoped thread. - Getters are `foo()`/`foo_mut()`, a fallible constructor is `try_new`, and type lifetimes have descriptive names. Every `unsafe` block has a `// SAFETY:` comment that says why it is sound. +- Make kernel calls through `std` or `rustix`, which need no `unsafe`, and rename on import when a module name clashes with `std` (`use rustix::fs as rustix_fs`). Use raw `libc` calls only where neither fits: signal handlers, the post-`clone` broker stub, `fail_stop`, the zero-signal pidfd check, the inotify header parser, `dup2` onto a fixed descriptor number, and `localtime_r`. `libc` constants stay fine. - Convert numbers losslessly with `From` (`f64::from(width)`) wherever it can be called; `const` initializers keep `as`. Take the middle of two values with `midpoint` (`f64::midpoint(a, b)`), which cannot overflow. - Inline named format arguments: `format!("{name}: {err}")`. - Use `map_or`, `map_or_else`, `is_some_and`, or `is_ok_and` instead of `map(..).unwrap_or(..)`. Use `let ... else` for a `match` that only unwraps or leaves, and `if let ... else` for a `match` with one pattern and a fallback. diff --git a/Cargo.toml b/Cargo.toml index f6b9622d..50f2d279 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -94,7 +94,7 @@ serde_ignored = "0.1" schemars = { version = "1.2", features = ["derive"], optional = true } libc = "0.2" # Safe wrappers for the kernel interfaces that the std library does not cover. -rustix = { version = "1", features = ["event", "fs", "net", "pipe", "process", "stdio", "termios", "thread", "time"] } +rustix = { version = "1", features = ["event", "fs", "net", "pipe", "process", "thread", "time"] } # Kernel random bytes for portal handle tokens and daemon protocol identifiers. getrandom = { version = "0.4", features = ["std"] } # Securely created, self-deleting temporary files for the OCR engine handoff. diff --git a/src/app/mod.rs b/src/app/mod.rs index 58e531bb..584d3991 100644 --- a/src/app/mod.rs +++ b/src/app/mod.rs @@ -22,10 +22,14 @@ use crate::{ }; use anyhow::Context; use env::env_flag_enabled; +#[cfg(unix)] +use rustix::process as rustix_process; use session::run_session_cli_commands; use std::ffi::OsString; use std::fs::{File, OpenOptions}; use std::io::ErrorKind; +#[cfg(unix)] +use std::io::IsTerminal; use std::path::{Path, PathBuf}; use usage::{log_overlay_controls, print_usage}; @@ -170,16 +174,17 @@ fn run_update_check() -> anyhow::Result<()> { #[cfg(unix)] fn detach_from_tty() { // Start a new session to drop the controlling terminal (prevents stuck shells). - // SAFETY: setsid takes no arguments; a failure only leaves the session as is. - unsafe { - let _ = libc::setsid(); - } + // A failure only leaves the session as is. + let _ = rustix_process::setsid(); // Point stdio that still refers to a TTY at /dev/null. Closing it instead // frees the descriptor number for the next open, and the logger's stderr // writes would then land in whatever file that open returned. - for fd in [libc::STDIN_FILENO, libc::STDOUT_FILENO, libc::STDERR_FILENO] { - // SAFETY: isatty only inspects the descriptor number it is given. - let is_tty = unsafe { libc::isatty(fd) } == 1; + let stdio = [ + (libc::STDIN_FILENO, io::stdin().is_terminal()), + (libc::STDOUT_FILENO, io::stdout().is_terminal()), + (libc::STDERR_FILENO, io::stderr().is_terminal()), + ]; + for (fd, is_tty) in stdio { if is_tty && let Err(error) = redirect_to_dev_null(fd) { log::warn!("Failed to detach descriptor {fd} from the terminal: {error}"); } @@ -414,6 +419,8 @@ mod tests { use std::io::Write; use std::os::fd::AsRawFd; + use rustix::io as rustix_io; + let temp = test_temp::tempdir().unwrap(); let path = temp.path().join("was-a-terminal"); let mut file = File::create(&path).unwrap(); @@ -422,8 +429,7 @@ mod tests { redirect_to_dev_null(file.as_raw_fd()).unwrap(); file.write_all(b"after").unwrap(); - // SAFETY: F_GETFD only inspects the descriptor the file still owns. - assert!(unsafe { libc::fcntl(file.as_raw_fd(), libc::F_GETFD) } >= 0); + rustix_io::fcntl_getfd(&file).unwrap(); assert_eq!(fs::read(&path).unwrap(), b"before "); } diff --git a/src/capture/tests/manager.rs b/src/capture/tests/manager.rs index e1edace2..623e9545 100644 --- a/src/capture/tests/manager.rs +++ b/src/capture/tests/manager.rs @@ -1,4 +1,3 @@ -use std::os::fd::AsRawFd; use std::path::PathBuf; use std::sync::atomic::{AtomicUsize, Ordering}; use std::sync::{Arc, Mutex, mpsc}; @@ -163,23 +162,19 @@ async fn completion_wakes_an_already_blocked_runtime_poll_after_publication() { .unwrap(); started.notified().await; - let wake_fd = runtime_wake.poll_fd().as_raw_fd(); let (polling_tx, polling_rx) = mpsc::sync_channel(0); let poller = thread::spawn(move || { - let mut pollfd = libc::pollfd { - fd: wake_fd, - events: libc::POLLIN, - revents: 0, - }; polling_tx.send(()).unwrap(); - // SAFETY: the runtime wake source outlives this bounded poll and owns - // the descriptor for the entire test. - assert_eq!(unsafe { libc::poll(&raw mut pollfd, 1, 1_000) }, 1); - assert_ne!(pollfd.revents & libc::POLLIN, 0); + assert!( + runtime_wake + .is_readable_within(Duration::from_secs(1)) + .unwrap() + ); + runtime_wake }); polling_rx.recv().unwrap(); release.notify_one(); - poller.join().unwrap(); + let runtime_wake = poller.join().unwrap(); runtime_wake.drain().unwrap(); assert!(matches!( diff --git a/src/daemon/protocol_v2/tests.rs b/src/daemon/protocol_v2/tests.rs index 4598d6f9..18231e3f 100644 --- a/src/daemon/protocol_v2/tests.rs +++ b/src/daemon/protocol_v2/tests.rs @@ -1,13 +1,22 @@ -use std::os::fd::AsRawFd; +use std::os::fd::{AsFd, BorrowedFd}; use std::process::Command; use std::time::{Duration, Instant}; use std::{env, process}; +use rustix::event::{self, PollFd, PollFlags, Timespec}; + use crate::tray_action::TrayAction; use crate::{env_vars, test_env, test_temp}; use super::*; +fn is_readable(fd: BorrowedFd<'_>, timeout: Duration) -> bool { + let mut pollfds = [PollFd::from_borrowed_fd(fd, PollFlags::IN)]; + let timeout = Timespec::try_from(timeout).unwrap(); + event::poll(&mut pollfds, Some(&timeout)).unwrap() == 1 + && pollfds[0].revents().contains(PollFlags::IN) +} + #[test] fn production_mode_is_closed_to_published_v2_after_cutover() { assert_eq!( @@ -71,15 +80,7 @@ fn boottime_deadline_source_arms_absolutely_and_drains_once() { .unwrap(); source.arm(deadline).unwrap(); - let mut poll_fd = libc::pollfd { - fd: source.poll_fd().as_raw_fd(), - events: libc::POLLIN, - revents: 0, - }; - // SAFETY: poll_fd remains valid for this bounded poll. - let ready = unsafe { libc::poll(&raw mut poll_fd, 1, 500) }; - assert_eq!(ready, 1); - assert_ne!(poll_fd.revents & libc::POLLIN, 0); + assert!(is_readable(source.poll_fd(), Duration::from_millis(500))); assert!(source.drain().unwrap()); assert!(!source.drain().unwrap()); source.disarm().unwrap(); @@ -128,14 +129,7 @@ fn pidfd_wait_times_out_while_alive_and_returns_once_exited() { #[test] fn self_pidfd_is_not_readable_while_process_is_alive() { let pidfd = linux::open_self_pidfd().unwrap(); - let mut poll_fd = libc::pollfd { - fd: pidfd.as_raw_fd(), - events: libc::POLLIN, - revents: 0, - }; - // SAFETY: poll_fd remains valid for the zero-timeout observation. - let ready = unsafe { libc::poll(&raw mut poll_fd, 1, 0) }; - assert_eq!(ready, 0); + assert!(!is_readable(pidfd.as_fd(), Duration::ZERO)); } #[test] @@ -189,13 +183,7 @@ fn command_queue_watch_reports_atomic_publication_and_rejects_replacement() { ) .unwrap(); - let mut poll_fd = libc::pollfd { - fd: watcher.poll_fd().as_raw_fd(), - events: libc::POLLIN, - revents: 0, - }; - // SAFETY: watcher owns the descriptor throughout the bounded poll. - assert_eq!(unsafe { libc::poll(&raw mut poll_fd, 1, 500) }, 1); + assert!(is_readable(watcher.poll_fd(), Duration::from_millis(500))); assert!(watcher.drain().unwrap().scan_pending); fs::rename(&queue, temp.path().join("old-queue")).unwrap(); diff --git a/src/process_broker/server.rs b/src/process_broker/server.rs index a000f40e..5b4646b2 100644 --- a/src/process_broker/server.rs +++ b/src/process_broker/server.rs @@ -633,7 +633,6 @@ mod tests { drop(ownership); - // SAFETY: signal zero only probes the test-owned provider after teardown. - assert_ne!(unsafe { libc::kill(pid, 0) }, 0); + assert!(rustix_process::test_kill_process(Pid::from_raw(pid).unwrap()).is_err()); } } diff --git a/src/process_broker/tests.rs b/src/process_broker/tests.rs index b801f27d..15005482 100644 --- a/src/process_broker/tests.rs +++ b/src/process_broker/tests.rs @@ -1,5 +1,5 @@ use std::ffi::OsStr; -use std::os::fd::{AsFd, AsRawFd}; +use std::os::fd::AsFd; use std::os::unix::process::CommandExt; use std::path::Path; use std::process::Command; @@ -7,10 +7,23 @@ use std::sync::mpsc; use std::time::{Duration, Instant}; use std::{env, fs, iter, thread}; +use rustix::net::{self, Shutdown}; +use rustix::process::{self as rustix_process, Pid, Signal}; + use crate::daemon::protocol_v2; use crate::test_fake_helper::Role; use crate::{env_vars, process_broker, test_env, test_fake_helper, test_temp}; +fn process_exists(pid: libc::pid_t) -> bool { + Pid::from_raw(pid).is_some_and(|pid| rustix_process::test_kill_process(pid).is_ok()) +} + +fn kill_for_cleanup(pid: libc::pid_t) { + if let Some(pid) = Pid::from_raw(pid) { + let _ = rustix_process::kill_process(pid, Signal::KILL); + } +} + use super::*; fn wire_arguments(arguments: &[&str]) -> Vec { @@ -28,10 +41,7 @@ fn release_test_provider(release_path: &Path, proof_path: &Path, provider_pid: i return; } if Instant::now() >= deadline { - // SAFETY: cleanup keeps a failing regression test from leaking its provider. - unsafe { - libc::kill(provider_pid, libc::SIGKILL); - } + kill_for_cleanup(provider_pid); panic!("successful provider could not act after normal broker shutdown"); } thread::sleep(Duration::from_millis(5)); @@ -540,8 +550,7 @@ fn process_group_guard_cleans_up_before_ownership_transfer() { drop(child); - // SAFETY: signal zero only probes the test-owned helper PID after guard cleanup. - assert_ne!(unsafe { libc::kill(pid, 0) }, 0); + assert!(!process_exists(pid)); } #[test] @@ -762,8 +771,7 @@ fn broker_guard_preempts_an_active_operation_and_kills_its_group() { "broker shutdown waited for the active operation" ); assert!(run.join().unwrap().is_err()); - // SAFETY: signal zero only probes the test-owned helper PID. - assert_ne!(unsafe { libc::kill(helper_pid, 0) }, 0); + assert!(!process_exists(helper_pid)); } #[test] @@ -891,10 +899,7 @@ fn a_descendant_outside_the_helper_group_cannot_hold_the_broker_on_its_pipes() { .ok() .and_then(|pid| pid.trim().parse::().ok()) { - // SAFETY: cleanup of the descendant this test started in its own session. - unsafe { - libc::kill(pid, libc::SIGKILL); - } + kill_for_cleanup(pid); } let error = result.expect_err("output still held open must not read as complete"); @@ -950,8 +955,7 @@ fn normal_broker_shutdown_releases_successful_provider_descendant() { .trim() .parse::() .unwrap(); - // SAFETY: signal zero only checks the test-owned provider. - assert_eq!(unsafe { libc::kill(provider_pid, 0) }, 0); + assert!(process_exists(provider_pid)); drop(guard); @@ -984,28 +988,19 @@ fn shutdown_channel_peer_loss_kills_retained_provider() { .trim() .parse::() .unwrap(); - // SAFETY: signal zero only checks the test-owned provider. - assert_eq!(unsafe { libc::kill(provider_pid, 0) }, 0); + assert!(process_exists(provider_pid)); // Simulate abrupt parent loss without writing the graceful-shutdown packet. - assert_eq!( - // SAFETY: the descriptor belongs to this test's live broker guard. - unsafe { libc::shutdown(guard.broker().inner.shutdown.as_raw_fd(), libc::SHUT_RDWR) }, - 0 - ); + net::shutdown(&guard.broker().inner.shutdown, Shutdown::Both).unwrap(); drop(guard); let deadline = Instant::now() + Duration::from_secs(1); loop { - // SAFETY: signal zero only probes the recorded test provider PID. - if unsafe { libc::kill(provider_pid, 0) } != 0 { + if !process_exists(provider_pid) { break; } if Instant::now() >= deadline { - // SAFETY: cleanup keeps a failing regression test from leaking its provider. - unsafe { - libc::kill(provider_pid, libc::SIGKILL); - } + kill_for_cleanup(provider_pid); panic!("provider survived abnormal broker channel loss"); } thread::sleep(Duration::from_millis(5)); @@ -1069,22 +1064,17 @@ fn retained_publication_replacement_disposes_the_previous_provider() { .unwrap(); let deadline = Instant::now() + Duration::from_secs(1); loop { - // SAFETY: signal zero only probes the recorded test provider PID. - if unsafe { libc::kill(first_pid, 0) } != 0 { + if !process_exists(first_pid) { break; } if Instant::now() >= deadline { - // SAFETY: cleanup keeps a failing regression test from leaking its helpers. - unsafe { - libc::kill(first_pid, libc::SIGKILL); - libc::kill(second_pid, libc::SIGKILL); - } + kill_for_cleanup(first_pid); + kill_for_cleanup(second_pid); panic!("replaced publication provider remained alive"); } thread::sleep(Duration::from_millis(5)); } - // SAFETY: signal zero only checks the current test-owned provider. - assert_eq!(unsafe { libc::kill(second_pid, 0) }, 0); + assert!(process_exists(second_pid)); drop(guard); release_test_provider(&second_release_path, &second_proof_path, second_pid); @@ -1146,20 +1136,15 @@ fn failed_publication_replacement_preserves_the_current_provider() { .trim() .parse::() .unwrap(); - // SAFETY: signal zero only checks the current test-owned provider. - assert_eq!(unsafe { libc::kill(current_pid, 0) }, 0); + assert!(process_exists(current_pid)); let deadline = Instant::now() + Duration::from_secs(1); loop { - // SAFETY: signal zero only probes the failed test provider PID. - if unsafe { libc::kill(failed_pid, 0) } != 0 { + if !process_exists(failed_pid) { break; } if Instant::now() >= deadline { - // SAFETY: cleanup keeps a failing regression test from leaking its helpers. - unsafe { - libc::kill(current_pid, libc::SIGKILL); - libc::kill(failed_pid, libc::SIGKILL); - } + kill_for_cleanup(current_pid); + kill_for_cleanup(failed_pid); panic!("failed replacement provider survived cleanup"); } thread::sleep(Duration::from_millis(5)); @@ -1205,8 +1190,7 @@ fn retained_publication_kills_failed_or_input_stalled_provider_groups() { } let deadline = Instant::now() + Duration::from_secs(1); loop { - // SAFETY: signal zero only probes the recorded test child PID. - if unsafe { libc::kill(provider_pid, 0) } != 0 { + if !process_exists(provider_pid) { break; } assert!( @@ -1307,8 +1291,7 @@ fn broker_shutdown_preempts_retained_publication_stdin_writer() { release_test_provider(¤t_release_path, ¤t_proof_path, current_pid); let deadline = Instant::now() + Duration::from_secs(1); loop { - // SAFETY: signal zero only probes the recorded test child PID. - if unsafe { libc::kill(provider_pid, 0) } != 0 { + if !process_exists(provider_pid) { break; } assert!( diff --git a/src/test_fake_helper.rs b/src/test_fake_helper.rs index 6eac2008..61f7c441 100644 --- a/src/test_fake_helper.rs +++ b/src/test_fake_helper.rs @@ -15,6 +15,8 @@ use std::os::unix::fs as unix_fs; use std::path::{Path, PathBuf}; use std::{env, fs, io, process}; +use rustix::process as rustix_process; + use anyhow::{Context, Result}; use crate::durable_io; @@ -124,8 +126,7 @@ fn play(arguments: &[OsString]) -> Result<()> { let report: Vec = match role { Role::Exit => return Ok(()), Role::ReportProcessGroup => { - // SAFETY: getpgrp has no preconditions and cannot fail. - let leader = unsafe { libc::getpgrp() } == process::id() as libc::pid_t; + let leader = rustix_process::getpgrp().as_raw_pid() == process::id() as libc::pid_t; if leader { "process-group-leader" } else { diff --git a/src/unix_signals/listener.rs b/src/unix_signals/listener.rs index a0328dd4..919e0004 100644 --- a/src/unix_signals/listener.rs +++ b/src/unix_signals/listener.rs @@ -7,6 +7,9 @@ use std::sync::{Arc, Mutex}; use std::thread::{self, JoinHandle}; use std::{fmt, io}; +#[cfg(test)] +use rustix::thread as rustix_thread; + use super::{ SIGNAL_WRITE_FD, begin_listener_teardown, clear_registered_signals, create_pipe, dispatch_pending_signals, finish_listener_teardown, install_handler, publish_listener_active, @@ -259,9 +262,9 @@ impl Drop for SignalListener { fn listener_thread(read_fd: RawFd, shared: &ListenerShared) { #[cfg(test)] { - // SAFETY: gettid has no preconditions and is used only by tests to - // observe that this exact thread reached its blocking read. - let tid = unsafe { libc::syscall(libc::SYS_gettid) as libc::pid_t }; + // Used only by tests to observe that this exact thread reached its + // blocking read. + let tid = rustix_thread::gettid().as_raw_pid(); shared.thread_tid.store(tid, Ordering::Release); }