From d8acdf151577132d4ee1de47a4d1822ae5acb762 Mon Sep 17 00:00:00 2001 From: felixxia-oai Date: Fri, 23 Oct 2026 12:00:32 +0000 Subject: [PATCH] Make sandbox mount cleanup tests tolerate file descriptor reuse (#56454) ## Why Concurrent tests can reuse a closed file descriptor number, causing cleanup assertions that require `EBADF` to fail even when the inherited directory descriptor was closed. ## What changed Capture the inherited directory's device and inode before mount verification. Check `/proc/self/fd` afterward and accept either a missing descriptor or one referring to a different inode, while still rejecting a leaked descriptor to the original directory. Apply this check to successful verification and rejected mount markers. GitOrigin-RevId: 42ff3aebcee9c37c481772f8a425a8643708c426 --- codex-rs/linux-sandbox/src/fd_mount_tests.rs | 56 ++++++++++++++------ 1 file changed, 39 insertions(+), 17 deletions(-) diff --git a/codex-rs/linux-sandbox/src/fd_mount_tests.rs b/codex-rs/linux-sandbox/src/fd_mount_tests.rs index fe0bf7d7d5..b6e7ad32df 100644 --- a/codex-rs/linux-sandbox/src/fd_mount_tests.rs +++ b/codex-rs/linux-sandbox/src/fd_mount_tests.rs @@ -1,7 +1,26 @@ +//! Check mount validation and cleanup without assuming closed descriptor numbers stay unused. + use super::verify_fd_mounts; use pretty_assertions::assert_eq; +use pretty_assertions::assert_ne; use std::fs::File; +use std::fs::Metadata; use std::os::fd::IntoRawFd; +use std::os::fd::RawFd; +use std::os::unix::fs::MetadataExt; + +// Another test can reuse the number after closure; only the original directory is a leak. +#[track_caller] +fn assert_inherited_descriptor_closed(descriptor: RawFd, inherited: &Metadata) { + match std::fs::metadata(format!("/proc/self/fd/{descriptor}")) { + Ok(current) => assert_ne!( + (current.dev(), current.ino()), + (inherited.dev(), inherited.ino()), + "the inherited directory descriptor must be closed" + ), + Err(error) => assert_eq!(error.kind(), std::io::ErrorKind::NotFound), + } +} /// A matching mount authenticates its original inode and closes the inherited descriptor. #[test] @@ -10,15 +29,14 @@ fn matching_mount_closes_inherited_descriptor() { let descriptor = File::open(root.path()) .expect("directory descriptor should open") .into_raw_fd(); + let descriptor_path = format!("/proc/self/fd/{descriptor}"); + let inherited = std::fs::metadata(&descriptor_path) + .expect("inherited descriptor metadata should be accessible"); let marker = format!("{descriptor}:{}", root.path().display()); verify_fd_mounts(&[marker]).expect("matching mount should verify"); - assert_eq!(unsafe { libc::fcntl(descriptor, libc::F_GETFD) }, -1); - assert_eq!( - std::io::Error::last_os_error().raw_os_error(), - Some(libc::EBADF) - ); + assert_inherited_descriptor_closed(descriptor, &inherited); } /// A swapped destination fails closed without leaking the original descriptor. @@ -29,16 +47,15 @@ fn mismatched_mount_closes_inherited_descriptor() { let descriptor = File::open(source.path()) .expect("source directory descriptor should open") .into_raw_fd(); + let descriptor_path = format!("/proc/self/fd/{descriptor}"); + let inherited = std::fs::metadata(&descriptor_path) + .expect("inherited descriptor metadata should be accessible"); let marker = format!("{descriptor}:{}", destination.path().display()); let error = verify_fd_mounts(&[marker]).expect_err("different inodes must be rejected"); assert_eq!(error.kind(), std::io::ErrorKind::PermissionDenied); - assert_eq!(unsafe { libc::fcntl(descriptor, libc::F_GETFD) }, -1); - assert_eq!( - std::io::Error::last_os_error().raw_os_error(), - Some(libc::EBADF) - ); + assert_inherited_descriptor_closed(descriptor, &inherited); } /// A symlink to the original inode is not itself the authenticated mount. @@ -53,16 +70,15 @@ fn symlinked_mount_destination_closes_inherited_descriptor() { let descriptor = File::open(&source) .expect("source directory descriptor should open") .into_raw_fd(); + let descriptor_path = format!("/proc/self/fd/{descriptor}"); + let inherited = std::fs::metadata(&descriptor_path) + .expect("inherited descriptor metadata should be accessible"); let marker = format!("{descriptor}:{}", destination.display()); let error = verify_fd_mounts(&[marker]).expect_err("symlinked mounts must be rejected"); assert_eq!(error.kind(), std::io::ErrorKind::PermissionDenied); - assert_eq!(unsafe { libc::fcntl(descriptor, libc::F_GETFD) }, -1); - assert_eq!( - std::io::Error::last_os_error().raw_os_error(), - Some(libc::EBADF) - ); + assert_inherited_descriptor_closed(descriptor, &inherited); } /// Malformed mount markers cannot claim standard streams or relative destinations. @@ -84,11 +100,14 @@ fn malformed_mount_markers_are_rejected() { let descriptor = File::open(root.path()) .expect("directory descriptor should open") .into_raw_fd(); + let descriptor_path = format!("/proc/self/fd/{descriptor}"); + let inherited = std::fs::metadata(&descriptor_path) + .expect("inherited descriptor metadata should be accessible"); let error = verify_fd_mounts(&[format!("{descriptor}:relative")]) .expect_err("relative mount destinations must be rejected"); assert_eq!(error.kind(), std::io::ErrorKind::InvalidInput); - assert_eq!(unsafe { libc::fcntl(descriptor, libc::F_GETFD) }, -1); + assert_inherited_descriptor_closed(descriptor, &inherited); } /// A transferred descriptor can be consumed only once even if a marker is repeated. @@ -98,11 +117,14 @@ fn duplicate_mount_descriptors_are_rejected() { let descriptor = File::open(root.path()) .expect("directory descriptor should open") .into_raw_fd(); + let descriptor_path = format!("/proc/self/fd/{descriptor}"); + let inherited = std::fs::metadata(&descriptor_path) + .expect("inherited descriptor metadata should be accessible"); let marker = format!("{descriptor}:{}", root.path().display()); let error = verify_fd_mounts(&[marker.clone(), marker]) .expect_err("a mount descriptor must not be consumed twice"); assert_eq!(error.kind(), std::io::ErrorKind::InvalidInput); - assert_eq!(unsafe { libc::fcntl(descriptor, libc::F_GETFD) }, -1); + assert_inherited_descriptor_closed(descriptor, &inherited); }