mirror of
https://github.com/openai/codex.git
synced 2026-10-21 09:54:56 +00:00
Use native spawning for bare macOS MCP commands (#42192)
## What changed - Extend the macOS MCP launcher to use native spawning for bare command names as well as relative executable paths. - Resolve bare commands against the child's configured `PATH`, including empty entries and the default path when `PATH` is unset, while preserving `argv[0]` and script spelling. - Fall back to the existing command launcher for failed searches and executable text without a shebang so its errors and shell behavior remain unchanged. ## Testing Add macOS coverage for child `PATH` resolution, default-path lookup, launch errors, and shebang-free executable fallback. GitOrigin-RevId: 06e76bf8dbd515331b588da797eb57cb765a546d
This commit is contained in:
@@ -1,11 +1,11 @@
|
||||
//! Native spawning for relative macOS MCP executables, without rewriting script paths.
|
||||
//! Native spawning for macOS MCP executables, without rewriting script paths.
|
||||
//!
|
||||
//! 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,
|
||||
//! a new process group, and default `argv[0]`. PATH lookup and executable files
|
||||
//! without shebangs retain the existing launcher. Each native child owns its PID
|
||||
//! until it has been reaped.
|
||||
//! a new process group, 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.
|
||||
|
||||
use std::ffi::CString;
|
||||
use std::ffi::OsStr;
|
||||
@@ -42,12 +42,12 @@ enum ChildKind {
|
||||
}
|
||||
|
||||
impl LocalChild {
|
||||
/// Uses native spawning for relative paths, retaining Tokio's fallback for
|
||||
/// PATH lookup and executable text without a shebang.
|
||||
/// Uses native spawning for relative paths and bare names, retaining Tokio's
|
||||
/// fallback for unsuccessful PATH searches and executable text without a shebang.
|
||||
pub(super) fn spawn(mut command: Command) -> io::Result<Self> {
|
||||
let program = command.as_std().get_program();
|
||||
if Path::new(program).is_relative()
|
||||
&& program.as_bytes().contains(&b'/')
|
||||
&& !program.is_empty()
|
||||
&& let Some((child, stdin, stdout, stderr)) = NativeChild::spawn(command.as_std())?
|
||||
{
|
||||
return Ok(Self {
|
||||
@@ -115,6 +115,7 @@ impl NativeChild {
|
||||
command: &std::process::Command,
|
||||
) -> io::Result<Option<(Self, ChildStdin, ChildStdout, ChildStderr)>> {
|
||||
let program = c_string(command.get_program())?;
|
||||
let search_path = !program.as_bytes().contains(&b'/');
|
||||
let args = std::iter::once(command.get_program())
|
||||
.chain(command.get_args())
|
||||
.map(c_string)
|
||||
@@ -193,18 +194,56 @@ impl NativeChild {
|
||||
&mut attrs.0,
|
||||
(libc::POSIX_SPAWN_SETPGROUP | libc::POSIX_SPAWN_SETSIGDEF) as _,
|
||||
))?;
|
||||
libc::posix_spawn(
|
||||
&mut pid,
|
||||
program.as_ptr(),
|
||||
&actions.0,
|
||||
&attrs.0,
|
||||
argv.as_ptr(),
|
||||
envp.as_ptr(),
|
||||
)
|
||||
let mut spawn = |executable: &CString| {
|
||||
libc::posix_spawn(
|
||||
&mut pid,
|
||||
executable.as_ptr(),
|
||||
&actions.0,
|
||||
&attrs.0,
|
||||
argv.as_ptr(),
|
||||
envp.as_ptr(),
|
||||
)
|
||||
};
|
||||
if !search_path {
|
||||
spawn(&program)
|
||||
} else {
|
||||
// posix_spawnp searches the parent's PATH, not envp. Search the
|
||||
// child's PATH ourselves, using Apple's default when it is unset.
|
||||
let path = command
|
||||
.get_envs()
|
||||
.find(|(key, _)| *key == "PATH")
|
||||
.and_then(|(_, value)| value)
|
||||
.unwrap_or(OsStr::new("/usr/bin:/bin"));
|
||||
let mut result = libc::ENOENT;
|
||||
for directory in std::env::split_paths(path) {
|
||||
let mut executable = directory.into_os_string();
|
||||
if executable.is_empty() {
|
||||
executable.push(".");
|
||||
}
|
||||
// Preserve the spelling execvp would pass to a shebang
|
||||
// interpreter, including empty entries and trailing slashes.
|
||||
executable.push("/");
|
||||
executable.push(command.get_program());
|
||||
if executable.as_bytes().len() >= libc::PATH_MAX as usize {
|
||||
return Ok(None);
|
||||
}
|
||||
result = spawn(&c_string(&executable)?);
|
||||
if !matches!(
|
||||
result,
|
||||
libc::ENOENT
|
||||
| libc::ENOTDIR
|
||||
| libc::EACCES
|
||||
| libc::ELOOP
|
||||
| libc::ENAMETOOLONG
|
||||
) {
|
||||
break;
|
||||
}
|
||||
}
|
||||
result
|
||||
}
|
||||
};
|
||||
// execvp also supports executable text files without a shebang. Leave
|
||||
// that compatibility behavior to the existing Command-based launcher.
|
||||
if result == libc::ENOEXEC {
|
||||
// Retain Command's shell fallback and exact PATH search errors.
|
||||
if result == libc::ENOEXEC || (search_path && result != 0) {
|
||||
return Ok(None);
|
||||
}
|
||||
cvt(result)?;
|
||||
|
||||
@@ -10,6 +10,72 @@ use std::time::Duration;
|
||||
use tokio::io::AsyncReadExt;
|
||||
use tokio::io::AsyncWriteExt;
|
||||
|
||||
async fn native_output(command: Command) -> anyhow::Result<std::process::Output> {
|
||||
let mut child = crate::local_child::spawn(command)?;
|
||||
assert!(matches!(child.inner, ChildKind::Native(_)));
|
||||
drop(child.stdin.take());
|
||||
let mut stdout = child.stdout.take().expect("piped stdout");
|
||||
let mut stderr = child.stderr.take().expect("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,
|
||||
})
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn bare_script_search_matches_child_path_and_preserves_script_spelling() -> anyhow::Result<()>
|
||||
{
|
||||
let root = tempfile::tempdir()?;
|
||||
let program = std::ffi::OsString::from("server=é");
|
||||
let bin = root.path().join("bin");
|
||||
let blocked = root.path().join("blocked");
|
||||
fs::create_dir(&bin)?;
|
||||
fs::create_dir(&blocked)?;
|
||||
fs::write(blocked.join(&program), "not executable")?;
|
||||
let script = bin.join(&program);
|
||||
fs::write(
|
||||
&script,
|
||||
"#!/bin/sh\nprintf '%s\\n' \"$0\" \"$1\" \"$MCP_TEST\"; /bin/pwd; printf diagnostic >&2; exit 23\n",
|
||||
)?;
|
||||
fs::set_permissions(&script, fs::Permissions::from_mode(/*mode*/ 0o755))?;
|
||||
symlink(&script, root.path().join(&program))?;
|
||||
for path in [
|
||||
bin.into_os_string(),
|
||||
"bin/".into(),
|
||||
"missing:blocked:bin".into(),
|
||||
"missing:".into(),
|
||||
"".into(),
|
||||
] {
|
||||
let mut command = Command::new(&program);
|
||||
command
|
||||
.current_dir(root.path())
|
||||
.env_clear()
|
||||
.env("PATH", path)
|
||||
.env("MCP_TEST", "kept")
|
||||
.arg("spaces ; literal $arg");
|
||||
let expected = command.output().await?;
|
||||
assert_eq!(native_output(command).await?, expected);
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn bare_executable_uses_default_path_and_preserves_argv0() -> anyhow::Result<()> {
|
||||
let mut command = Command::new("sh");
|
||||
command.env_clear().args(["-c", "printf '%s' \"$0\""]);
|
||||
let expected = command.output().await?;
|
||||
assert_eq!(native_output(command).await?, expected);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn relative_script_preserves_paths_stdio_environment_and_process_group() -> anyhow::Result<()>
|
||||
{
|
||||
@@ -137,11 +203,17 @@ async fn launch_failures_preserve_os_errors() -> anyhow::Result<()> {
|
||||
for (program, cwd, errno) in [
|
||||
("./missing", root.path().to_path_buf(), libc::ENOENT),
|
||||
("./not-executable", root.path().to_path_buf(), libc::EACCES),
|
||||
("missing", root.path().to_path_buf(), libc::ENOENT),
|
||||
("not-executable", root.path().to_path_buf(), libc::EACCES),
|
||||
("", root.path().to_path_buf(), libc::ENOENT),
|
||||
("/bin/sh", root.path().join("missing"), libc::ENOENT),
|
||||
] {
|
||||
let mut command = std::process::Command::new(program);
|
||||
command.env_clear().current_dir(cwd);
|
||||
let error = NativeChild::spawn(&command)
|
||||
let mut command = Command::new(program);
|
||||
command
|
||||
.env_clear()
|
||||
.env("PATH", root.path())
|
||||
.current_dir(cwd);
|
||||
let error = crate::local_child::spawn(command)
|
||||
.err()
|
||||
.expect("spawn should fail");
|
||||
assert_eq!(error.raw_os_error(), Some(errno));
|
||||
@@ -155,18 +227,23 @@ async fn executable_text_without_shebang_retains_command_fallback() -> anyhow::R
|
||||
let script = root.path().join("server");
|
||||
fs::write(&script, "printf '%s' \"$0\"\n")?;
|
||||
fs::set_permissions(script, fs::Permissions::from_mode(0o755))?;
|
||||
let mut command = Command::new("./server");
|
||||
command.current_dir(root.path()).env_clear();
|
||||
let mut child = crate::local_child::spawn(command)?;
|
||||
let mut output = Vec::new();
|
||||
child
|
||||
.stdout
|
||||
.take()
|
||||
.expect("piped stdout")
|
||||
.read_to_end(&mut output)
|
||||
.await?;
|
||||
assert!(child.wait().await?.success());
|
||||
assert_eq!(output, b"./server");
|
||||
for program in ["./server", "server"] {
|
||||
let mut command = Command::new(program);
|
||||
command
|
||||
.current_dir(root.path())
|
||||
.env_clear()
|
||||
.env("PATH", ".");
|
||||
let mut child = crate::local_child::spawn(command)?;
|
||||
let mut output = Vec::new();
|
||||
child
|
||||
.stdout
|
||||
.take()
|
||||
.expect("piped stdout")
|
||||
.read_to_end(&mut output)
|
||||
.await?;
|
||||
assert!(child.wait().await?.success());
|
||||
assert_eq!(output, b"./server");
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user