mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
fix(linux-sandbox): fall back when system bwrap lacks perms (#20628)
## Why Codex `0.128` started using `--perms` in more routine Linux sandbox construction when protected workspace metadata mounts landed in #19852. Upstream bubblewrap added `--perms` in `v0.5.0`, so system `bwrap` versions older than that, including the `v0.4.0` and `v0.4.1` family, do not support the flag. The launcher still selected those binaries as long as they existed on `PATH`. That means affected hosts can fail every sandboxed command up front with: ```text bwrap: Unknown option --perms ``` The reports in #20590 and duplicate #20623 match that compatibility gap; #20623 explicitly shows system bubblewrap `0.4.0`. ## What changed - Replace the single `--argv0` probe with a small system-bwrap capability probe in `codex-rs/linux-sandbox/src/launcher.rs`. - Continue using the old-system `--argv0` compatibility path when needed, but only select a system `bwrap` if it also advertises `--perms`. - Fall back to the vendored `bwrap` when the system binary is too old for the flags Codex now requires. - Add regression coverage for the old-system-bwrap case so binaries without `--perms` stay on the vendored path. ## Verification - Added `falls_back_to_vendored_when_system_bwrap_lacks_perms` to cover the reported compatibility gap. - Ran `cargo test -p codex-linux-sandbox` and `cargo clippy -p codex-linux-sandbox --tests` locally. On macOS, the crate builds but its Linux-only tests are cfg-gated out, so the new regression test still needs Linux CI or a Linux devbox run for real execution coverage. ## Related issues - Fixes #20590 - Duplicate report: #20623
This commit is contained in:
committed by
GitHub
Unverified
parent
541e99cf09
commit
5b80f87c97
@@ -198,7 +198,7 @@ set -euo pipefail
|
||||
|
||||
for arg in "$@"; do
|
||||
if [[ "${arg}" == "--help" ]]; then
|
||||
echo "Usage: bwrap --argv0"
|
||||
echo "Usage: bwrap --argv0 --perms"
|
||||
exit 0
|
||||
fi
|
||||
done
|
||||
|
||||
@@ -23,6 +23,12 @@ struct SystemBwrapLauncher {
|
||||
supports_argv0: bool,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
|
||||
struct SystemBwrapCapabilities {
|
||||
supports_argv0: bool,
|
||||
supports_perms: bool,
|
||||
}
|
||||
|
||||
pub(crate) fn exec_bwrap(argv: Vec<String>, preserved_files: Vec<File>) -> ! {
|
||||
match preferred_bwrap_launcher() {
|
||||
BubblewrapLauncher::System(launcher) => {
|
||||
@@ -43,18 +49,24 @@ fn preferred_bwrap_launcher() -> BubblewrapLauncher {
|
||||
}
|
||||
|
||||
fn preferred_bwrap_launcher_for_path(system_bwrap_path: &Path) -> BubblewrapLauncher {
|
||||
preferred_bwrap_launcher_for_path_with_probe(system_bwrap_path, system_bwrap_supports_argv0)
|
||||
preferred_bwrap_launcher_for_path_with_probe(system_bwrap_path, system_bwrap_capabilities)
|
||||
}
|
||||
|
||||
fn preferred_bwrap_launcher_for_path_with_probe(
|
||||
system_bwrap_path: &Path,
|
||||
system_bwrap_supports_argv0: impl FnOnce(&Path) -> bool,
|
||||
system_bwrap_capabilities: impl FnOnce(&Path) -> Option<SystemBwrapCapabilities>,
|
||||
) -> BubblewrapLauncher {
|
||||
if !system_bwrap_path.is_file() {
|
||||
return BubblewrapLauncher::Vendored;
|
||||
}
|
||||
|
||||
let supports_argv0 = system_bwrap_supports_argv0(system_bwrap_path);
|
||||
let Some(SystemBwrapCapabilities {
|
||||
supports_argv0,
|
||||
supports_perms: true,
|
||||
}) = system_bwrap_capabilities(system_bwrap_path)
|
||||
else {
|
||||
return BubblewrapLauncher::Vendored;
|
||||
};
|
||||
let system_bwrap_path = match AbsolutePathBuf::from_absolute_path(system_bwrap_path) {
|
||||
Ok(path) => path,
|
||||
Err(err) => panic!(
|
||||
@@ -75,7 +87,7 @@ pub(crate) fn preferred_bwrap_supports_argv0() -> bool {
|
||||
}
|
||||
}
|
||||
|
||||
fn system_bwrap_supports_argv0(system_bwrap_path: &Path) -> bool {
|
||||
fn system_bwrap_capabilities(system_bwrap_path: &Path) -> Option<SystemBwrapCapabilities> {
|
||||
// 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
|
||||
@@ -83,11 +95,14 @@ fn system_bwrap_supports_argv0(system_bwrap_path: &Path) -> bool {
|
||||
// in that case.
|
||||
let output = match Command::new(system_bwrap_path).arg("--help").output() {
|
||||
Ok(output) => output,
|
||||
Err(_) => return false,
|
||||
Err(_) => return None,
|
||||
};
|
||||
let stdout = String::from_utf8_lossy(&output.stdout);
|
||||
let stderr = String::from_utf8_lossy(&output.stderr);
|
||||
stdout.contains("--argv0") || stderr.contains("--argv0")
|
||||
Some(SystemBwrapCapabilities {
|
||||
supports_argv0: stdout.contains("--argv0") || stderr.contains("--argv0"),
|
||||
supports_perms: stdout.contains("--perms") || stderr.contains("--perms"),
|
||||
})
|
||||
}
|
||||
|
||||
fn exec_system_bwrap(
|
||||
@@ -164,7 +179,12 @@ mod tests {
|
||||
let expected = AbsolutePathBuf::from_absolute_path(fake_bwrap_path).expect("absolute");
|
||||
|
||||
assert_eq!(
|
||||
preferred_bwrap_launcher_for_path_with_probe(fake_bwrap_path, |_| true),
|
||||
preferred_bwrap_launcher_for_path_with_probe(fake_bwrap_path, |_| {
|
||||
Some(SystemBwrapCapabilities {
|
||||
supports_argv0: true,
|
||||
supports_perms: true,
|
||||
})
|
||||
}),
|
||||
BubblewrapLauncher::System(SystemBwrapLauncher {
|
||||
program: expected,
|
||||
supports_argv0: true,
|
||||
@@ -178,7 +198,12 @@ mod tests {
|
||||
let fake_bwrap_path = fake_bwrap.path();
|
||||
|
||||
assert_eq!(
|
||||
preferred_bwrap_launcher_for_path_with_probe(fake_bwrap_path, |_| false),
|
||||
preferred_bwrap_launcher_for_path_with_probe(fake_bwrap_path, |_| {
|
||||
Some(SystemBwrapCapabilities {
|
||||
supports_argv0: false,
|
||||
supports_perms: true,
|
||||
})
|
||||
}),
|
||||
BubblewrapLauncher::System(SystemBwrapLauncher {
|
||||
program: AbsolutePathBuf::from_absolute_path(fake_bwrap_path).expect("absolute"),
|
||||
supports_argv0: false,
|
||||
@@ -186,6 +211,21 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn falls_back_to_vendored_when_system_bwrap_lacks_perms() {
|
||||
let fake_bwrap = NamedTempFile::new().expect("temp file");
|
||||
|
||||
assert_eq!(
|
||||
preferred_bwrap_launcher_for_path_with_probe(fake_bwrap.path(), |_| {
|
||||
Some(SystemBwrapCapabilities {
|
||||
supports_argv0: false,
|
||||
supports_perms: false,
|
||||
})
|
||||
}),
|
||||
BubblewrapLauncher::Vendored
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn falls_back_to_vendored_when_system_bwrap_is_missing() {
|
||||
assert_eq!(
|
||||
|
||||
Reference in New Issue
Block a user