mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
fix(linux-sandbox): prefer system /usr/bin/bwrap when available (#14963)
## Problem Ubuntu/AppArmor hosts started failing in the default Linux sandbox path after the switch to vendored/default bubblewrap in `0.115.0`. The clearest report is in [#14919](https://github.com/openai/codex/issues/14919), especially [this investigation comment](https://github.com/openai/codex/issues/14919#issuecomment-4076504751): on affected Ubuntu systems, `/usr/bin/bwrap` works, but a copied or vendored `bwrap` binary fails with errors like `bwrap: setting up uid map: Permission denied` or `bwrap: loopback: Failed RTM_NEWADDR: Operation not permitted`. The root cause is Ubuntu's `/etc/apparmor.d/bwrap-userns-restrict` profile, which grants `userns` access specifically to `/usr/bin/bwrap`. Once Codex started using a vendored/internal bubblewrap path, that path was no longer covered by the distro AppArmor exception, so sandbox namespace setup could fail even when user namespaces were otherwise enabled and `uidmap` was installed. ## What this PR changes - prefer system `/usr/bin/bwrap` whenever it is available - keep vendored bubblewrap as the fallback when `/usr/bin/bwrap` is missing - when `/usr/bin/bwrap` is missing, surface a Codex startup warning through the app-server/TUI warning path instead of printing directly from the sandbox helper with `eprintln!` - use the same launcher decision for both the main sandbox execution path and the `/proc` preflight path - document the updated Linux bubblewrap behavior in the Linux sandbox and core READMEs ## Why this fix This still fixes the Ubuntu/AppArmor regression from [#14919](https://github.com/openai/codex/issues/14919), but it keeps the runtime rule simple and platform-agnostic: if the standard system bubblewrap is installed, use it; otherwise fall back to the vendored helper. The warning now follows that same simple rule. If Codex cannot find `/usr/bin/bwrap`, it tells the user that it is falling back to the vendored helper, and it does so through the existing startup warning plumbing that reaches the TUI and app-server instead of low-level sandbox stderr. ## Testing - `cargo test -p codex-linux-sandbox` - `cargo test -p codex-app-server --lib` - `cargo test -p codex-tui-app-server tests::embedded_app_server_start_failure_is_returned` - `cargo clippy -p codex-linux-sandbox --all-targets` - `cargo clippy -p codex-app-server --all-targets` - `cargo clippy -p codex-tui-app-server --all-targets`
This commit is contained in:
committed by
GitHub
Unverified
parent
98be562fd3
commit
0d1539e74c
@@ -477,6 +477,14 @@ pub async fn run_main_with_transport(
|
||||
range: None,
|
||||
});
|
||||
}
|
||||
if let Some(warning) = codex_core::config::missing_system_bwrap_warning() {
|
||||
config_warnings.push(ConfigWarningNotification {
|
||||
summary: warning,
|
||||
details: None,
|
||||
path: None,
|
||||
range: None,
|
||||
});
|
||||
}
|
||||
|
||||
let feedback = CodexFeedback::new();
|
||||
|
||||
|
||||
@@ -60,6 +60,11 @@ only when the split filesystem policy round-trips through the legacy
|
||||
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.
|
||||
|
||||
### All Platforms
|
||||
|
||||
Expects the binary containing `codex-core` to simulate the virtual `apply_patch` CLI when `arg1` is `--codex-run-as-apply-patch`. See the `codex-arg0` crate for details.
|
||||
|
||||
@@ -30,6 +30,7 @@ use pretty_assertions::assert_eq;
|
||||
|
||||
use std::collections::BTreeMap;
|
||||
use std::collections::HashMap;
|
||||
use std::path::Path;
|
||||
use std::time::Duration;
|
||||
use tempfile::TempDir;
|
||||
|
||||
@@ -5498,6 +5499,18 @@ shell_tool = true
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[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()
|
||||
);
|
||||
|
||||
#[cfg(not(target_os = "linux"))]
|
||||
assert!(missing_system_bwrap_warning().is_none());
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn approvals_reviewer_defaults_to_manual_only_without_guardian_feature() -> std::io::Result<()>
|
||||
{
|
||||
|
||||
@@ -140,12 +140,30 @@ pub(crate) const DEFAULT_AGENT_JOB_MAX_RUNTIME_SECONDS: Option<u64> = None;
|
||||
|
||||
pub const CONFIG_TOML_FILE: &str = "config.toml";
|
||||
const OPENAI_BASE_URL_ENV_VAR: &str = "OPENAI_BASE_URL";
|
||||
#[cfg(target_os = "linux")]
|
||||
const SYSTEM_BWRAP_PATH: &str = "/usr/bin/bwrap";
|
||||
const RESERVED_MODEL_PROVIDER_IDS: [&str; 3] = [
|
||||
OPENAI_PROVIDER_ID,
|
||||
OLLAMA_OSS_PROVIDER_ID,
|
||||
LMSTUDIO_OSS_PROVIDER_ID,
|
||||
];
|
||||
|
||||
#[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."
|
||||
))
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(not(target_os = "linux"))]
|
||||
pub fn missing_system_bwrap_warning() -> Option<String> {
|
||||
None
|
||||
}
|
||||
|
||||
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();
|
||||
|
||||
@@ -663,6 +663,12 @@ 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() {
|
||||
let _ = event_processor.process_event(Event {
|
||||
id: String::new(),
|
||||
msg: EventMsg::Warning(codex_protocol::protocol::WarningEvent { message }),
|
||||
});
|
||||
}
|
||||
|
||||
info!("Codex initialized with event: {session_configured:?}");
|
||||
|
||||
|
||||
@@ -7,13 +7,20 @@ This crate is responsible for producing:
|
||||
- the `codex-exec` CLI can check if its arg0 is `codex-linux-sandbox` and, if so, execute as if it were `codex-linux-sandbox`
|
||||
- this should also be true of the `codex` multitool CLI
|
||||
|
||||
On Linux, the bubblewrap pipeline uses the vendored bubblewrap path compiled
|
||||
into this binary.
|
||||
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
|
||||
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.
|
||||
|
||||
**Current Behavior**
|
||||
- Legacy `SandboxPolicy` / `sandbox_mode` configs remain supported.
|
||||
- Bubblewrap is the default filesystem sandbox pipeline and is standardized on
|
||||
the vendored path.
|
||||
- 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.
|
||||
- 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`)
|
||||
|
||||
@@ -0,0 +1,134 @@
|
||||
use std::ffi::CString;
|
||||
use std::fs::File;
|
||||
use std::os::fd::AsRawFd;
|
||||
use std::os::raw::c_char;
|
||||
use std::os::unix::ffi::OsStrExt;
|
||||
use std::path::Path;
|
||||
|
||||
use crate::vendored_bwrap::exec_vendored_bwrap;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
|
||||
const SYSTEM_BWRAP_PATH: &str = "/usr/bin/bwrap";
|
||||
|
||||
#[derive(Debug, Clone, PartialEq, Eq)]
|
||||
enum BubblewrapLauncher {
|
||||
System(AbsolutePathBuf),
|
||||
Vendored,
|
||||
}
|
||||
|
||||
pub(crate) fn exec_bwrap(argv: Vec<String>, preserved_files: Vec<File>) -> ! {
|
||||
match preferred_bwrap_launcher() {
|
||||
BubblewrapLauncher::System(program) => exec_system_bwrap(&program, argv, preserved_files),
|
||||
BubblewrapLauncher::Vendored => exec_vendored_bwrap(argv, preserved_files),
|
||||
}
|
||||
}
|
||||
|
||||
fn preferred_bwrap_launcher() -> BubblewrapLauncher {
|
||||
if !Path::new(SYSTEM_BWRAP_PATH).is_file() {
|
||||
return BubblewrapLauncher::Vendored;
|
||||
}
|
||||
|
||||
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}"),
|
||||
};
|
||||
BubblewrapLauncher::System(system_bwrap_path)
|
||||
}
|
||||
|
||||
fn exec_system_bwrap(
|
||||
program: &AbsolutePathBuf,
|
||||
argv: Vec<String>,
|
||||
preserved_files: Vec<File>,
|
||||
) -> ! {
|
||||
// System bwrap runs across an exec boundary, so preserved fds must survive exec.
|
||||
make_files_inheritable(&preserved_files);
|
||||
|
||||
let program_path = program.as_path().display().to_string();
|
||||
let program = CString::new(program.as_path().as_os_str().as_bytes())
|
||||
.unwrap_or_else(|err| panic!("invalid system bubblewrap path: {err}"));
|
||||
let cstrings = argv_to_cstrings(&argv);
|
||||
let mut argv_ptrs: Vec<*const c_char> = cstrings.iter().map(|arg| arg.as_ptr()).collect();
|
||||
argv_ptrs.push(std::ptr::null());
|
||||
|
||||
// SAFETY: `program` and every entry in `argv_ptrs` are valid C strings for
|
||||
// the duration of the call. On success `execv` does not return.
|
||||
unsafe {
|
||||
libc::execv(program.as_ptr(), argv_ptrs.as_ptr());
|
||||
}
|
||||
let err = std::io::Error::last_os_error();
|
||||
panic!("failed to exec system bubblewrap {program_path}: {err}");
|
||||
}
|
||||
|
||||
fn argv_to_cstrings(argv: &[String]) -> Vec<CString> {
|
||||
let mut cstrings: Vec<CString> = Vec::with_capacity(argv.len());
|
||||
for arg in argv {
|
||||
match CString::new(arg.as_str()) {
|
||||
Ok(value) => cstrings.push(value),
|
||||
Err(err) => panic!("failed to convert argv to CString: {err}"),
|
||||
}
|
||||
}
|
||||
cstrings
|
||||
}
|
||||
|
||||
fn make_files_inheritable(files: &[File]) {
|
||||
for file in files {
|
||||
clear_cloexec(file.as_raw_fd());
|
||||
}
|
||||
}
|
||||
|
||||
fn clear_cloexec(fd: libc::c_int) {
|
||||
// SAFETY: `fd` is an owned descriptor kept alive by `files`.
|
||||
let flags = unsafe { libc::fcntl(fd, libc::F_GETFD) };
|
||||
if flags < 0 {
|
||||
let err = std::io::Error::last_os_error();
|
||||
panic!("failed to read fd flags for preserved bubblewrap file descriptor {fd}: {err}");
|
||||
}
|
||||
let cleared_flags = flags & !libc::FD_CLOEXEC;
|
||||
if cleared_flags == flags {
|
||||
return;
|
||||
}
|
||||
|
||||
// SAFETY: `fd` is valid and we are only clearing FD_CLOEXEC.
|
||||
let result = unsafe { libc::fcntl(fd, libc::F_SETFD, cleared_flags) };
|
||||
if result < 0 {
|
||||
let err = std::io::Error::last_os_error();
|
||||
panic!("failed to clear CLOEXEC for preserved bubblewrap file descriptor {fd}: {err}");
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use pretty_assertions::assert_eq;
|
||||
use tempfile::NamedTempFile;
|
||||
|
||||
#[test]
|
||||
fn preserved_files_are_made_inheritable_for_system_exec() {
|
||||
let file = NamedTempFile::new().expect("temp file");
|
||||
set_cloexec(file.as_file().as_raw_fd());
|
||||
|
||||
make_files_inheritable(std::slice::from_ref(file.as_file()));
|
||||
|
||||
assert_eq!(fd_flags(file.as_file().as_raw_fd()) & libc::FD_CLOEXEC, 0);
|
||||
}
|
||||
|
||||
fn set_cloexec(fd: libc::c_int) {
|
||||
let flags = fd_flags(fd);
|
||||
// SAFETY: `fd` is valid for the duration of the test.
|
||||
let result = unsafe { libc::fcntl(fd, libc::F_SETFD, flags | libc::FD_CLOEXEC) };
|
||||
if result < 0 {
|
||||
let err = std::io::Error::last_os_error();
|
||||
panic!("failed to set CLOEXEC for test fd {fd}: {err}");
|
||||
}
|
||||
}
|
||||
|
||||
fn fd_flags(fd: libc::c_int) -> libc::c_int {
|
||||
// SAFETY: `fd` is valid for the duration of the test.
|
||||
let flags = unsafe { libc::fcntl(fd, libc::F_GETFD) };
|
||||
if flags < 0 {
|
||||
let err = std::io::Error::last_os_error();
|
||||
panic!("failed to read fd flags for test fd {fd}: {err}");
|
||||
}
|
||||
flags
|
||||
}
|
||||
}
|
||||
@@ -8,6 +8,8 @@ mod bwrap;
|
||||
#[cfg(target_os = "linux")]
|
||||
mod landlock;
|
||||
#[cfg(target_os = "linux")]
|
||||
mod launcher;
|
||||
#[cfg(target_os = "linux")]
|
||||
mod linux_run_main;
|
||||
#[cfg(target_os = "linux")]
|
||||
mod proxy_routing;
|
||||
|
||||
@@ -11,10 +11,9 @@ use crate::bwrap::BwrapNetworkMode;
|
||||
use crate::bwrap::BwrapOptions;
|
||||
use crate::bwrap::create_bwrap_command_args;
|
||||
use crate::landlock::apply_sandbox_policy_to_current_thread;
|
||||
use crate::launcher::exec_bwrap;
|
||||
use crate::proxy_routing::activate_proxy_routes_in_netns;
|
||||
use crate::proxy_routing::prepare_host_proxy_route_spec;
|
||||
use crate::vendored_bwrap::exec_vendored_bwrap;
|
||||
use crate::vendored_bwrap::run_vendored_bwrap_main;
|
||||
use codex_protocol::protocol::FileSystemSandboxPolicy;
|
||||
use codex_protocol::protocol::NetworkSandboxPolicy;
|
||||
use codex_protocol::protocol::SandboxPolicy;
|
||||
@@ -434,7 +433,7 @@ fn run_bwrap_with_proc_fallback(
|
||||
command_cwd,
|
||||
options,
|
||||
);
|
||||
exec_vendored_bwrap(bwrap_args.args, bwrap_args.preserved_files);
|
||||
exec_bwrap(bwrap_args.args, bwrap_args.preserved_files);
|
||||
}
|
||||
|
||||
fn bwrap_network_mode(
|
||||
@@ -568,8 +567,7 @@ fn run_bwrap_in_child_capture_stderr(bwrap_args: crate::bwrap::BwrapArgs) -> Str
|
||||
close_fd_or_panic(write_fd, "close write end in bubblewrap child");
|
||||
}
|
||||
|
||||
let exit_code = run_vendored_bwrap_main(&bwrap_args.args, &bwrap_args.preserved_files);
|
||||
std::process::exit(exit_code);
|
||||
exec_bwrap(bwrap_args.args, bwrap_args.preserved_files);
|
||||
}
|
||||
|
||||
// Parent: close the write end and read stderr while the child runs.
|
||||
|
||||
@@ -76,4 +76,3 @@ Notes:
|
||||
}
|
||||
|
||||
pub(crate) use imp::exec_vendored_bwrap;
|
||||
pub(crate) use imp::run_vendored_bwrap_main;
|
||||
|
||||
@@ -275,6 +275,16 @@ 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 {
|
||||
return;
|
||||
};
|
||||
|
||||
app_event_tx.send(AppEvent::InsertHistoryCell(Box::new(
|
||||
history_cell::new_warning_event(message),
|
||||
)));
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, PartialEq, Eq)]
|
||||
struct SessionSummary {
|
||||
usage_line: String,
|
||||
@@ -1963,6 +1973,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);
|
||||
tui.set_notification_method(config.tui_notification_method);
|
||||
|
||||
let harness_overrides =
|
||||
|
||||
@@ -286,6 +286,16 @@ 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 {
|
||||
return;
|
||||
};
|
||||
|
||||
app_event_tx.send(AppEvent::InsertHistoryCell(Box::new(
|
||||
history_cell::new_warning_event(message),
|
||||
)));
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, PartialEq, Eq)]
|
||||
struct SessionSummary {
|
||||
usage_line: String,
|
||||
@@ -2383,6 +2393,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);
|
||||
tui.set_notification_method(config.tui_notification_method);
|
||||
|
||||
let harness_overrides =
|
||||
|
||||
Reference in New Issue
Block a user