Skip to content

Commit 16d0012

Browse files
committed
fix(sandbox): document unsafe blocks and enforce SAFETY comments with clippy
Every non-test unsafe block in openshell-sandbox now carries a // SAFETY: comment stating its invariant, and clippy::undocumented_unsafe_blocks is enabled at warn level in the crate roots (lib, the sandbox bin, and the seccomp-perf bin), so the CI clippy lanes with -D warnings fail the build on new undocumented blocks. Test-only blocks are exempt through cfg_attr(test, allow) to keep this change free of test-comment churn; the lint fires 89 times on main and zero times after this change. reset_child_signal_dispositions gains a # Safety section documenting its caller contract (forked pre-exec child only), and the vsock control stream reinterpreted as a UnixStream is documented in place: the family-agnostic operations it relies on, the AF_UNIX members that must never be called on it, and the dial-side twin of the same reinterpretation. Small mechanical edits were required so each comment sits on the line directly above its unsafe block (the lint is line-anchored): nine expressions hoisted into let bindings or nested ifs (five syscall probes in main.rs, two F_SETFL guards in network_broker.rs, two in boundary_server.rs), plus two reorderings (a timeout binding in network_broker.rs, the SAFETY comment placement in seccomp_notify.rs). Every movement preserves the original short-circuits and the errno adjacency the probe helpers rely on, and four new tests pin those properties: two run the self-protection probes against seccomp filters (one lets the FIOSETOWN ioctl through to the kernel, so a probe that reads another syscall's errno fails), and two cover the F_SETFL error arms in connect_exact (an O_PATH descriptor for the transition failure, a deny filter for the restore failure). multiple_unsafe_ops_per_block is deliberately not enabled: it is restriction-group and needs block splits at production sites owned by #5180. Closes #5188 Signed-off-by: Emilien Macchi <emacchi@redhat.com>
1 parent 74eacee commit 16d0012

8 files changed

Lines changed: 438 additions & 35 deletions

File tree

‎crates/openshell-sandbox/src/bin/seccomp-perf.rs‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,10 @@
33

44
//! Microbenchmark entry point for the production seccomp network broker.
55
6+
// Crate-scoped: Cargo rejects mixing `[lints] workspace = true` with a
7+
// crate-specific lint table (see #5188).
8+
#![warn(clippy::undocumented_unsafe_blocks)]
9+
610
#[cfg(target_os = "linux")]
711
use std::net::SocketAddr;
812

‎crates/openshell-sandbox/src/boundary_server.rs‎

Lines changed: 34 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4828,6 +4828,8 @@ mod linux {
48284828
.map_err(|_| {
48294829
io::Error::new(io::ErrorKind::InvalidInput, "sockaddr_vm exceeds socklen_t")
48304830
})?;
4831+
// SAFETY: socket() creates a fresh descriptor and takes only
4832+
// scalar arguments; the result is checked below before any use.
48314833
let raw_fd = unsafe {
48324834
libc::socket(
48334835
libc::AF_VSOCK,
@@ -4838,6 +4840,8 @@ mod linux {
48384840
if raw_fd < 0 {
48394841
return Err(io::Error::last_os_error());
48404842
}
4843+
// SAFETY: raw_fd was checked non-negative above and is uniquely
4844+
// owned; this is its single wrap into an OwnedFd.
48414845
let fd = unsafe { OwnedFd::from_raw_fd(raw_fd) };
48424846
let address = libc::sockaddr_vm {
48434847
svm_family: family,
@@ -4846,6 +4850,8 @@ mod linux {
48464850
svm_cid: libc::VMADDR_CID_ANY,
48474851
svm_zero: [0; 4],
48484852
};
4853+
// SAFETY: bind operates on the live OwnedFd with a fully
4854+
// initialized sockaddr_vm of the declared length.
48494855
let result = unsafe {
48504856
libc::bind(
48514857
fd.as_raw_fd(),
@@ -4856,6 +4862,7 @@ mod linux {
48564862
if result < 0 {
48574863
return Err(io::Error::last_os_error());
48584864
}
4865+
// SAFETY: listen operates on the live, bound OwnedFd.
48594866
if unsafe { libc::listen(fd.as_raw_fd(), 16) } < 0 {
48604867
return Err(io::Error::last_os_error());
48614868
}
@@ -4879,6 +4886,9 @@ mod linux {
48794886
listener,
48804887
server_config,
48814888
} => {
4889+
// SAFETY: accept4 takes the live listener descriptor with
4890+
// SOCK_CLOEXEC and a valid null address buffer; the
4891+
// returned descriptor is checked below before any use.
48824892
let raw_fd = unsafe {
48834893
libc::accept4(
48844894
listener.as_raw_fd(),
@@ -4890,8 +4900,11 @@ mod linux {
48904900
if raw_fd < 0 {
48914901
Err(io::Error::last_os_error())
48924902
} else {
4903+
// SAFETY: raw_fd was checked non-negative above and
4904+
// is uniquely owned; this is its single wrap.
4905+
let stream = unsafe { File::from_raw_fd(raw_fd) };
48934906
Ok(ControlStream::PendingTls {
4894-
stream: PlainControlStream::Vsock(unsafe { File::from_raw_fd(raw_fd) }),
4907+
stream: PlainControlStream::Vsock(stream),
48954908
server_config: server_config.clone(),
48964909
})
48974910
}
@@ -5086,8 +5099,26 @@ mod linux {
50865099
) -> io::Result<openshell_isolation_interface::contract::BoundaryDuplexStream> {
50875100
match self {
50885101
Self::Vsock(file) => {
5089-
let stream =
5090-
unsafe { std::os::unix::net::UnixStream::from_raw_fd(file.into_raw_fd()) };
5102+
let raw_fd = file.into_raw_fd();
5103+
// SAFETY: `into_raw_fd` handed over a valid, open,
5104+
// solely-owned descriptor exactly once, so this is its
5105+
// single re-wrap. The descriptor is an AF_VSOCK stream
5106+
// reinterpreted as a Unix stream; the stream only ever
5107+
// sees family-agnostic operations (fcntl, read/write,
5108+
// epoll registration via `from_std`, and shutdown), which
5109+
// behave the same on AF_VSOCK streams. `peer_addr`,
5110+
// `local_addr`, and `peer_cred` must never be called on
5111+
// it: they would fail at runtime on a vsock descriptor.
5112+
// The surface is fenced by this private enum and by the
5113+
// `Box<dyn DuplexStream>` erasure in `into_tokio`. The
5114+
// dial side performs the same reinterpretation in the
5115+
// `RuntimeInitiatedVsock` arm of `dial_supervisor`, but
5116+
// files its vsock descriptors under the `Unix` variant
5117+
// (not `Vsock`) and uses only `take_error()` (a
5118+
// SOL_SOCKET getsockopt) and `writable()`, both
5119+
// family-agnostic; the allowed-ops list above covers
5120+
// this accept-side stream only.
5121+
let stream = unsafe { std::os::unix::net::UnixStream::from_raw_fd(raw_fd) };
50915122
stream.set_nonblocking(true)?;
50925123
Ok(Box::new(tokio::net::UnixStream::from_std(stream)?))
50935124
}

‎crates/openshell-sandbox/src/lib.rs‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,11 @@
33

44
//! Capability-free in-workload sandbox boundary.
55
6+
// Crate-scoped: Cargo rejects mixing `[lints] workspace = true` with a
7+
// crate-specific lint table (see #5188).
8+
#![warn(clippy::undocumented_unsafe_blocks)]
9+
#![cfg_attr(test, allow(clippy::undocumented_unsafe_blocks))]
10+
611
pub mod binary_pins;
712
#[cfg(target_os = "linux")]
813
pub mod boundary_exec;

‎crates/openshell-sandbox/src/linux/seccomp_notify.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -831,8 +831,6 @@ fn verify_notification_sizes() -> io::Result<()> {
831831
}
832832

833833
fn ioctl_ptr(fd: RawFd, request: libc::c_ulong, argument: *mut libc::c_void) -> io::Result<i64> {
834-
// SAFETY: every caller supplies the UAPI structure encoded into `request`,
835-
// alive and writable for the ioctl duration.
836834
// `libc::ioctl` models the request as `c_ulong` for glibc and `c_int`
837835
// for musl. Linux UAPI request values fit both representations.
838836
#[cfg(target_env = "musl")]
@@ -841,6 +839,8 @@ fn ioctl_ptr(fd: RawFd, request: libc::c_ulong, argument: *mut libc::c_void) ->
841839
})?;
842840
#[cfg(target_env = "musl")]
843841
let request = libc::c_int::from_ne_bytes(request.to_ne_bytes());
842+
// SAFETY: every caller supplies the UAPI structure encoded into `request`,
843+
// alive and writable for the ioctl duration.
844844
let result = unsafe { libc::ioctl(fd, request, argument) };
845845
if result < 0 {
846846
Err(io::Error::last_os_error())

0 commit comments

Comments
 (0)