From ffe7e2277fa6c9dbeabea1599305aa47fa319978 Mon Sep 17 00:00:00 2001 From: Ryan Ragona Date: Sun, 27 Apr 2025 12:42:14 -0700 Subject: [PATCH] artial, broken --- codex-rs/Cargo.lock | 15 +++++- codex-rs/session/Cargo.toml | 3 +- codex-rs/session/src/lib.rs | 1 + codex-rs/session/src/sig.rs | 29 ++++++++++++ codex-rs/session/src/spawn.rs | 86 +++++++++++++++++++++++------------ 5 files changed, 102 insertions(+), 32 deletions(-) create mode 100644 codex-rs/session/src/sig.rs diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 22e7eb5975..9572b8afe4 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -588,11 +588,12 @@ dependencies = [ "codex-core", "codex-exec", "codex-repl", + "command-group", "dirs", "humansize", "libc", "names", - "nix 0.27.1", + "nix 0.28.0", "petname", "rand 0.9.1", "serde", @@ -660,6 +661,18 @@ version = "1.0.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "5b63caa9aa9397e2d9480a9b13673856c78d8ac123288526c37d7839f2a86990" +[[package]] +name = "command-group" +version = "5.0.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a68fa787550392a9d58f44c21a3022cfb3ea3e2458b7f85d3b399d0ceeccf409" +dependencies = [ + "async-trait", + "nix 0.27.1", + "tokio", + "winapi", +] + [[package]] name = "compact_str" version = "0.8.1" diff --git a/codex-rs/session/Cargo.toml b/codex-rs/session/Cargo.toml index 3ac65b386b..4a4c279c78 100644 --- a/codex-rs/session/Cargo.toml +++ b/codex-rs/session/Cargo.toml @@ -32,7 +32,7 @@ dirs = "6" sysinfo = "0.29" tabwriter = "1.3" names = { version = "0.14", default-features = false } -nix = { version = "0.27", default-features = false, features = ["process", "signal", "term", "fs"] } +nix = { version = "0.28", default-features = false, features = ["process", "signal", "term", "fs"] } petname = "2.0.2" rand = "0.9.1" @@ -40,6 +40,7 @@ rand = "0.9.1" codex_exec = { package = "codex-exec", path = "../exec" } codex_repl = { package = "codex-repl", path = "../repl" } humansize = "2.1.3" +command-group = { version = "5.0.1", features = ["with-tokio"] } [dev-dependencies] tempfile = "3" diff --git a/codex-rs/session/src/lib.rs b/codex-rs/session/src/lib.rs index 6ebacae0b0..bd2ddf9d39 100644 --- a/codex-rs/session/src/lib.rs +++ b/codex-rs/session/src/lib.rs @@ -9,6 +9,7 @@ pub mod build; pub mod cli; pub mod meta; mod spawn; +mod sig; pub mod store; pub use cli::Cli; diff --git a/codex-rs/session/src/sig.rs b/codex-rs/session/src/sig.rs new file mode 100644 index 0000000000..21a27418db --- /dev/null +++ b/codex-rs/session/src/sig.rs @@ -0,0 +1,29 @@ +//! Small safe wrappers around a handful of `nix::sys::signal` calls that are +//! considered `unsafe` by the `nix` crate. By concentrating the `unsafe` blocks +//! in a single, well-audited module we can keep the rest of the codebase — and +//! in particular `spawn.rs` — entirely `unsafe`-free. + +#[cfg(unix)] +use nix::sys::signal::{signal as nix_signal, SigHandler, Signal}; + +/// Safely ignore `SIGHUP` for the current process. +/// +/// Internally this delegates to `nix::sys::signal::signal(…, SigIgn)` which is +/// marked *unsafe* because changing signal handlers can break invariants in +/// foreign code. In our very controlled environment we *only* ever install the +/// predefined, always-safe `SIG_IGN` handler, which is guaranteed not to cause +/// undefined behaviour. Therefore it is sound to wrap the call in `unsafe` and +/// expose it as a safe function. +#[cfg(unix)] +pub fn ignore_sighup() -> nix::Result<()> { + // SAFETY: Installing the built-in `SIG_IGN` handler is always safe. + unsafe { nix_signal(Signal::SIGHUP, SigHandler::SigIgn) }.map(|_| ()) +} + +#[cfg(not(unix))] +#[allow(clippy::unused_io_amount)] +pub fn ignore_sighup() -> std::io::Result<()> { + // No-op on non-Unix platforms. + Ok(()) +} + diff --git a/codex-rs/session/src/spawn.rs b/codex-rs/session/src/spawn.rs index d74bcc63d7..b65f2d2419 100644 --- a/codex-rs/session/src/spawn.rs +++ b/codex-rs/session/src/spawn.rs @@ -7,6 +7,22 @@ use std::fs::OpenOptions; use tokio::process::Child; use tokio::process::Command; +// ------------------------------------------------------------------------- +// Additional (Unix-only) imports to replace the former unsafe `libc` calls. +// These are guarded by `cfg(unix)` so Windows builds are completely unaffected. +// ------------------------------------------------------------------------- +#[cfg(unix)] +use command_group::AsyncCommandGroup; // provides `group_spawn` for tokio::process::Command + +#[cfg(unix)] +use nix::{ + errno::Errno, + sys::{ + stat::Mode, + }, + unistd::mkfifo, +}; + /// Open (and create if necessary) the log files that stdout / stderr of the /// spawned agent will be redirected to. fn open_log_files(paths: &Paths) -> Result<(std::fs::File, std::fs::File)> { @@ -39,28 +55,36 @@ fn base_command(bin: &str, paths: &Paths) -> Result { pub fn spawn_exec(paths: &Paths, exec_args: &[String]) -> Result { #[cfg(unix)] { - use std::io; + // ----------------------------------------------------------------- + // UNIX IMPLEMENTATION (now 100 % safe) + // ----------------------------------------------------------------- + // Build the base command and add the user-supplied arguments. let mut cmd = base_command("codex-exec", paths)?; cmd.args(exec_args); // Replace the `stdin` that `base_command` configured (null) with - // `/dev/null` opened for reading -- keeps the previous behaviour while + // `/dev/null` opened for reading – keeps the previous behaviour while // still leveraging the common helper. let stdin = OpenOptions::new().read(true).open("/dev/null")?; cmd.stdin(stdin); - unsafe { - cmd.pre_exec(|| { - if libc::setsid() == -1 { - return Err(io::Error::last_os_error()); - } - libc::signal(libc::SIGHUP, libc::SIG_IGN); - Ok(()) - }); - } + // Spawn the child as a *process group* / new session leader. + // `group_spawn()` internally performs the traditional + // 1. `fork()` + // 2. `setsid()` + // 3. `execvp()` + // sequence that we previously had to code manually via an unsafe + // `pre_exec` closure. + let child = cmd + .group_spawn() // <- safe wrapper from the `command-group` crate + .context("failed to spawn codex-exec")? + .into_inner(); // convert AsyncGroupChild -> tokio::process::Child + + // Ignore SIGHUP in the parent, mirroring the behaviour of the previous + // unsafe `libc::signal` call. + crate::sig::ignore_sighup()?; - let child = cmd.spawn().context("failed to spawn codex-exec")?; Ok(child) } @@ -83,39 +107,41 @@ pub fn spawn_exec(paths: &Paths, exec_args: &[String]) -> Result { pub fn spawn_repl(paths: &Paths, repl_args: &[String]) -> Result { #[cfg(unix)] { - use std::io; - use std::os::unix::ffi::OsStrExt; + // ----------------------------------------------------------------- + // UNIX IMPLEMENTATION (now 100 % safe) + // ----------------------------------------------------------------- + // Ensure a FIFO exists at `paths.stdin` with permissions rw------- if !paths.stdin.exists() { - let c_path = std::ffi::CString::new(paths.stdin.as_os_str().as_bytes()).unwrap(); - let res = unsafe { libc::mkfifo(c_path.as_ptr(), 0o600) }; - if res != 0 { - let err = std::io::Error::last_os_error(); - if err.kind() != io::ErrorKind::AlreadyExists { - return Err(err).context("mkfifo failed"); + if let Err(e) = mkfifo(&paths.stdin, Mode::from_bits_truncate(0o600)) { + // If the FIFO already exists we silently accept, just as the + // previous implementation did. + if e != Errno::EEXIST { + return Err(std::io::Error::from(e)).context("mkfifo failed"); } } } + // Open the FIFO for *both* reading and writing so we don't deadlock + // when there is no writer yet (mimics the previous behaviour). let stdin = OpenOptions::new() .read(true) .write(true) .open(&paths.stdin)?; + // Build the command. let mut cmd = base_command("codex-repl", paths)?; cmd.args(repl_args).stdin(stdin); - unsafe { - cmd.pre_exec(|| { - if libc::setsid() == -1 { - return Err(io::Error::last_os_error()); - } - libc::signal(libc::SIGHUP, libc::SIG_IGN); - Ok(()) - }); - } + // Detached spawn. + let child = cmd + .group_spawn() + .context("failed to spawn codex-repl")? + .into_inner(); + + // Ignore SIGHUP as before. + crate::sig::ignore_sighup()?; - let child = cmd.spawn().context("failed to spawn codex-repl")?; Ok(child) }