mirror of
https://github.com/openai/codex.git
synced 2026-11-07 20:30:33 +00:00
Avoid fork when spawning macOS filesystem helpers (#46661)
## Why Filesystem helpers use a `pre_exec` callback to close inherited descriptors on macOS, forcing a fork before execution. Native spawning needs to preserve that isolation and support the socket used for file descriptor transfer. ## What changed - Launch filesystem helpers through `codex_utils_pty::Command`, using `posix_spawn` with `POSIX_SPAWN_CLOEXEC_DEFAULT` on macOS and returning native launch errors without a fork fallback. - Extend the shared command wrapper with explicit descriptor and fallback policies, socket-backed stdin, and custom `argv[0]` support. - Add `Child::wait_with_output` to drain stdout and stderr concurrently, retain kill-on-drop behavior on cancellation, and keep output pipes open until the child exits. ## Testing Add regression tests for fork-free sandboxed reads, writes, and file descriptor transfers; sandbox denial of outside paths and symlink escapes; descriptor isolation; bidirectional socket stdin; custom `argv[0]`; executable-format errors; and output-pipe lifetimes. GitOrigin-RevId: d49c00787f30ec0bc196f9e066a0770fc8151152
This commit is contained in:
@@ -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<u8>,
|
||||
) -> Result<FsHelperPayload, JSONRPCErrorError> {
|
||||
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<Result<Vec<u8>, 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<Result<Vec<u8>, 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<std::process::Output, JSONRPCErrorError> {
|
||||
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<tokio::process::Child, JSONRPCErrorError> {
|
||||
stdin: ChildStdin,
|
||||
) -> Result<Child, JSONRPCErrorError> {
|
||||
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)
|
||||
}
|
||||
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -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<tokio::fs::File, JSONRPCErrorError> {
|
||||
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()
|
||||
|
||||
Reference in New Issue
Block a user