mirror of
https://github.com/openai/codex.git
synced 2026-11-06 20:20:34 +00:00
fix(linux-sandbox): preserve detached children
Co-authored-by: Codex noreply@openai.com
This commit is contained in:
co-authored by
Codex noreply@openai.com
parent
e2551a5e36
commit
913d8afeb4
@@ -65,6 +65,8 @@ pub(crate) struct BwrapOptions {
|
||||
pub mount_proc: bool,
|
||||
/// How networking should be configured inside the bubblewrap sandbox.
|
||||
pub network_mode: BwrapNetworkMode,
|
||||
/// How the sandbox lifetime should relate to the initial command process.
|
||||
pub process_lifetime: BwrapProcessLifetime,
|
||||
/// Optional maximum depth for expanding unreadable glob patterns with ripgrep.
|
||||
///
|
||||
/// Keep this uncapped by default so existing nested deny-read matches are
|
||||
@@ -77,11 +79,22 @@ impl Default for BwrapOptions {
|
||||
Self {
|
||||
mount_proc: true,
|
||||
network_mode: BwrapNetworkMode::FullAccess,
|
||||
process_lifetime: BwrapProcessLifetime::TerminateWithParent,
|
||||
glob_scan_max_depth: None,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// Lifetime behavior for descendants running inside bubblewrap.
|
||||
#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)]
|
||||
pub(crate) enum BwrapProcessLifetime {
|
||||
/// Ask bubblewrap to kill the sandbox when its parent exits.
|
||||
#[default]
|
||||
TerminateWithParent,
|
||||
/// Allow intentionally detached children to keep running in the sandbox.
|
||||
AllowDetachedChildren,
|
||||
}
|
||||
|
||||
/// Network policy modes for bubblewrap.
|
||||
#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)]
|
||||
pub(crate) enum BwrapNetworkMode {
|
||||
@@ -265,9 +278,11 @@ pub(crate) fn create_bwrap_command_args(
|
||||
}
|
||||
|
||||
fn create_bwrap_flags_full_filesystem(command: Vec<String>, options: BwrapOptions) -> BwrapArgs {
|
||||
let mut args = vec![
|
||||
"--new-session".to_string(),
|
||||
"--die-with-parent".to_string(),
|
||||
let mut args = vec!["--new-session".to_string()];
|
||||
if options.process_lifetime == BwrapProcessLifetime::TerminateWithParent {
|
||||
args.push("--die-with-parent".to_string());
|
||||
}
|
||||
args.extend([
|
||||
"--bind".to_string(),
|
||||
"/".to_string(),
|
||||
"/".to_string(),
|
||||
@@ -275,7 +290,7 @@ fn create_bwrap_flags_full_filesystem(command: Vec<String>, options: BwrapOption
|
||||
// not need ambient CAP_SYS_ADMIN to create the remaining namespaces.
|
||||
"--unshare-user".to_string(),
|
||||
"--unshare-pid".to_string(),
|
||||
];
|
||||
]);
|
||||
if options.network_mode.should_unshare_network() {
|
||||
args.push("--unshare-net".to_string());
|
||||
}
|
||||
@@ -316,7 +331,9 @@ fn create_bwrap_flags(
|
||||
let normalized_command_cwd = normalize_command_cwd_for_bwrap(command_cwd);
|
||||
let mut args = Vec::new();
|
||||
args.push("--new-session".to_string());
|
||||
args.push("--die-with-parent".to_string());
|
||||
if options.process_lifetime == BwrapProcessLifetime::TerminateWithParent {
|
||||
args.push("--die-with-parent".to_string());
|
||||
}
|
||||
args.extend(filesystem_args);
|
||||
// Request a user namespace explicitly rather than relying on bubblewrap's
|
||||
// auto-enable behavior, which is skipped when the caller runs as uid 0.
|
||||
@@ -1411,6 +1428,31 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn allow_detached_children_omits_die_with_parent_but_keeps_pid_namespace() {
|
||||
let policy = FileSystemSandboxPolicy::restricted(vec![FileSystemSandboxEntry {
|
||||
path: FileSystemPath::Special {
|
||||
value: FileSystemSpecialPath::Root,
|
||||
},
|
||||
access: FileSystemAccessMode::Read,
|
||||
}]);
|
||||
let args = create_bwrap_command_args(
|
||||
vec!["/bin/true".to_string()],
|
||||
&policy,
|
||||
Path::new("/"),
|
||||
Path::new("/"),
|
||||
BwrapOptions {
|
||||
process_lifetime: BwrapProcessLifetime::AllowDetachedChildren,
|
||||
..Default::default()
|
||||
},
|
||||
)
|
||||
.expect("create bwrap args");
|
||||
|
||||
assert!(!args.args.contains(&"--die-with-parent".to_string()));
|
||||
assert!(args.args.contains(&"--unshare-pid".to_string()));
|
||||
assert!(args.args.contains(&"--unshare-user".to_string()));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn full_disk_write_with_unreadable_glob_still_wraps_and_masks_match() {
|
||||
if !ripgrep_available() {
|
||||
|
||||
@@ -19,6 +19,7 @@ use std::time::Duration;
|
||||
|
||||
use crate::bwrap::BwrapNetworkMode;
|
||||
use crate::bwrap::BwrapOptions;
|
||||
use crate::bwrap::BwrapProcessLifetime;
|
||||
use crate::bwrap::create_bwrap_command_args;
|
||||
use crate::landlock::apply_permission_profile_to_current_thread;
|
||||
use crate::launcher::exec_bwrap;
|
||||
@@ -122,6 +123,11 @@ pub struct LandlockCommand {
|
||||
#[arg(long = "allow-network-for-proxy", hide = true, default_value_t = false)]
|
||||
pub allow_network_for_proxy: bool,
|
||||
|
||||
/// Internal: permit intentionally detached command descendants to outlive
|
||||
/// the one-shot sandbox helper invocation.
|
||||
#[arg(long = "allow-detached-children", hide = true, default_value_t = false)]
|
||||
pub allow_detached_children: bool,
|
||||
|
||||
/// Internal route spec used for managed proxy routing in bwrap mode.
|
||||
#[arg(long = "proxy-route-spec", hide = true)]
|
||||
pub proxy_route_spec: Option<String>,
|
||||
@@ -152,6 +158,7 @@ pub fn run_main() -> ! {
|
||||
use_legacy_landlock,
|
||||
apply_seccomp_then_exec,
|
||||
allow_network_for_proxy,
|
||||
allow_detached_children,
|
||||
proxy_route_spec,
|
||||
no_proc,
|
||||
command,
|
||||
@@ -238,6 +245,11 @@ pub fn run_main() -> ! {
|
||||
inner,
|
||||
!no_proc,
|
||||
allow_network_for_proxy,
|
||||
if allow_detached_children {
|
||||
BwrapProcessLifetime::AllowDetachedChildren
|
||||
} else {
|
||||
BwrapProcessLifetime::TerminateWithParent
|
||||
},
|
||||
);
|
||||
}
|
||||
|
||||
@@ -322,6 +334,7 @@ fn run_bwrap_with_proc_fallback(
|
||||
inner: Vec<String>,
|
||||
mount_proc: bool,
|
||||
allow_network_for_proxy: bool,
|
||||
process_lifetime: BwrapProcessLifetime,
|
||||
) -> ! {
|
||||
let network_mode = bwrap_network_mode(network_sandbox_policy, allow_network_for_proxy);
|
||||
let mut mount_proc = mount_proc;
|
||||
@@ -344,6 +357,7 @@ fn run_bwrap_with_proc_fallback(
|
||||
let options = BwrapOptions {
|
||||
mount_proc,
|
||||
network_mode,
|
||||
process_lifetime,
|
||||
..Default::default()
|
||||
};
|
||||
let mut bwrap_args = build_bwrap_argv(
|
||||
|
||||
@@ -21,6 +21,7 @@ use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use pretty_assertions::assert_eq;
|
||||
use std::collections::HashMap;
|
||||
use std::path::PathBuf;
|
||||
use std::time::Duration;
|
||||
use tempfile::NamedTempFile;
|
||||
|
||||
// At least on GitHub CI, the arm64 tests appear to need longer timeouts.
|
||||
@@ -393,6 +394,47 @@ async fn sandbox_ignores_missing_writable_roots_under_bwrap() {
|
||||
assert_eq!(output.stdout.text, "sandbox-ok");
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn detached_children_survive_parent_exit_under_bwrap() {
|
||||
if should_skip_bwrap_tests().await {
|
||||
eprintln!("skipping bwrap test: bwrap sandbox prerequisites are unavailable");
|
||||
return;
|
||||
}
|
||||
|
||||
let tempdir = tempfile::tempdir().expect("tempdir");
|
||||
let log_path = tempdir.path().join("detached.log");
|
||||
let command = format!(
|
||||
"nohup sh -c 'for i in 1 2 3 4 5 6 7 8 9 10; do printf x >> \"{log}\"; sleep 0.2; done' >/dev/null 2>&1 </dev/null &",
|
||||
log = log_path.to_string_lossy(),
|
||||
);
|
||||
|
||||
let output = run_cmd_result_with_writable_roots(
|
||||
&["bash", "-lc", &command],
|
||||
&[tempdir.path().to_path_buf()],
|
||||
LONG_TIMEOUT_MS,
|
||||
/*use_legacy_landlock*/ false,
|
||||
/*network_access*/ true,
|
||||
)
|
||||
.await
|
||||
.expect("sandboxed command should execute");
|
||||
|
||||
assert_eq!(output.exit_code, 0);
|
||||
|
||||
tokio::time::sleep(Duration::from_millis(500)).await;
|
||||
let first_len = std::fs::metadata(&log_path)
|
||||
.expect("detached child should create log file")
|
||||
.len();
|
||||
|
||||
tokio::time::sleep(Duration::from_millis(500)).await;
|
||||
let second_len = std::fs::metadata(&log_path)
|
||||
.expect("detached child log file should still exist")
|
||||
.len();
|
||||
assert!(
|
||||
second_len > first_len,
|
||||
"detached child should keep writing after the parent exits",
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_no_new_privs_is_enabled() {
|
||||
let output = run_cmd_output(
|
||||
|
||||
@@ -53,6 +53,7 @@ pub fn create_linux_sandbox_command_args_for_permission_profile(
|
||||
if allow_network_for_proxy {
|
||||
linux_cmd.push("--allow-network-for-proxy".to_string());
|
||||
}
|
||||
linux_cmd.push("--allow-detached-children".to_string());
|
||||
linux_cmd.push("--".to_string());
|
||||
linux_cmd.extend(command);
|
||||
linux_cmd
|
||||
|
||||
@@ -77,6 +77,7 @@ fn permission_profile_flag_is_included() {
|
||||
.any(|window| window[0] == "--command-cwd" && window[1] == "/tmp/link"),
|
||||
true
|
||||
);
|
||||
assert!(args.contains(&"--allow-detached-children".to_string()));
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
||||
Reference in New Issue
Block a user