artial, broken

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"

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"

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;

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(())
+}
+

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<Command> {
 pub fn spawn_exec(paths: &Paths, exec_args: &[String]) -> Result<Child> {
     #[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<Child> {
 pub fn spawn_repl(paths: &Paths, repl_args: &[String]) -> Result<Child> {
     #[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)
     }