From ed6082c9f92fd1de862c944d2f135c86a8e18998 Mon Sep 17 00:00:00 2001 From: viyatb-oai Date: Tue, 5 May 2026 10:45:35 -0700 Subject: [PATCH] fix(sandboxing): Bound advisory system bwrap startup probe (#20111) ## Why Linux startup runs an advisory system `bwrap` warning probe on each launch. On hosts with NFS or autofs mounts, its `--ro-bind / /` probe can take tens of seconds before Codex prints anything, matching #19828. Because this probe only decides whether to surface a warning, it should not be allowed to stall startup. Relevant pre-change path: [`codex-rs/sandboxing/src/bwrap.rs`](https://github.com/openai/codex/blob/de2ccf94735a3d8a2a7077e6a5292026413867cf/codex-rs/sandboxing/src/bwrap.rs#L64-L80) ## What changed - Bound the advisory system `bwrap` probe to 500 ms. - Preserve the existing warning behavior when `bwrap` promptly reports a known user-namespace failure. - Kill and reap the probe child on timeout, then suppress the advisory warning instead of blocking startup. - Read probe stderr with a bounded nonblocking drain so descendants that inherit the pipe cannot extend startup after the probe child exits. - Add regression coverage for both a deliberately slow fake `bwrap` process and a fake probe whose descendant keeps stderr open. ## Security This only bounds the advisory startup probe. It does not change the command execution path or add a fail-open sandbox fallback. The related command-side hang in #20017 remains separate from this PR. ## Verification - Added `system_bwrap_probe_times_out_without_reporting_a_warning`. - Added `system_bwrap_probe_does_not_wait_for_descendants_holding_stderr_open`. - `cargo test -p codex-sandboxing` - `cargo clippy -p codex-sandboxing --all-targets -- -D warnings` Fixes #19828 Related: #20017 --- codex-rs/sandboxing/src/bwrap.rs | 67 +++++++++++++++++++++++--- codex-rs/sandboxing/src/bwrap_tests.rs | 39 +++++++++++++++ 2 files changed, 100 insertions(+), 6 deletions(-) diff --git a/codex-rs/sandboxing/src/bwrap.rs b/codex-rs/sandboxing/src/bwrap.rs index 3435c6d19..840bb3e62 100644 --- a/codex-rs/sandboxing/src/bwrap.rs +++ b/codex-rs/sandboxing/src/bwrap.rs @@ -1,9 +1,16 @@ use crate::policy_transforms::should_require_platform_sandbox; use codex_protocol::models::PermissionProfile; +use std::io::ErrorKind; +use std::io::Read; +use std::os::fd::AsRawFd; use std::path::Path; use std::path::PathBuf; use std::process::Command; use std::process::Output; +use std::process::Stdio; +use std::thread; +use std::time::Duration; +use std::time::Instant; const SYSTEM_BWRAP_PROGRAM: &str = "bwrap"; const MISSING_BWRAP_WARNING: &str = concat!( @@ -26,6 +33,9 @@ const USER_NAMESPACE_FAILURES: [&str; 4] = [ "setting up uid map: Permission denied", "No permissions to create a new namespace", ]; +const SYSTEM_BWRAP_PROBE_TIMEOUT: Duration = Duration::from_millis(500); +const SYSTEM_BWRAP_PROBE_POLL_INTERVAL: Duration = Duration::from_millis(50); +const SYSTEM_BWRAP_PROBE_STDERR_LIMIT_BYTES: u64 = 64 * 1024; pub fn system_bwrap_warning(permission_profile: &PermissionProfile) -> Option { if !should_warn_about_system_bwrap(permission_profile) { @@ -54,15 +64,15 @@ fn system_bwrap_warning_for_path(system_bwrap_path: Option<&Path>) -> Option bool { - let output = match Command::new(system_bwrap_path) +fn system_bwrap_has_user_namespace_access(system_bwrap_path: &Path, timeout: Duration) -> bool { + let mut child = match Command::new(system_bwrap_path) .args([ "--unshare-user", "--unshare-net", @@ -71,13 +81,58 @@ fn system_bwrap_has_user_namespace_access(system_bwrap_path: &Path) -> bool { "/", "/bin/true", ]) - .output() + .stdout(Stdio::null()) + .stderr(Stdio::piped()) + .spawn() { - Ok(output) => output, + Ok(child) => child, Err(_) => return true, }; - output.status.success() || !is_user_namespace_failure(&output) + let deadline = Instant::now() + timeout; + loop { + match child.try_wait() { + Ok(Some(status)) => { + let stderr = child.stderr.take().map_or_else(Vec::new, |stderr| { + let fd = stderr.as_raw_fd(); + let flags = unsafe { libc::fcntl(fd, libc::F_GETFL) }; + if flags < 0 + || unsafe { libc::fcntl(fd, libc::F_SETFL, flags | libc::O_NONBLOCK) } < 0 + { + return Vec::new(); + } + + let mut bytes = Vec::new(); + let mut stderr = stderr.take(SYSTEM_BWRAP_PROBE_STDERR_LIMIT_BYTES); + if let Err(err) = stderr.read_to_end(&mut bytes) + && err.kind() != ErrorKind::WouldBlock + { + return bytes; + } + bytes + }); + let output = Output { + status, + stdout: Vec::new(), + stderr, + }; + return output.status.success() || !is_user_namespace_failure(&output); + } + Ok(None) => { + if Instant::now() >= deadline { + let _ = child.kill(); + let _ = child.wait(); + return true; + } + thread::sleep(SYSTEM_BWRAP_PROBE_POLL_INTERVAL); + } + Err(_) => { + let _ = child.kill(); + let _ = child.wait(); + return true; + } + } + } } pub(crate) fn is_wsl1() -> bool { diff --git a/codex-rs/sandboxing/src/bwrap_tests.rs b/codex-rs/sandboxing/src/bwrap_tests.rs index f36848e1e..3c7a50392 100644 --- a/codex-rs/sandboxing/src/bwrap_tests.rs +++ b/codex-rs/sandboxing/src/bwrap_tests.rs @@ -2,6 +2,8 @@ use super::*; use pretty_assertions::assert_eq; use std::path::Path; use std::path::PathBuf; +use std::time::Duration; +use std::time::Instant; use tempfile::tempdir; #[test] @@ -44,6 +46,43 @@ exit 1 assert_eq!(system_bwrap_warning_for_path(Some(fake_bwrap_path)), None); } +#[test] +fn system_bwrap_probe_times_out_without_reporting_a_warning() { + let fake_bwrap = write_fake_bwrap( + r#"#!/bin/sh +sleep 1 +exit 0 +"#, + ); + let fake_bwrap_path: &Path = fake_bwrap.as_ref(); + let started_at = Instant::now(); + + assert!(system_bwrap_has_user_namespace_access( + fake_bwrap_path, + Duration::from_millis(10), + )); + assert!(started_at.elapsed() < Duration::from_millis(500)); +} + +#[test] +fn system_bwrap_probe_does_not_wait_for_descendants_holding_stderr_open() { + let fake_bwrap = write_fake_bwrap( + r#"#!/bin/sh +echo 'No permissions to create a new namespace' >&2 +sleep 1 & +exit 1 +"#, + ); + let fake_bwrap_path: &Path = fake_bwrap.as_ref(); + let started_at = Instant::now(); + + assert!(!system_bwrap_has_user_namespace_access( + fake_bwrap_path, + Duration::from_millis(100), + )); + assert!(started_at.elapsed() < Duration::from_millis(500)); +} + #[test] fn detects_wsl1_proc_version_formats() { assert!(proc_version_indicates_wsl1(