mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
fix: fall back to vendored bubblewrap when system bwrap lacks --argv0 (#15338)
## Why Fixes [#15283](https://github.com/openai/codex/issues/15283), where sandboxed tool calls fail on older distro `bubblewrap` builds because `/usr/bin/bwrap` does not understand `--argv0`. The upstream [bubblewrap v0.9.0 release notes](https://github.com/containers/bubblewrap/releases/tag/v0.9.0) explicitly call out `Add --argv0`. Flipping `use_legacy_landlock` globally works around that compatibility bug, but it also weakens the default Linux sandbox and breaks proxy-routed and split-policy cases called out in review. The follow-up Linux CI failure was in the new launcher test rather than the launcher logic: the fake `bwrap` helper stayed open for writing, so Linux would not exec it. This update also closes the user-visibility gap from review by surfacing the same startup warning when `/usr/bin/bwrap` is present but too old for `--argv0`, not only when it is missing. ## What Changed - keep `use_legacy_landlock` default-disabled - teach `codex-rs/linux-sandbox/src/launcher.rs` to fall back to the vendored bubblewrap build when `/usr/bin/bwrap` does not advertise `--argv0` support - add launcher tests for supported, unsupported, and missing system `bwrap` - write the fake `bwrap` test helper to a closed temp path so the supported-path launcher test works on Linux too - extend the startup warning path so Codex warns when `/usr/bin/bwrap` is missing or too old to support `--argv0` - mirror the warning/fallback wording across `codex-rs/linux-sandbox/README.md` and `codex-rs/core/README.md`, including that the fallback is the vendored bubblewrap compiled into the binary - cite the upstream `bubblewrap` release that introduced `--argv0` ## Verification - `bazel test --config=remote --platforms=//:rbe //codex-rs/linux-sandbox:linux-sandbox-unit-tests --test_filter=launcher::tests::prefers_system_bwrap_when_help_lists_argv0 --test_output=errors` - `cargo test -p codex-core system_bwrap_warning` - `cargo check -p codex-exec -p codex-tui -p codex-tui-app-server -p codex-app-server` - `just argument-comment-lint`
This commit is contained in:
committed by
GitHub
Unverified
parent
d807d44ae7
commit
d1088158b8
@@ -479,7 +479,7 @@ pub async fn run_main_with_transport(
|
||||
range: None,
|
||||
});
|
||||
}
|
||||
if let Some(warning) = codex_core::config::missing_system_bwrap_warning() {
|
||||
if let Some(warning) = codex_core::config::system_bwrap_warning() {
|
||||
config_warnings.push(ConfigWarningNotification {
|
||||
summary: warning,
|
||||
details: None,
|
||||
|
||||
@@ -61,9 +61,11 @@ cases like `/repo = write`, `/repo/a = none`, `/repo/a/b = write`, where the
|
||||
more specific writable child must reopen under a denied parent.
|
||||
|
||||
The Linux sandbox helper prefers `/usr/bin/bwrap` whenever it is available and
|
||||
falls back to the vendored bubblewrap path otherwise. When `/usr/bin/bwrap` is
|
||||
missing, Codex also surfaces a startup warning through its normal notification
|
||||
path instead of printing directly from the sandbox helper.
|
||||
supports the required argv-rewrite flags, and falls back to the vendored
|
||||
bubblewrap path compiled into the binary otherwise. When `/usr/bin/bwrap` is
|
||||
missing or too old to support the required flags, Codex also surfaces a startup
|
||||
warning through its normal notification path instead of printing directly from
|
||||
the sandbox helper.
|
||||
|
||||
### Windows
|
||||
|
||||
|
||||
@@ -5582,16 +5582,69 @@ shell_tool = true
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[cfg(target_os = "linux")]
|
||||
#[test]
|
||||
fn missing_system_bwrap_warning_matches_system_bwrap_presence() {
|
||||
#[cfg(target_os = "linux")]
|
||||
assert_eq!(
|
||||
missing_system_bwrap_warning().is_some(),
|
||||
!Path::new("/usr/bin/bwrap").is_file()
|
||||
);
|
||||
fn system_bwrap_warning_reports_missing_system_bwrap() {
|
||||
let warning = system_bwrap_warning_for_path(Path::new("/definitely/not/a/bwrap"))
|
||||
.expect("missing system bwrap should emit a warning");
|
||||
|
||||
#[cfg(not(target_os = "linux"))]
|
||||
assert!(missing_system_bwrap_warning().is_none());
|
||||
assert!(warning.contains("could not find system bubblewrap"));
|
||||
}
|
||||
|
||||
#[cfg(target_os = "linux")]
|
||||
#[test]
|
||||
fn system_bwrap_warning_reports_too_old_system_bwrap() {
|
||||
let fake_bwrap = write_fake_bwrap(
|
||||
r#"#!/bin/sh
|
||||
if [ "$1" = "--help" ]; then
|
||||
echo 'usage: bwrap [OPTION...] COMMAND'
|
||||
exit 0
|
||||
fi
|
||||
exit 1
|
||||
"#,
|
||||
);
|
||||
let fake_bwrap_path: &Path = fake_bwrap.as_ref();
|
||||
let warning = system_bwrap_warning_for_path(fake_bwrap_path)
|
||||
.expect("old system bwrap should emit a warning");
|
||||
|
||||
assert!(warning.contains("too old to support `--argv0`"));
|
||||
}
|
||||
|
||||
#[cfg(target_os = "linux")]
|
||||
#[test]
|
||||
fn system_bwrap_warning_skips_supported_system_bwrap() {
|
||||
let fake_bwrap = write_fake_bwrap(
|
||||
r#"#!/bin/sh
|
||||
if [ "$1" = "--help" ]; then
|
||||
echo ' --argv0 PROGRAM'
|
||||
exit 0
|
||||
fi
|
||||
exit 1
|
||||
"#,
|
||||
);
|
||||
let fake_bwrap_path: &Path = fake_bwrap.as_ref();
|
||||
|
||||
assert_eq!(system_bwrap_warning_for_path(fake_bwrap_path), None);
|
||||
}
|
||||
|
||||
#[cfg(not(target_os = "linux"))]
|
||||
#[test]
|
||||
fn system_bwrap_warning_is_disabled_off_linux() {
|
||||
assert!(system_bwrap_warning().is_none());
|
||||
}
|
||||
|
||||
#[cfg(target_os = "linux")]
|
||||
fn write_fake_bwrap(contents: &str) -> tempfile::TempPath {
|
||||
use std::fs;
|
||||
use std::os::unix::fs::PermissionsExt;
|
||||
use tempfile::NamedTempFile;
|
||||
|
||||
// Linux rejects exec-ing a file that is still open for writing.
|
||||
let path = NamedTempFile::new().expect("temp file").into_temp_path();
|
||||
fs::write(&path, contents).expect("write fake bwrap");
|
||||
let permissions = fs::Permissions::from_mode(0o755);
|
||||
fs::set_permissions(&path, permissions).expect("chmod fake bwrap");
|
||||
path
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
|
||||
@@ -96,6 +96,8 @@ use std::collections::HashMap;
|
||||
use std::io::ErrorKind;
|
||||
use std::path::Path;
|
||||
use std::path::PathBuf;
|
||||
#[cfg(target_os = "linux")]
|
||||
use std::process::Command;
|
||||
|
||||
use crate::config::permissions::compile_permission_profile;
|
||||
use crate::config::permissions::get_readable_roots_required_for_codex_runtime;
|
||||
@@ -153,21 +155,46 @@ const RESERVED_MODEL_PROVIDER_IDS: [&str; 3] = [
|
||||
];
|
||||
|
||||
#[cfg(target_os = "linux")]
|
||||
pub fn missing_system_bwrap_warning() -> Option<String> {
|
||||
if Path::new(SYSTEM_BWRAP_PATH).is_file() {
|
||||
None
|
||||
} else {
|
||||
Some(format!(
|
||||
"Codex could not find system bubblewrap at {SYSTEM_BWRAP_PATH}. Please install bubblewrap with your package manager. Codex will use the vendored bubblewrap in the meantime."
|
||||
))
|
||||
}
|
||||
pub fn system_bwrap_warning() -> Option<String> {
|
||||
system_bwrap_warning_for_path(Path::new(SYSTEM_BWRAP_PATH))
|
||||
}
|
||||
|
||||
#[cfg(not(target_os = "linux"))]
|
||||
pub fn missing_system_bwrap_warning() -> Option<String> {
|
||||
pub fn system_bwrap_warning() -> Option<String> {
|
||||
None
|
||||
}
|
||||
|
||||
#[cfg(target_os = "linux")]
|
||||
fn system_bwrap_warning_for_path(system_bwrap_path: &Path) -> Option<String> {
|
||||
if !system_bwrap_path.is_file() {
|
||||
return Some(format!(
|
||||
"Codex could not find system bubblewrap at {}. Please install bubblewrap with your package manager. Codex will use the vendored bubblewrap in the meantime.",
|
||||
system_bwrap_path.display()
|
||||
));
|
||||
}
|
||||
if system_bwrap_supports_argv0(system_bwrap_path) {
|
||||
return None;
|
||||
}
|
||||
|
||||
Some(format!(
|
||||
"Codex found system bubblewrap at {}, but it is too old to support `--argv0`. Please upgrade bubblewrap with your package manager. Codex will use the vendored bubblewrap in the meantime.",
|
||||
system_bwrap_path.display()
|
||||
))
|
||||
}
|
||||
|
||||
#[cfg(target_os = "linux")]
|
||||
fn system_bwrap_supports_argv0(system_bwrap_path: &Path) -> bool {
|
||||
// bubblewrap added `--argv0` in v0.9.0:
|
||||
// https://github.com/containers/bubblewrap/releases/tag/v0.9.0
|
||||
let output = match Command::new(system_bwrap_path).arg("--help").output() {
|
||||
Ok(output) => output,
|
||||
Err(_) => return false,
|
||||
};
|
||||
let stdout = String::from_utf8_lossy(&output.stdout);
|
||||
let stderr = String::from_utf8_lossy(&output.stderr);
|
||||
stdout.contains("--argv0") || stderr.contains("--argv0")
|
||||
}
|
||||
|
||||
fn resolve_sqlite_home_env(resolved_cwd: &Path) -> Option<PathBuf> {
|
||||
let raw = std::env::var(codex_state::SQLITE_HOME_ENV).ok()?;
|
||||
let trimmed = raw.trim();
|
||||
|
||||
@@ -669,7 +669,7 @@ async fn run_exec_session(args: ExecRunArgs) -> anyhow::Result<()> {
|
||||
// Print the effective configuration and initial request so users can see what Codex
|
||||
// is using.
|
||||
event_processor.print_config_summary(&config, &prompt_summary, &session_configured);
|
||||
if !json_mode && let Some(message) = codex_core::config::missing_system_bwrap_warning() {
|
||||
if !json_mode && let Some(message) = codex_core::config::system_bwrap_warning() {
|
||||
let _ = event_processor.process_event(Event {
|
||||
id: String::new(),
|
||||
msg: EventMsg::Warning(codex_protocol::protocol::WarningEvent { message }),
|
||||
|
||||
@@ -8,19 +8,23 @@ This crate is responsible for producing:
|
||||
- this should also be true of the `codex` multitool CLI
|
||||
|
||||
On Linux, the bubblewrap pipeline prefers the system `/usr/bin/bwrap` whenever
|
||||
it is available. If `/usr/bin/bwrap` is missing, the helper still falls back to
|
||||
it is available and supports the required argv-rewrite flags. If `/usr/bin/bwrap`
|
||||
is missing or too old to support the required flags, the helper falls back to
|
||||
the vendored bubblewrap path compiled into this binary.
|
||||
Codex also surfaces a startup warning when `/usr/bin/bwrap` is missing so users
|
||||
know it is falling back to the vendored helper.
|
||||
Codex also surfaces a startup warning when `/usr/bin/bwrap` is missing or too
|
||||
old to support the required flags so users know it is falling back to the
|
||||
vendored helper.
|
||||
|
||||
**Current Behavior**
|
||||
- Legacy `SandboxPolicy` / `sandbox_mode` configs remain supported.
|
||||
- Bubblewrap is the default filesystem sandbox pipeline.
|
||||
- If `/usr/bin/bwrap` is present, the helper uses it.
|
||||
- If `/usr/bin/bwrap` is missing, the helper falls back to the vendored
|
||||
bubblewrap path.
|
||||
- If `/usr/bin/bwrap` is missing, Codex also surfaces a startup warning instead
|
||||
of printing directly from the sandbox helper.
|
||||
- If `/usr/bin/bwrap` is present and supports the required argv-rewrite flags,
|
||||
the helper uses it.
|
||||
- If `/usr/bin/bwrap` is missing or too old to support the required flags, the
|
||||
helper falls back to the vendored bubblewrap path.
|
||||
- If `/usr/bin/bwrap` is missing or too old to support the required flags,
|
||||
Codex also surfaces a startup warning instead of printing directly from the
|
||||
sandbox helper.
|
||||
- Legacy Landlock + mount protections remain available as an explicit legacy
|
||||
fallback path.
|
||||
- Set `features.use_legacy_landlock = true` (or CLI `-c use_legacy_landlock=true`)
|
||||
|
||||
@@ -4,6 +4,8 @@ use std::os::fd::AsRawFd;
|
||||
use std::os::raw::c_char;
|
||||
use std::os::unix::ffi::OsStrExt;
|
||||
use std::path::Path;
|
||||
use std::process::Command;
|
||||
use std::sync::OnceLock;
|
||||
|
||||
use crate::vendored_bwrap::exec_vendored_bwrap;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
@@ -24,17 +26,41 @@ pub(crate) fn exec_bwrap(argv: Vec<String>, preserved_files: Vec<File>) -> ! {
|
||||
}
|
||||
|
||||
fn preferred_bwrap_launcher() -> BubblewrapLauncher {
|
||||
if !Path::new(SYSTEM_BWRAP_PATH).is_file() {
|
||||
static LAUNCHER: OnceLock<BubblewrapLauncher> = OnceLock::new();
|
||||
LAUNCHER
|
||||
.get_or_init(|| preferred_bwrap_launcher_for_path(Path::new(SYSTEM_BWRAP_PATH)))
|
||||
.clone()
|
||||
}
|
||||
|
||||
fn preferred_bwrap_launcher_for_path(system_bwrap_path: &Path) -> BubblewrapLauncher {
|
||||
if !system_bwrap_supports_argv0(system_bwrap_path) {
|
||||
return BubblewrapLauncher::Vendored;
|
||||
}
|
||||
|
||||
let system_bwrap_path = match AbsolutePathBuf::from_absolute_path(SYSTEM_BWRAP_PATH) {
|
||||
let system_bwrap_path = match AbsolutePathBuf::from_absolute_path(system_bwrap_path) {
|
||||
Ok(path) => path,
|
||||
Err(err) => panic!("failed to normalize system bubblewrap path {SYSTEM_BWRAP_PATH}: {err}"),
|
||||
Err(err) => panic!(
|
||||
"failed to normalize system bubblewrap path {}: {err}",
|
||||
system_bwrap_path.display()
|
||||
),
|
||||
};
|
||||
BubblewrapLauncher::System(system_bwrap_path)
|
||||
}
|
||||
|
||||
fn system_bwrap_supports_argv0(system_bwrap_path: &Path) -> bool {
|
||||
// bubblewrap added `--argv0` in v0.9.0:
|
||||
// https://github.com/containers/bubblewrap/releases/tag/v0.9.0
|
||||
// Older distro packages (for example Ubuntu 20.04/22.04) ship builds that
|
||||
// reject `--argv0`, so prefer the vendored build in that case.
|
||||
let output = match Command::new(system_bwrap_path).arg("--help").output() {
|
||||
Ok(output) => output,
|
||||
Err(_) => return false,
|
||||
};
|
||||
let stdout = String::from_utf8_lossy(&output.stdout);
|
||||
let stderr = String::from_utf8_lossy(&output.stderr);
|
||||
stdout.contains("--argv0") || stderr.contains("--argv0")
|
||||
}
|
||||
|
||||
fn exec_system_bwrap(
|
||||
program: &AbsolutePathBuf,
|
||||
argv: Vec<String>,
|
||||
@@ -100,7 +126,57 @@ fn clear_cloexec(fd: libc::c_int) {
|
||||
mod tests {
|
||||
use super::*;
|
||||
use pretty_assertions::assert_eq;
|
||||
use std::fs;
|
||||
use std::os::unix::fs::PermissionsExt;
|
||||
use tempfile::NamedTempFile;
|
||||
use tempfile::TempPath;
|
||||
|
||||
#[test]
|
||||
fn prefers_system_bwrap_when_help_lists_argv0() {
|
||||
let fake_bwrap = write_fake_bwrap(
|
||||
r#"#!/bin/sh
|
||||
if [ "$1" = "--help" ]; then
|
||||
echo ' --argv0 PROGRAM'
|
||||
exit 0
|
||||
fi
|
||||
exit 1
|
||||
"#,
|
||||
);
|
||||
let fake_bwrap_path: &Path = fake_bwrap.as_ref();
|
||||
let expected = AbsolutePathBuf::from_absolute_path(fake_bwrap_path).expect("absolute");
|
||||
|
||||
assert_eq!(
|
||||
preferred_bwrap_launcher_for_path(fake_bwrap_path),
|
||||
BubblewrapLauncher::System(expected)
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn falls_back_to_vendored_when_system_bwrap_lacks_argv0() {
|
||||
let fake_bwrap = write_fake_bwrap(
|
||||
r#"#!/bin/sh
|
||||
if [ "$1" = "--help" ]; then
|
||||
echo 'usage: bwrap [OPTION...] COMMAND'
|
||||
exit 0
|
||||
fi
|
||||
exit 1
|
||||
"#,
|
||||
);
|
||||
let fake_bwrap_path: &Path = fake_bwrap.as_ref();
|
||||
|
||||
assert_eq!(
|
||||
preferred_bwrap_launcher_for_path(fake_bwrap_path),
|
||||
BubblewrapLauncher::Vendored
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn falls_back_to_vendored_when_system_bwrap_is_missing() {
|
||||
assert_eq!(
|
||||
preferred_bwrap_launcher_for_path(Path::new("/definitely/not/a/bwrap")),
|
||||
BubblewrapLauncher::Vendored
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn preserved_files_are_made_inheritable_for_system_exec() {
|
||||
@@ -131,4 +207,13 @@ mod tests {
|
||||
}
|
||||
flags
|
||||
}
|
||||
|
||||
fn write_fake_bwrap(contents: &str) -> TempPath {
|
||||
// Linux rejects exec-ing a file that is still open for writing.
|
||||
let path = NamedTempFile::new().expect("temp file").into_temp_path();
|
||||
fs::write(&path, contents).expect("write fake bwrap");
|
||||
let permissions = fs::Permissions::from_mode(0o755);
|
||||
fs::set_permissions(&path, permissions).expect("chmod fake bwrap");
|
||||
path
|
||||
}
|
||||
}
|
||||
|
||||
@@ -398,8 +398,8 @@ fn emit_project_config_warnings(app_event_tx: &AppEventSender, config: &Config)
|
||||
)));
|
||||
}
|
||||
|
||||
fn emit_missing_system_bwrap_warning(app_event_tx: &AppEventSender) {
|
||||
let Some(message) = codex_core::config::missing_system_bwrap_warning() else {
|
||||
fn emit_system_bwrap_warning(app_event_tx: &AppEventSender) {
|
||||
let Some(message) = codex_core::config::system_bwrap_warning() else {
|
||||
return;
|
||||
};
|
||||
|
||||
@@ -2197,7 +2197,7 @@ impl App {
|
||||
let (app_event_tx, mut app_event_rx) = unbounded_channel();
|
||||
let app_event_tx = AppEventSender::new(app_event_tx);
|
||||
emit_project_config_warnings(&app_event_tx, &config);
|
||||
emit_missing_system_bwrap_warning(&app_event_tx);
|
||||
emit_system_bwrap_warning(&app_event_tx);
|
||||
emit_custom_prompt_deprecation_notice(&app_event_tx, &config.codex_home).await;
|
||||
tui.set_notification_method(config.tui_notification_method);
|
||||
|
||||
|
||||
@@ -435,8 +435,8 @@ fn emit_project_config_warnings(app_event_tx: &AppEventSender, config: &Config)
|
||||
)));
|
||||
}
|
||||
|
||||
fn emit_missing_system_bwrap_warning(app_event_tx: &AppEventSender) {
|
||||
let Some(message) = codex_core::config::missing_system_bwrap_warning() else {
|
||||
fn emit_system_bwrap_warning(app_event_tx: &AppEventSender) {
|
||||
let Some(message) = codex_core::config::system_bwrap_warning() else {
|
||||
return;
|
||||
};
|
||||
|
||||
@@ -2794,7 +2794,7 @@ impl App {
|
||||
let (app_event_tx, mut app_event_rx) = unbounded_channel();
|
||||
let app_event_tx = AppEventSender::new(app_event_tx);
|
||||
emit_project_config_warnings(&app_event_tx, &config);
|
||||
emit_missing_system_bwrap_warning(&app_event_tx);
|
||||
emit_system_bwrap_warning(&app_event_tx);
|
||||
tui.set_notification_method(config.tui_notification_method);
|
||||
|
||||
let harness_overrides =
|
||||
|
||||
Reference in New Issue
Block a user