mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
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
This commit is contained in:
committed by
GitHub
Unverified
parent
a3a09dfc9b
commit
ed6082c9f9
@@ -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<String> {
|
||||
if !should_warn_about_system_bwrap(permission_profile) {
|
||||
@@ -54,15 +64,15 @@ fn system_bwrap_warning_for_path(system_bwrap_path: Option<&Path>) -> Option<Str
|
||||
return Some(MISSING_BWRAP_WARNING.to_string());
|
||||
};
|
||||
|
||||
if !system_bwrap_has_user_namespace_access(system_bwrap_path) {
|
||||
if !system_bwrap_has_user_namespace_access(system_bwrap_path, SYSTEM_BWRAP_PROBE_TIMEOUT) {
|
||||
return Some(USER_NAMESPACE_WARNING.to_string());
|
||||
}
|
||||
|
||||
None
|
||||
}
|
||||
|
||||
fn system_bwrap_has_user_namespace_access(system_bwrap_path: &Path) -> 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 {
|
||||
|
||||
@@ -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(
|
||||
|
||||
Reference in New Issue
Block a user