diff --git a/codex-rs/exec-server/src/fs_sandbox.rs b/codex-rs/exec-server/src/fs_sandbox.rs index 6ad7b56f30..cb04e811a9 100644 --- a/codex-rs/exec-server/src/fs_sandbox.rs +++ b/codex-rs/exec-server/src/fs_sandbox.rs @@ -25,12 +25,17 @@ use codex_utils_path_uri::LegacyAppPathString; #[cfg(any(windows, test))] use codex_utils_path_uri::PathConvention; use codex_utils_path_uri::PathUri; +use codex_utils_pty::Child; +use codex_utils_pty::ChildStdin; +use codex_utils_pty::Command; +#[cfg(target_os = "macos")] +use codex_utils_pty::DescriptorPolicy; +use codex_utils_pty::SpawnFallback; #[cfg(any(windows, test))] use tokio::io::AsyncBufReadExt; #[cfg(any(windows, test))] use tokio::io::AsyncReadExt; use tokio::io::AsyncWriteExt; -use tokio::process::Command; use crate::ExecServerRuntimePaths; use crate::FileSystemSandboxContext; @@ -367,7 +372,7 @@ async fn run_command( command: SandboxExecRequest, request_json: Vec, ) -> Result { - let mut child = spawn_command(command, std::process::Stdio::piped())?; + let mut child = spawn_command(command, ChildStdin::Piped)?; let mut stdin = child .stdin .take() @@ -426,7 +431,7 @@ pub(crate) async fn read_helper_response( #[cfg(any(windows, test))] pub(crate) fn drain_helper_stderr( - child: &mut tokio::process::Child, + child: &mut Child, ) -> tokio::task::JoinHandle, std::io::Error>> { let stderr_pipe = child.stderr.take(); tokio::spawn(async move { @@ -444,7 +449,7 @@ pub(crate) fn drain_helper_stderr( #[cfg(any(windows, test))] pub(crate) async fn reap_helper_after_response( - mut child: tokio::process::Child, + mut child: Child, stderr: tokio::task::JoinHandle, std::io::Error>>, ) -> Result<(), JSONRPCErrorError> { let (status, stderr) = match tokio::time::timeout(FS_HELPER_EXIT_TIMEOUT, async { @@ -477,7 +482,7 @@ pub(crate) async fn reap_helper_after_response( #[cfg(not(windows))] pub(crate) async fn wait_for_helper_output( - child: tokio::process::Child, + child: Child, ) -> Result { let output = child.wait_with_output().await.map_err(io_error)?; if !output.status.success() { @@ -498,8 +503,8 @@ pub(crate) fn spawn_command( arg0, .. }: SandboxExecRequest, - stdin: std::process::Stdio, -) -> Result { + stdin: ChildStdin, +) -> Result { let Some((program, args)) = argv.split_first() else { return Err(invalid_request("fs sandbox command was empty".to_string())); }; @@ -515,21 +520,13 @@ pub(crate) fn spawn_command( let cwd = cwd.to_abs_path().map_err(io_error)?; command.current_dir(cwd.as_path()); env.retain(|name, _| !codex_protocol::shell_environment::is_non_inheritable_env_var(name)); - command.env_clear(); command.envs(env); command.stdin(stdin); - command.stdout(std::process::Stdio::piped()); - command.stderr(std::process::Stdio::piped()); - command.kill_on_drop(true); + // A helper is a known executable: native launch errors must not retry through fork. + command.fallback(SpawnFallback::ReturnError); // macOS cannot receive passed fds with close-on-exec set atomically. #[cfg(target_os = "macos")] - // SAFETY: Descriptor cleanup only uses fork-safe system calls. - unsafe { - command.pre_exec(|| { - codex_utils_pty::pty::close_inherited_fds_except(&[]); - Ok(()) - }); - } + command.descriptor_policy(DescriptorPolicy::StdioOnly); command.spawn().map_err(io_error) } diff --git a/codex-rs/exec-server/src/fs_sandbox_windows_tests.rs b/codex-rs/exec-server/src/fs_sandbox_windows_tests.rs index c86cd09c41..fce05b15b9 100644 --- a/codex-rs/exec-server/src/fs_sandbox_windows_tests.rs +++ b/codex-rs/exec-server/src/fs_sandbox_windows_tests.rs @@ -2,7 +2,6 @@ use std::collections::HashMap; #[cfg(windows)] use std::path::Path; -use std::process::Stdio; use std::time::Duration; #[cfg(windows)] @@ -15,11 +14,11 @@ use codex_sandboxing::SandboxExecRequest; use codex_sandboxing::SandboxType; #[cfg(windows)] use codex_utils_path_uri::PathUri; +use codex_utils_pty::Command; use pretty_assertions::assert_eq; #[cfg(windows)] use tokio::io::AsyncReadExt; use tokio::io::AsyncWriteExt; -use tokio::process::Command; use super::drain_helper_stderr; use super::read_helper_response; @@ -85,9 +84,7 @@ async fn noisy_failing_helper_preserves_exit_status_and_bounded_stderr() { .arg("[Console]::Error.Write('expected helper diagnostic' + ('x' * 131072)); exit 7"); command }; - command.stdout(Stdio::null()); - command.stderr(Stdio::piped()); - command.kill_on_drop(true); + command.envs(std::env::vars_os()); let mut child = command.spawn().expect("noisy helper process"); let stderr = drain_helper_stderr(&mut child); @@ -132,9 +129,7 @@ async fn helper_stderr_is_drained_before_the_response() { ); command }; - command.stdout(Stdio::piped()); - command.stderr(Stdio::piped()); - command.kill_on_drop(true); + command.envs(std::env::vars_os()); let mut child = command.spawn().expect("noisy helper process"); let stdout = child.stdout.take().expect("helper stdout"); let stderr = drain_helper_stderr(&mut child); diff --git a/codex-rs/exec-server/src/sandboxed_file_open.rs b/codex-rs/exec-server/src/sandboxed_file_open.rs index 9cb4c75537..5752576a45 100644 --- a/codex-rs/exec-server/src/sandboxed_file_open.rs +++ b/codex-rs/exec-server/src/sandboxed_file_open.rs @@ -58,7 +58,7 @@ async fn open_platform( let (mut receiver, sender) = UnixStream::pair().map_err(io_error)?; let sender: OwnedFd = sender.into(); - let child = spawn_command(command, std::process::Stdio::from(sender))?; + let child = spawn_command(command, codex_utils_pty::ChildStdin::File(sender))?; receiver.write_all(&request).map_err(io_error)?; receiver .shutdown(std::net::Shutdown::Write) @@ -78,7 +78,7 @@ async fn open_platform( ) -> Result { use tokio::io::AsyncWriteExt; - let mut child = spawn_command(command, std::process::Stdio::piped())?; + let mut child = spawn_command(command, codex_utils_pty::ChildStdin::Piped)?; let mut stdin = child .stdin .take() diff --git a/codex-rs/exec-server/tests/macos_fs_spawn.rs b/codex-rs/exec-server/tests/macos_fs_spawn.rs new file mode 100644 index 0000000000..f261c0c7d5 --- /dev/null +++ b/codex-rs/exec-server/tests/macos_fs_spawn.rs @@ -0,0 +1,170 @@ +//! Exercise native filesystem-helper spawning with real sandbox and fd-transfer operations. +//! Atfork registration stays in an isolated process because its hooks cannot be removed. + +#![cfg(target_os = "macos")] + +mod common; +#[path = "file_system/support.rs"] +#[allow(dead_code, clippy::expect_used)] +mod support; + +use std::io; +use std::os::unix::fs::symlink; +use std::os::unix::process::CommandExt; +use std::sync::atomic::AtomicUsize; +use std::sync::atomic::Ordering; +use std::time::Duration; + +use anyhow::Context; +use anyhow::Result; +use codex_utils_path_uri::PathUri; +use futures::TryStreamExt; +use pretty_assertions::assert_eq; +use tokio::process::Command; +use tokio::time::timeout; + +use crate::support::FileSystemImplementation; +use crate::support::create_file_system_context; +use crate::support::workspace_write_sandbox; + +const CHILD_FLAG: &str = "CODEX_EXEC_SERVER_MACOS_FS_SPAWN_TEST_CHILD"; +const CHILD_COMPLETED: &str = "filesystem spawn assertions completed"; +static PARENT_FORKS: AtomicUsize = AtomicUsize::new(0); + +extern "C" fn record_parent_fork() { + PARENT_FORKS.fetch_add(1, Ordering::Relaxed); +} + +#[tokio::test] +async fn sandboxed_filesystem_operations_avoid_fork() -> Result<()> { + let mut command = Command::new(std::env::current_exe()?); + command + .args([ + "--exact", + "sandboxed_filesystem_operations_avoid_fork_child", + "--ignored", + "--test-threads=1", + "--nocapture", + ]) + .env(CHILD_FLAG, "1") + .kill_on_drop(true); + let output = timeout(Duration::from_secs(/*secs*/ 60), command.output()).await??; + assert!( + output.status.success() + && String::from_utf8_lossy(&output.stdout).contains(CHILD_COMPLETED), + "filesystem spawn test failed:\n{}\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr), + ); + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +#[ignore = "isolated child for sandboxed_filesystem_operations_avoid_fork"] +async fn sandboxed_filesystem_operations_avoid_fork_child() -> Result<()> { + assert_eq!(std::env::var(CHILD_FLAG)?, "1"); + let context = create_file_system_context(FileSystemImplementation::Local).await?; + let file_system = &context.file_system; + let directory = tempfile::tempdir()?; + let root = directory.path().canonicalize()?; + let allowed = root.join("allowed"); + let outside = root.join("outside"); + std::fs::create_dir(&allowed)?; + std::fs::create_dir(&outside)?; + let secret = outside.join("secret.txt"); + std::fs::write(&secret, b"secret")?; + symlink(&outside, allowed.join("escape"))?; + let sandbox = workspace_write_sandbox(allowed.clone()); + let path = allowed.join("file.txt"); + let uri = PathUri::from_host_native_path(&path)?; + + // SAFETY: This isolated process runs one test, and the parent hook only updates a + // lock-free atomic. The hook remains valid until this process exits. + let registered = unsafe { + libc::pthread_atfork( + /*prepare*/ None, + Some(record_parent_fork), + /*child*/ None, + ) + }; + assert_eq!(registered, 0); + let mut legacy = std::process::Command::new("/usr/bin/true"); + // SAFETY: A no-op callback forces the legacy fork route for this positive control. + unsafe { + legacy.pre_exec(|| Ok(())); + } + assert!(legacy.status()?.success()); + let baseline = PARENT_FORKS.load(Ordering::Relaxed); + assert_eq!(baseline, 1, "the positive control must detect fork"); + + file_system + .write_file( + &uri, + b"allowed".to_vec(), + Default::default(), + Some(&sandbox), + ) + .await?; + assert_eq!( + file_system + .read_file(&uri, Default::default(), Some(&sandbox)) + .await?, + b"allowed", + ); + // The transferred fd must retain its file after the path is replaced. + let stream = file_system.read_file_stream(&uri, Some(&sandbox)).await?; + std::fs::rename(&path, allowed.join("opened.txt"))?; + std::fs::write(&path, b"replacement")?; + assert_eq!(stream.try_collect::>().await?.concat(), b"allowed",); + + for denied in [secret.clone(), allowed.join("escape/secret.txt")] { + assert_eq!(std::fs::read(&denied)?, b"secret"); + let denied = PathUri::from_host_native_path(denied)?; + let read_error = file_system + .read_file(&denied, Default::default(), Some(&sandbox)) + .await + .expect_err("sandboxed read must reject an outside file"); + assert_sandbox_denied(&read_error); + let stream_error = file_system + .read_file_stream(&denied, Some(&sandbox)) + .await + .err() + .context("sandboxed stream must reject an outside file")?; + assert_sandbox_denied(&stream_error); + let write_error = file_system + .write_file( + &denied, + b"changed".to_vec(), + Default::default(), + Some(&sandbox), + ) + .await + .expect_err("sandboxed write must reject an outside file"); + assert_sandbox_denied(&write_error); + } + assert_eq!(std::fs::read(&secret)?, b"secret"); + assert_eq!( + PARENT_FORKS.load(Ordering::Relaxed), + baseline, + "sandboxed reads, writes, and fd transfers must avoid fork", + ); + println!("{CHILD_COMPLETED}"); + Ok(()) +} + +fn assert_sandbox_denied(error: &io::Error) { + assert!( + matches!( + error.kind(), + io::ErrorKind::PermissionDenied | io::ErrorKind::InvalidInput + ), + "expected a sandbox denial, got {error:?}", + ); + let message = error.to_string(); + assert!( + message.contains("is not permitted") + || message.contains("Operation not permitted") + || message.contains("Permission denied"), + "expected a permission denial, got {message}", + ); +} diff --git a/codex-rs/utils/pty/src/child.rs b/codex-rs/utils/pty/src/child.rs index 6abd9fcd22..a703f04a62 100644 --- a/codex-rs/utils/pty/src/child.rs +++ b/codex-rs/utils/pty/src/child.rs @@ -6,6 +6,7 @@ use std::io; use std::process::ExitStatus; +use tokio::io::AsyncReadExt; use tokio::process::ChildStderr; use tokio::process::ChildStdin; use tokio::process::ChildStdout; @@ -47,6 +48,38 @@ impl Child { } } + /// Drain both output pipes while waiting, retaining kill-on-drop on cancellation. + pub async fn wait_with_output(mut self) -> io::Result { + let mut stdout = self.stdout.take(); + let mut stderr = self.stderr.take(); + let mut output = Vec::new(); + let mut diagnostic = Vec::new(); + let (status, _, _) = tokio::try_join!( + self.wait(), + async { + if let Some(stdout) = stdout.as_mut() { + stdout.read_to_end(&mut output).await?; + } + Ok::<_, io::Error>(()) + }, + async { + if let Some(stderr) = stderr.as_mut() { + stderr.read_to_end(&mut diagnostic).await?; + } + Ok::<_, io::Error>(()) + }, + )?; + // Keep the pipes open until the child exits, even after EOF, matching Tokio. + // See https://github.com/tokio-rs/tokio/issues/4309. + drop(stdout); + drop(stderr); + Ok(std::process::Output { + status, + stdout: output, + stderr: diagnostic, + }) + } + /// Kill the direct child and reap it. Process-tree policy belongs to the caller. pub async fn kill(&mut self) -> io::Result<()> { self.stdin.take(); @@ -57,3 +90,7 @@ impl Child { } } } + +#[cfg(all(test, unix))] +#[path = "child_tests.rs"] +mod tests; diff --git a/codex-rs/utils/pty/src/child_command.rs b/codex-rs/utils/pty/src/child_command.rs index 2f84d83dc7..89c890ddfa 100644 --- a/codex-rs/utils/pty/src/child_command.rs +++ b/codex-rs/utils/pty/src/child_command.rs @@ -2,12 +2,15 @@ //! //! The wrapped Tokio command is private: callers cannot install callbacks or //! change settings that the native backend cannot inspect. Children receive only -//! explicitly supplied environment variables, piped stdio, and kill-on-drop. +//! explicitly supplied environment variables and kill-on-drop. Stdio, descriptor +//! inheritance, and compatibility fallbacks are configured independently. use std::ffi::OsStr; +#[cfg(unix)] +use std::ffi::OsString; use std::io; use std::path::Path; -use std::process::Stdio; +use std::process::Stdio as TokioStdio; use crate::child::Child; use crate::child::ChildKind; @@ -19,10 +22,36 @@ pub enum ProcessMode { NewGroup, } +/// Descriptors visible to a Unix child beyond its explicit stdio. +#[derive(Clone, Copy, PartialEq, Eq)] +pub enum DescriptorPolicy { + Inherit, + StdioOnly, +} + +/// Whether a native launch may use Command's executable-text and PATH fallbacks. +#[derive(Clone, Copy, PartialEq, Eq)] +pub enum SpawnFallback { + Compatible, + ReturnError, +} + +/// An explicit child stdin, including a socket used for bidirectional fd transfer. +pub enum ChildStdin { + Piped, + #[cfg(unix)] + File(std::os::fd::OwnedFd), +} + /// A local command whose complete launch contract is known to both backends. pub struct Command { - inner: tokio::process::Command, - process_mode: ProcessMode, + pub(crate) inner: tokio::process::Command, + pub(crate) process_mode: ProcessMode, + pub(crate) descriptor_policy: DescriptorPolicy, + pub(crate) fallback: SpawnFallback, + pub(crate) stdin: ChildStdin, + #[cfg(unix)] + pub(crate) arg0: Option, } impl Command { @@ -31,12 +60,17 @@ impl Command { inner .env_clear() .kill_on_drop(true) - .stdin(Stdio::piped()) - .stdout(Stdio::piped()) - .stderr(Stdio::piped()); + .stdin(TokioStdio::piped()) + .stdout(TokioStdio::piped()) + .stderr(TokioStdio::piped()); Self { inner, process_mode: ProcessMode::Inherit, + descriptor_policy: DescriptorPolicy::Inherit, + fallback: SpawnFallback::Compatible, + stdin: ChildStdin::Piped, + #[cfg(unix)] + arg0: None, } } @@ -74,6 +108,28 @@ impl Command { self } + pub fn stdin(&mut self, stdin: ChildStdin) -> &mut Self { + self.stdin = stdin; + self + } + + pub fn descriptor_policy(&mut self, policy: DescriptorPolicy) -> &mut Self { + self.descriptor_policy = policy; + self + } + + pub fn fallback(&mut self, fallback: SpawnFallback) -> &mut Self { + self.fallback = fallback; + self + } + + #[cfg(unix)] + pub fn arg0(&mut self, arg0: impl AsRef) -> &mut Self { + self.inner.arg0(arg0.as_ref()); + self.arg0 = Some(arg0.as_ref().to_owned()); + self + } + /// Preserve Job Object assignment before the child begins executing on Windows. #[cfg(windows)] pub fn prepare_suspended_spawn(&mut self, job: &crate::JobObject) { @@ -90,16 +146,27 @@ impl Command { { let command = self.inner.as_std(); let program = command.get_program(); - if Path::new(program).is_relative() + if (Path::new(program).is_relative() + || self.descriptor_policy == DescriptorPolicy::StdioOnly + || self.fallback == SpawnFallback::ReturnError) && !program.is_empty() - && let Some((child, stdin, stdout, stderr)) = - crate::child::macos::NativeChild::spawn(command, self.process_mode)? + && let Some(child) = crate::child::macos::NativeChild::spawn(&self)? { - return Ok(Child { - inner: ChildKind::Native(child), - stdin: Some(stdin), - stdout: Some(stdout), - stderr: Some(stderr), + return Ok(child); + } + } + self.inner.stdin(match self.stdin { + ChildStdin::Piped => TokioStdio::piped(), + #[cfg(unix)] + ChildStdin::File(fd) => TokioStdio::from(fd), + }); + #[cfg(unix)] + if self.descriptor_policy == DescriptorPolicy::StdioOnly { + // SAFETY: This preserves the existing Unix descriptor cleanup before exec. + unsafe { + self.inner.pre_exec(|| { + crate::pty::close_inherited_fds_except(&[]); + Ok(()) }); } } @@ -116,3 +183,7 @@ impl Command { #[cfg(all(test, target_os = "macos"))] #[path = "macos_child_tests.rs"] mod tests; + +#[cfg(all(test, target_os = "macos"))] +#[path = "macos_descriptor_tests.rs"] +mod descriptor_tests; diff --git a/codex-rs/utils/pty/src/child_tests.rs b/codex-rs/utils/pty/src/child_tests.rs new file mode 100644 index 0000000000..f6364733d0 --- /dev/null +++ b/codex-rs/utils/pty/src/child_tests.rs @@ -0,0 +1,68 @@ +//! Regression coverage for output-pipe lifetimes while waiting for a child. + +use std::future::Future; +use std::future::poll_fn; +use std::os::fd::AsRawFd; +use std::os::unix::process::ExitStatusExt; +use std::task::Poll; +use std::time::Duration; + +use pretty_assertions::assert_eq; +use tokio::io::AsyncReadExt; +use tokio::io::AsyncWriteExt; + +use crate::Command; + +#[tokio::test] +async fn wait_with_output_keeps_eof_pipes_open_until_exit() -> anyhow::Result<()> { + let mut command = Command::new("/bin/sh"); + command.args(["-c", "exec 1>&- 2>&-; read -r line"]); + let mut child = command.spawn()?; + let mut stdin = child.stdin.take().ok_or_else(|| anyhow::anyhow!("stdin"))?; + let stdout = child + .stdout + .as_mut() + .ok_or_else(|| anyhow::anyhow!("stdout"))?; + let stderr = child + .stderr + .as_mut() + .ok_or_else(|| anyhow::anyhow!("stderr"))?; + let fds = [stdout.as_raw_fd(), stderr.as_raw_fd()]; + + // Cache EOF readiness on both pipes while stdin keeps the child alive. + let mut byte = [0]; + assert_eq!( + tokio::time::timeout(Duration::from_secs(5), stdout.read(&mut byte)).await??, + 0 + ); + assert_eq!( + tokio::time::timeout(Duration::from_secs(5), stderr.read(&mut byte)).await??, + 0 + ); + let output = child.wait_with_output(); + tokio::pin!(output); + poll_fn(|cx| { + assert!(output.as_mut().poll(cx).is_pending()); + Poll::Ready(()) + }) + .await; + for fd in fds { + // SAFETY: F_GETFD only inspects the descriptor; it does not modify it. + assert_ne!( + unsafe { libc::fcntl(fd, libc::F_GETFD) }, + -1, + "EOF pipe closed before child exit" + ); + } + + stdin.write_all(b"exit\n").await?; + assert_eq!( + tokio::time::timeout(Duration::from_secs(5), output).await??, + std::process::Output { + status: std::process::ExitStatus::from_raw(0), + stdout: Vec::new(), + stderr: Vec::new(), + } + ); + Ok(()) +} diff --git a/codex-rs/utils/pty/src/lib.rs b/codex-rs/utils/pty/src/lib.rs index 732ecf06fc..d3438698b4 100644 --- a/codex-rs/utils/pty/src/lib.rs +++ b/codex-rs/utils/pty/src/lib.rs @@ -1,8 +1,11 @@ mod child; pub use child::Child; mod child_command; +pub use child_command::ChildStdin; pub use child_command::Command; +pub use child_command::DescriptorPolicy; pub use child_command::ProcessMode; +pub use child_command::SpawnFallback; pub mod pipe; mod process; pub mod process_group; diff --git a/codex-rs/utils/pty/src/macos_child.rs b/codex-rs/utils/pty/src/macos_child.rs index 782e7262e4..f36a8ca3dc 100644 --- a/codex-rs/utils/pty/src/macos_child.rs +++ b/codex-rs/utils/pty/src/macos_child.rs @@ -1,11 +1,9 @@ -//! Native spawning for macOS MCP executables, without rewriting script paths. +//! Native macOS spawning without rewriting executable paths or argv[0]. //! -//! Rust falls back to fork for a historical relative-path/cwd bug in Apple's -//! `posix_spawnp`. Calling `posix_spawn` directly avoids that wrapper. This module only -//! accepts the launcher's cleared-environment command shape, with piped stdio, -//! an explicit process-group mode, and default `argv[0]`. Bare commands search the child's -//! PATH. Unsuccessful searches and executable files without shebangs retain the -//! existing launcher. Each native child owns its PID until it has been reaped. +//! Spawn attributes and file actions implement the shared command's process-group +//! and descriptor policies. Bare commands search the child's PATH. Callers choose +//! whether incompatible executable formats and failed searches may retry through +//! Tokio. Each native child owns its PID until it has been reaped. use std::ffi::CString; use std::ffi::OsStr; @@ -43,16 +41,12 @@ pub(crate) struct NativeChild { } impl NativeChild { - /// Spawns the MCP command without changing its executable path or `argv[0]`. - /// The caller must clear inherited environment variables before setting the - /// child's environment, because only explicit command entries are copied. - pub(crate) fn spawn( - command: &std::process::Command, - process_mode: crate::ProcessMode, - ) -> io::Result> { + /// Spawn the explicit command, retaining the caller's executable spelling and argv[0]. + pub(crate) fn spawn(request: &crate::Command) -> io::Result> { + let command = request.inner.as_std(); let program = c_string(command.get_program())?; let search_path = !program.as_bytes().contains(&b'/'); - let args = std::iter::once(command.get_program()) + let args = std::iter::once(request.arg0.as_deref().unwrap_or(command.get_program())) .chain(command.get_args()) .map(c_string) .collect::>>()?; @@ -83,15 +77,23 @@ impl NativeChild { // Subscribe before spawning so a child that exits immediately cannot be missed. let sigchld = signal(SignalKind::child())?; - let (stdin_read, stdin_write) = io::pipe()?; + let (stdin_read, stdin) = match &request.stdin { + crate::ChildStdin::Piped => { + let (reader, writer) = io::pipe()?; + ( + OwnedFd::from(reader), + Some(ChildStdin::from_std(OwnedFd::from(writer).into())?), + ) + } + crate::ChildStdin::File(fd) => (fd.try_clone()?, None), + }; let (stdout_read, stdout_write) = io::pipe()?; let (stderr_read, stderr_write) = io::pipe()?; let child_fds = [ - child_fd(stdin_read.into())?, + child_fd(stdin_read)?, child_fd(stdout_write.into())?, child_fd(stderr_write.into())?, ]; - let stdin = ChildStdin::from_std(OwnedFd::from(stdin_write).into())?; let stdout = ChildStdout::from_std(OwnedFd::from(stdout_read).into())?; let stderr = ChildStderr::from_std(OwnedFd::from(stderr_read).into())?; @@ -116,7 +118,7 @@ impl NativeChild { target as i32, ))?; } - let group_flags = match process_mode { + let group_flags = match request.process_mode { crate::ProcessMode::Inherit => 0, crate::ProcessMode::NewGroup => { cvt(libc::posix_spawnattr_setpgroup( @@ -130,11 +132,13 @@ impl NativeChild { cvt_errno(libc::sigemptyset(&mut defaults))?; cvt_errno(libc::sigaddset(&mut defaults, libc::SIGPIPE))?; cvt(libc::posix_spawnattr_setsigdefault(&mut attrs.0, &defaults))?; - // Match Command's descriptor inheritance: honor FD_CLOEXEC rather - // than introducing a different policy with CLOEXEC_DEFAULT. + let descriptor_flags = match request.descriptor_policy { + crate::DescriptorPolicy::Inherit => 0, + crate::DescriptorPolicy::StdioOnly => libc::POSIX_SPAWN_CLOEXEC_DEFAULT, + }; cvt(libc::posix_spawnattr_setflags( &mut attrs.0, - (group_flags | libc::POSIX_SPAWN_SETSIGDEF) as _, + (group_flags | descriptor_flags | libc::POSIX_SPAWN_SETSIGDEF) as _, ))?; let mut spawn = |executable: &CString| { libc::posix_spawn( @@ -167,7 +171,11 @@ impl NativeChild { executable.push("/"); executable.push(command.get_program()); if executable.as_bytes().len() >= libc::PATH_MAX as usize { - return Ok(None); + return if request.fallback == crate::SpawnFallback::Compatible { + Ok(None) + } else { + Err(io::Error::from_raw_os_error(libc::ENAMETOOLONG)) + }; } result = spawn(&c_string(&executable)?); if !matches!( @@ -185,7 +193,9 @@ impl NativeChild { } }; // Retain Command's shell fallback and exact PATH search errors. - if result == libc::ENOEXEC || (search_path && result != 0) { + if request.fallback == crate::SpawnFallback::Compatible + && (result == libc::ENOEXEC || (search_path && result != 0)) + { return Ok(None); } cvt(result)?; @@ -194,7 +204,12 @@ impl NativeChild { status: None, sigchld, }; - Ok(Some((child, stdin, stdout, stderr))) + Ok(Some(crate::Child { + inner: super::ChildKind::Native(child), + stdin, + stdout: Some(stdout), + stderr: Some(stderr), + })) } pub(crate) fn id(&self) -> Option { diff --git a/codex-rs/utils/pty/src/macos_child_tests.rs b/codex-rs/utils/pty/src/macos_child_tests.rs index f534460708..921a71bfa7 100644 --- a/codex-rs/utils/pty/src/macos_child_tests.rs +++ b/codex-rs/utils/pty/src/macos_child_tests.rs @@ -19,29 +19,9 @@ use tokio::io::AsyncReadExt; use tokio::io::AsyncWriteExt; async fn native_output(command: Command) -> anyhow::Result { - let mut child = command.spawn()?; + let child = command.spawn()?; assert!(matches!(child.inner, ChildKind::Native(_))); - drop(child.stdin.take()); - let mut stdout = child - .stdout - .take() - .ok_or_else(|| io::Error::other("piped stdout"))?; - let mut stderr = child - .stderr - .take() - .ok_or_else(|| io::Error::other("piped stderr"))?; - let mut output = Vec::new(); - let mut diagnostic = Vec::new(); - let (_, _, status) = tokio::try_join!( - stdout.read_to_end(&mut output), - stderr.read_to_end(&mut diagnostic), - child.wait() - )?; - Ok(std::process::Output { - status, - stdout: output, - stderr: diagnostic, - }) + Ok(child.wait_with_output().await?) } #[tokio::test] @@ -111,12 +91,18 @@ async fn relative_script_preserves_paths_stdio_environment_and_process_group() - .env("MCP_TEST", "kept") .arg("spaces ; literal $arg") .arg(std::ffi::OsString::from_vec(b"raw-\xff".to_vec())); - let (mut child, mut stdin, mut stdout, mut stderr) = - NativeChild::spawn(command.inner.as_std(), command.process_mode)?.expect("native child"); + let mut child = NativeChild::spawn(&command)?.expect("native child"); + let mut stdin = child.stdin.take(); + let mut stdout = child.stdout.take().expect("piped stdout"); + let mut stderr = child.stderr.take().expect("piped stderr"); let pid = child.id().expect("live PID") as libc::pid_t; // SAFETY: getpgid only inspects the live child, which waits for input below. assert_eq!(unsafe { libc::getpgid(pid) }, pid); - stdin.write_all(b"hello\n").await?; + stdin + .as_mut() + .expect("piped stdin") + .write_all(b"hello\n") + .await?; drop(stdin); let mut output = Vec::new(); let mut diagnostic = String::new(); @@ -148,8 +134,9 @@ async fn native_executable_preserves_argv0() -> anyhow::Result<()> { .current_dir(root.path()) .process_mode(ProcessMode::NewGroup) .args(["-c", "printf '%s' \"$0\""]); - let (mut child, stdin, mut stdout, _stderr) = - NativeChild::spawn(command.inner.as_std(), command.process_mode)?.expect("native child"); + let mut child = NativeChild::spawn(&command)?.expect("native child"); + let stdin = child.stdin.take(); + let mut stdout = child.stdout.take().expect("piped stdout"); drop(stdin); let mut output = Vec::new(); stdout.read_to_end(&mut output).await?; @@ -166,8 +153,8 @@ async fn cancelled_wait_can_still_kill_and_reap_child() -> anyhow::Result<()> { command .current_dir(root.path()) .process_mode(ProcessMode::NewGroup); - let (mut child, _stdin, _stdout, _stderr) = - NativeChild::spawn(command.inner.as_std(), command.process_mode)?.expect("native child"); + let mut child = NativeChild::spawn(&command)?.expect("native child"); + let _stdin = child.stdin.take(); assert!( tokio::time::timeout(Duration::from_millis(20), child.wait()) .await @@ -202,9 +189,9 @@ async fn descriptor_inheritance_matches_command() -> anyhow::Result<()> { "-c", "if [ -e /dev/fd/\"$SENTINEL\" ]; then printf inherited; else printf closed; fi", ]); - let (mut child, stdin, mut stdout, _stderr) = - NativeChild::spawn(command.inner.as_std(), command.process_mode)? - .expect("native child"); + let mut child = NativeChild::spawn(&command)?.expect("native child"); + let stdin = child.stdin.take(); + let mut stdout = child.stdout.take().expect("piped stdout"); drop(stdin); let mut output = String::new(); stdout.read_to_string(&mut output).await?; @@ -274,15 +261,16 @@ fn dropping_after_runtime_shutdown_kills_and_reaps_child() -> anyhow::Result<()> let runtime = tokio::runtime::Builder::new_current_thread() .enable_all() .build()?; - let (child, stdin, _stdout, _stderr) = runtime + let mut child = runtime .block_on(async { let mut command = Command::new("./server"); command .current_dir(root.path()) .process_mode(ProcessMode::NewGroup); - NativeChild::spawn(command.inner.as_std(), command.process_mode) + NativeChild::spawn(&command) })? .expect("native child"); + let stdin = child.stdin.take(); let pid = child.id().expect("live PID") as libc::pid_t; drop(runtime); drop(child); diff --git a/codex-rs/utils/pty/src/macos_descriptor_tests.rs b/codex-rs/utils/pty/src/macos_descriptor_tests.rs new file mode 100644 index 0000000000..405c14b852 --- /dev/null +++ b/codex-rs/utils/pty/src/macos_descriptor_tests.rs @@ -0,0 +1,94 @@ +//! Native descriptor isolation, socket stdio, and launch errors. + +use super::*; +use pretty_assertions::assert_eq; +use std::fs; +use std::os::fd::AsRawFd; +use std::os::fd::FromRawFd; +use std::os::fd::OwnedFd; +use std::os::unix::fs::PermissionsExt; +use std::os::unix::net::UnixStream; +use tokio::io::AsyncReadExt; +use tokio::io::AsyncWriteExt; + +#[tokio::test] +async fn explicit_stdio_excludes_inheritable_descriptors_without_changing_parent() +-> anyhow::Result<()> { + let file = fs::File::open("/dev/null")?; + // SAFETY: Duplicate a live descriptor without CLOEXEC so the control inherits it. + let raw = unsafe { libc::fcntl(file.as_raw_fd(), libc::F_DUPFD, 200) }; + assert!(raw >= 200); + // SAFETY: fcntl returned a new descriptor owned by this test. + let sentinel = unsafe { OwnedFd::from_raw_fd(raw) }; + // SAFETY: The test owns this descriptor throughout both launches. + let flags = unsafe { libc::fcntl(raw, libc::F_GETFD) }; + let script = format!("if [ -e /dev/fd/{raw} ]; then printf open; else printf closed; fi"); + let mut control = Command::new("/bin/sh"); + control.args(["-c", &script]); + let control = control.spawn()?.wait_with_output().await?; + assert_eq!( + (control.status.success(), control.stdout), + (true, b"open".to_vec()) + ); + + let mut command = Command::new("/bin/sh"); + command + .args(["-c", &script]) + .descriptor_policy(DescriptorPolicy::StdioOnly); + let output = command.spawn()?.wait_with_output().await?; + assert_eq!( + (output.status.success(), output.stdout, output.stderr), + (true, b"closed".to_vec(), vec![]) + ); + // SAFETY: Spawning must not mutate the parent's descriptor flags. + assert_eq!( + unsafe { libc::fcntl(sentinel.as_raw_fd(), libc::F_GETFD) }, + flags + ); + Ok(()) +} + +#[tokio::test] +async fn socket_stdin_preserves_bidirectional_io_and_custom_argv0() -> anyhow::Result<()> { + let (parent, socket) = UnixStream::pair()?; + parent.set_nonblocking(true)?; + let mut parent = tokio::net::UnixStream::from_std(parent)?; + let mut command = Command::new("/bin/sh"); + command + .args(["--norc", "-c", "IFS= read -r line; printf 'reply:%s\\n' \"$line\" >&0; printf '%s:%s:%s' \"$0\" \"$FS_TEST\" \"${HOME-unset}\"; printf diagnostic >&2; exit 19"]) + .arg0("helper") + .env("FS_TEST", "literal ; $value") + .stdin(ChildStdin::File(socket.into())) + .descriptor_policy(DescriptorPolicy::StdioOnly) + .fallback(SpawnFallback::ReturnError); + let child = command.spawn()?; + assert!(child.stdin.is_none()); + parent.write_all(b"socket\n").await?; + parent.shutdown().await?; + let mut reply = Vec::new(); + parent.read_to_end(&mut reply).await?; + let output = child.wait_with_output().await?; + assert_eq!(reply, b"reply:socket\n"); + assert_eq!( + (output.status.code(), output.stdout, output.stderr), + ( + Some(19), + b"helper:literal ; $value:unset".to_vec(), + b"diagnostic".to_vec() + ) + ); + Ok(()) +} + +#[tokio::test] +async fn native_launch_can_reject_executable_text_without_shell_fallback() -> anyhow::Result<()> { + let directory = tempfile::tempdir()?; + let script = directory.path().join("no-shebang"); + fs::write(&script, "printf unexpected-fallback\n")?; + fs::set_permissions(&script, fs::Permissions::from_mode(/*mode*/ 0o755))?; + let mut command = Command::new(script); + command.fallback(SpawnFallback::ReturnError); + let error = command.spawn().err().expect("executable text must fail"); + assert_eq!(error.raw_os_error(), Some(libc::ENOEXEC)); + Ok(()) +}