mirror of
https://github.com/openai/codex.git
synced 2026-05-05 03:47:01 +00:00
## Summary - avoid setting a new process group when stdio is inherited (keeps child in foreground PG) - keep process-group isolation when stdio is redirected so killpg cleanup still works - prevents macOS job-control SIGTTIN stops that look like hangs after output ## Testing - `cargo build -p codex-cli` - `GIT_CONFIG_GLOBAL=/dev/null GIT_CONFIG_NOSYSTEM=1 CARGO_BIN_EXE_codex=/Users/denis/Code/codex/codex-rs/target/debug/codex /opt/homebrew/bin/timeout 30m cargo test -p codex-core -p codex-exec` ## Context This fixes macOS sandbox hangs for commands like `elixir -v` / `erl -noshell`, where the child was moved into a new process group while still attached to the controlling TTY. See issue #8690. ## Authorship & collaboration - This change and analysis were authored by **Codex** (AI coding agent). - Human collaborator: @seeekr provided repro environment, context, and review guidance. - CLI used: `codex-cli 0.77.0`. - Model: `gpt-5.2-codex (xhigh)`. Co-authored-by: Eric Traut <etraut@openai.com>
119 lines
4.5 KiB
Rust
119 lines
4.5 KiB
Rust
use std::collections::HashMap;
|
|
use std::path::PathBuf;
|
|
use std::process::Stdio;
|
|
use tokio::process::Child;
|
|
use tokio::process::Command;
|
|
use tracing::trace;
|
|
|
|
use crate::protocol::SandboxPolicy;
|
|
|
|
/// Experimental environment variable that will be set to some non-empty value
|
|
/// if both of the following are true:
|
|
///
|
|
/// 1. The process was spawned by Codex as part of a shell tool call.
|
|
/// 2. SandboxPolicy.has_full_network_access() was false for the tool call.
|
|
///
|
|
/// We may try to have just one environment variable for all sandboxing
|
|
/// attributes, so this may change in the future.
|
|
pub const CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR: &str = "CODEX_SANDBOX_NETWORK_DISABLED";
|
|
|
|
/// Should be set when the process is spawned under a sandbox. Currently, the
|
|
/// value is "seatbelt" for macOS, but it may change in the future to
|
|
/// accommodate sandboxing configuration and other sandboxing mechanisms.
|
|
pub const CODEX_SANDBOX_ENV_VAR: &str = "CODEX_SANDBOX";
|
|
|
|
#[derive(Debug, Clone, Copy)]
|
|
pub enum StdioPolicy {
|
|
RedirectForShellTool,
|
|
Inherit,
|
|
}
|
|
|
|
/// Spawns the appropriate child process for the ExecParams and SandboxPolicy,
|
|
/// ensuring the args and environment variables used to create the `Command`
|
|
/// (and `Child`) honor the configuration.
|
|
///
|
|
/// For now, we take `SandboxPolicy` as a parameter to spawn_child() because
|
|
/// we need to determine whether to set the
|
|
/// `CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR` environment variable.
|
|
pub(crate) async fn spawn_child_async(
|
|
program: PathBuf,
|
|
args: Vec<String>,
|
|
#[cfg_attr(not(unix), allow(unused_variables))] arg0: Option<&str>,
|
|
cwd: PathBuf,
|
|
sandbox_policy: &SandboxPolicy,
|
|
stdio_policy: StdioPolicy,
|
|
env: HashMap<String, String>,
|
|
) -> std::io::Result<Child> {
|
|
trace!(
|
|
"spawn_child_async: {program:?} {args:?} {arg0:?} {cwd:?} {sandbox_policy:?} {stdio_policy:?} {env:?}"
|
|
);
|
|
|
|
let mut cmd = Command::new(&program);
|
|
#[cfg(unix)]
|
|
cmd.arg0(arg0.map_or_else(|| program.to_string_lossy().to_string(), String::from));
|
|
cmd.args(args);
|
|
cmd.current_dir(cwd);
|
|
cmd.env_clear();
|
|
cmd.envs(env);
|
|
|
|
if !sandbox_policy.has_full_network_access() {
|
|
cmd.env(CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR, "1");
|
|
}
|
|
|
|
// If this Codex process dies (including being killed via SIGKILL), we want
|
|
// any child processes that were spawned as part of a `"shell"` tool call
|
|
// to also be terminated.
|
|
|
|
#[cfg(unix)]
|
|
unsafe {
|
|
let set_process_group = matches!(stdio_policy, StdioPolicy::RedirectForShellTool);
|
|
#[cfg(target_os = "linux")]
|
|
let parent_pid = libc::getpid();
|
|
cmd.pre_exec(move || {
|
|
if set_process_group && libc::setpgid(0, 0) == -1 {
|
|
return Err(std::io::Error::last_os_error());
|
|
}
|
|
|
|
// This relies on prctl(2), so it only works on Linux.
|
|
#[cfg(target_os = "linux")]
|
|
{
|
|
// This prctl call effectively requests, "deliver SIGTERM when my
|
|
// current parent dies."
|
|
if libc::prctl(libc::PR_SET_PDEATHSIG, libc::SIGTERM) == -1 {
|
|
return Err(std::io::Error::last_os_error());
|
|
}
|
|
|
|
// Though if there was a race condition and this pre_exec() block is
|
|
// run _after_ the parent (i.e., the Codex process) has already
|
|
// exited, then parent will be the closest configured "subreaper"
|
|
// ancestor process, or PID 1 (init). If the Codex process has exited
|
|
// already, so should the child process.
|
|
if libc::getppid() != parent_pid {
|
|
libc::raise(libc::SIGTERM);
|
|
}
|
|
}
|
|
Ok(())
|
|
});
|
|
}
|
|
|
|
match stdio_policy {
|
|
StdioPolicy::RedirectForShellTool => {
|
|
// Do not create a file descriptor for stdin because otherwise some
|
|
// commands may hang forever waiting for input. For example, ripgrep has
|
|
// a heuristic where it may try to read from stdin as explained here:
|
|
// https://github.com/BurntSushi/ripgrep/blob/e2362d4d5185d02fa857bf381e7bd52e66fafc73/crates/core/flags/hiargs.rs#L1101-L1103
|
|
cmd.stdin(Stdio::null());
|
|
|
|
cmd.stdout(Stdio::piped()).stderr(Stdio::piped());
|
|
}
|
|
StdioPolicy::Inherit => {
|
|
// Inherit stdin, stdout, and stderr from the parent process.
|
|
cmd.stdin(Stdio::inherit())
|
|
.stdout(Stdio::inherit())
|
|
.stderr(Stdio::inherit());
|
|
}
|
|
}
|
|
|
|
cmd.kill_on_drop(true).spawn()
|
|
}
|