Soundness: Violation of I/O Safety via Unwound Double-Close in `ReadReady::num_ready_bytes`

[Manishearth]

Note

This finding was identified during an agentic unsafe Rust code review performed by Gemini AI, followed by human review and verification.

The Issue

ReadReady::num_ready_bytes for std::fs::File constructs an owned std::fs::File wrapper directly from self's raw OS file descriptor or handle via from_raw_filelike to determine remaining bytes without modifying current stream position:

system-interface/src/io/read_ready.rs

Lines 75 to 78 in 8ee8c82

	let mut tmp = unsafe { std::fs::File::from_raw_filelike(self.as_raw_filelike()) };
	let current = tmp.seek(SeekFrom::Current(0));
	std::mem::forget(tmp);
	return Ok(metadata.len() - current?);

According to RFC 3128, std::fs::File assumes exclusive ownership over the lifecycle of its underlying OS file descriptor or handle.

The implementation invokes tmp.seek(...) on line 76 and only afterwards calls std::mem::forget(tmp) on line 77. If tmp.seek (or any thread profiling or hooking mechanism) panics or unwinds before line 77 is reached, tmp will be dropped during stack unwinding. Its destructor (File::drop) will close the underlying OS file descriptor or handle while self (and any cloned File instances or borrowed handles across concurrent threads) remains alive and believes it owns the open descriptor.

This is probably minor, but worth fixing.

Suggested Fix

Wrap the owned file wrapper immediately in std::mem::ManuallyDrop upon creation, ensuring its destructor can never execute regardless of unwinding:

- let mut tmp = unsafe { std::fs::File::from_raw_filelike(self.as_raw_filelike()) };
+ let mut tmp = std::mem::ManuallyDrop::new(unsafe { std::fs::File::from_raw_filelike(self.as_raw_filelike()) });
  let current = tmp.seek(SeekFrom::Current(0));
- std::mem::forget(tmp);
  return Ok(metadata.len() - current?);

---

Note

The full audit report below also contains additional minor findings (such as missing safety comments or undocumented FFI assumptions) that are probably worth fixing as well but not the primary goal of this issue. The audit report has not been human-reviewed, it may contain misleading claims.

Full Gemini Codebase Audit Report Appendix

Unsafe Rust Review: system_interface (v0_27)

Overall Safety Assessment

system_interface (v0_27) provides extension traits (FileIoExt, IoExt, ReadReady, IsReadWrite) aimed at extending standard library I/O types (std::fs::File, std::net::TcpStream, Stdin, etc.) with POSIX/Windows-specific capabilities like vectored reads at offsets, immediate read readiness queries, and peek operations.

The crate contains a moderate density of unsafe code distributed across Windows FFI calls (PeekNamedPipe, ioctlsocket, send, recv), OS handle conversions (from_raw_filelike), and raw pointer slicing polyfills (advance and advance_mut).

From a safety documentation perspective, the crate exhibits limited safety documentation: unsafe blocks generally lack formal // SAFETY: proof comments, and private unsafe fn helpers omit # Safety docstrings detailing their contracts. More significantly, auditing revealed two Critical findings: (1) an I/O Safety violation where unwinding during File::seek causes a double-close vulnerability on borrowed file descriptors, and (2) an intentional bad pointer dereference (usize::MAX) passed to Windows recv() to test socket shutdown state.

Critical Findings

1. Violation of I/O Safety (RFC 3128) via Unwound Double-Close in ReadReady::num_ready_bytes 🔴 🧪

• Priority: 🔴 High

• Threat Vector: 🧪 Contrived Setup

• Bug Type: I/O Safety Violation

• Location: src/io/read_ready.rs:75-77

• Code:

  let mut tmp = unsafe { std::fs::File::from_raw_filelike(self.as_raw_filelike()) };
  let current = tmp.seek(SeekFrom::Current(0));
  std::mem::forget(tmp);
  return Ok(metadata.len() - current?);

• Description: To determine the remaining bytes in a regular file without modifying its current stream position, the implementation constructs an owned std::fs::File wrapper (tmp) directly from self's raw OS file descriptor/handle via from_raw_filelike. It subsequently invokes tmp.seek(...) on line 76 and only afterwards calls std::mem::forget(tmp) on line 77. Under Rust's I/O safety conventions (RFC 3128), std::fs::File assumes exclusive ownership over the lifecycle of its underlying file descriptor/handle. If tmp.seek (or any thread interruption/hooking mechanism) panics or unwinds before line 77 is reached, tmp will be dropped during stack unwinding. Its destructor (File::drop) will close the underlying OS file descriptor/handle while self (and any cloned File instances or borrowed handles across concurrent threads) remains alive and believes it owns the open descriptor. This leads to use-after-close or file descriptor misdelivery vulnerabilities when the OS reassigns the closed descriptor number to a subsequent open file in another thread.

• Remediation: Wrap the owned file wrapper immediately in std::mem::ManuallyDrop upon creation, ensuring its destructor can never execute regardless of unwinding:

  let mut tmp = std::mem::ManuallyDrop::new(unsafe { std::fs::File::from_raw_filelike(self.as_raw_filelike()) });
  let current = tmp.seek(SeekFrom::Current(0));
  return Ok(metadata.len() - current?);

2. ASan-Fatal Bad Pointer Dereference and Brittle Exception Trap Reliance in raw_socket_is_read_write 🔴 ⚠️

• Priority: 🔴 High

• Threat Vector: ⚠️ Accidental Misuse

• Bug Type: Invalid Pointer Dereference

• Location: src/io/is_read_write.rs:144

• Code:

  // Detect read shutdown. A normal zero-length `recv` does block, so
  // use deliberately invalid pointer, as we get different error codes in
  // the case of a shut-down stream.
  let read_result = unsafe { recv(socket, usize::MAX as *mut _, 1, MSG_PEEK) };

• Description: To distinguish between an open blocking socket with no pending network packets and a socket shut down for reading (WSAESHUTDOWN), the implementation passes an unallocated, unaligned bogus address (usize::MAX as *mut _ / 0xFFFFFFFFFFFFFFFF) with length 1 to the Windows recv() Winsock system call. The author included a helpful code comment explaining the intent: a normal zero-length recv would block, so an invalid pointer is intentionally passed to leverage error code differences (WSAEFAULT, 10014) on shut-down streams. However, relying on the OS/kernel to safely catch invalid pointer access violations presents serious issues:

1. Dynamic Analysis & Sanitizer Incompatibility: Under AddressSanitizer (ASan), Valgrind, Dr. Memory, or API-hooking endpoint detection and response (EDR) agents, passing 0xFFFFFFFFFFFFFFFF with len = 1 to recv() is immediately trapped as an illegal memory access or heap buffer overflow attempt, crashing the process with a fatal sanitizer abort.
2. Undefined Behavior in Foreign Function Contracts: In C / POSIX / Winsock specifications, passing an invalid pointer to recv when length > 0 is undefined behavior under foreign call contracts. Assuming foreign syscall wrappers will safely convert bad pointers into EFAULT / WSAEFAULT rather than exhibiting undefined behavior or corrupting state violates safety preconditions.
3. Indefinite Thread Hanging: If layered service providers (LSPs) or network drivers block waiting for network packets before validating user-mode buffer pointers, calling is_read_write() on any idle open socket will hang the calling thread indefinitely.

• Remediation: Allocate a valid 1-byte stack buffer (let mut buf = [0u8; 1];) and pass buf.as_mut_ptr() with MSG_PEEK. Alternatively, use non-blocking socket queries (ioctlsocket with FIONREAD or WSAPoll / select).

Fishy Findings

1. Undocumented Preconditions on unsafe fn _reopen Helpers and Unsound Safe Abstraction 🟠 ⚠️

• Priority: 🟠 Medium

• Threat Vector: ⚠️ Accidental Misuse

• Bug Type: Unsound Safe Abstraction

• Location: src/fs/file_io_ext.rs:1005-1037

• Code:

  fn reopen<Filelike: AsFilelike>(filelike: &Filelike) -> io::Result<fs::File> {
      let file = filelike.as_filelike_view::<std::fs::File>();
      unsafe { _reopen(&file) }
  }
  
  unsafe fn _reopen(file: &fs::File) -> io::Result<fs::File> {
      file.reopen(cap_fs_ext::OpenOptions::new().read(true))
  }

• Description: The functions _reopen, _reopen_write, and _reopen_append are marked unsafe fn because they delegate to cap_fs_ext::Reopen::reopen, which is an unsafe fn on Windows. Reopening a file handle by object ID or path on Windows requires strict assumptions regarding filesystem capabilities (e.g., OpenFileById support) and absence of concurrent handle redirection or sharing mode conflicts. However, none of these unsafe fn declarations possess # Safety docstrings defining their safety obligations. Furthermore, the private helper fn reopen wraps _reopen in a completely safe function accepting any Filelike: AsFilelike. Calling reopen from safe trait methods (read_at, read_exact_at, read_vectored_at) without documenting or proving why cap_fs_ext's safety obligations are unconditionally satisfied for all std::fs::File objects leaves an unverified gap in the crate's safety architecture.

2. Code Duplication of Subtle Unsafe Pointer Slicing Polyfills 🟡 ⚠️

• Priority: 🟡 Low

• Threat Vector: ⚠️ Accidental Misuse

• Bug Type: Code Duplication

• Location: src/fs/file_io_ext.rs:362, 394 and src/io/io_ext.rs:168, 200

• Description: The polyfill functions advance and advance_mut—which perform raw pointer arithmetic (ptr.add) and lifetime reconstruction (slice::from_raw_parts) on IoSlice and IoSliceMut—are duplicated line-for-line across the filesystem (fs) and I/O (io) modules. Maintaining duplicate copies of non-trivial unsafe pointer manipulation increases auditing burden and creates risk of divergence if soundness patches are applied to only one module.

Missing Safety Comments

The codebase contains 18 locations where unsafe operations lack formal safety comments or docstrings. Below are the exact file:line locations along with rigorous proposed proof obligations:

1. src/fs/file_io_ext.rs:362 🔴

• Context: unsafe { ptr = ptr.add(advance_by); ... } inside advance.

• Proposed Proof Comment:

  // SAFETY: `first` is a valid `IoSlice` whose length satisfies `accumulated_len + first.len() > n`, ensuring `advance_by < first.len()`. `ptr` points to the contiguous slice memory, so `ptr.add(advance_by)` remains within the same allocated object. `len` is `first.len() - advance_by > 0`. `slice::from_raw_parts` preserves the valid lifetime `'a` and alignment (1 byte for `u8`) of the original slice.

2. src/fs/file_io_ext.rs:394 🔴

• Context: unsafe { ptr = ptr.add(advance_by); ... } inside advance_mut.

• Proposed Proof Comment:

  // SAFETY: `first` is uniquely borrowed (`&mut`) and `advance_by < first.len()`. `ptr.add(advance_by)` remains within the bounds of the allocated buffer. `slice::from_raw_parts_mut` creates a non-aliasing mutable subslice of length `len = first.len() - advance_by` preserving lifetime `'a`.

3. src/fs/file_io_ext.rs:1007 🔴

• Context: unsafe { _reopen(&file) } inside reopen.

• Proposed Proof Comment:

  // SAFETY: `file` is a valid open `std::fs::File` handle. Reopening it via `_reopen` is safe because `file` represents a regular filesystem object supporting reopening by object ID.

4. src/fs/file_io_ext.rs:1011 🔴

• Context: unsafe fn _reopen(file: &fs::File) -> io::Result<fs::File>

• Proposed Docstring:

  /// # Safety
  /// `file` must be a valid open file handle pointing to a filesystem object that supports reopening by object ID or path without causing undefined handle redirection behavior.
  

5. src/fs/file_io_ext.rs:1019 🔴

• Context: unsafe { _reopen_write(&file) } inside reopen_write.

• Proposed Proof Comment:

  // SAFETY: `file` is a valid open `std::fs::File` handle supporting reopening for write access.

6. src/fs/file_io_ext.rs:1023 🔴

• Context: unsafe fn _reopen_write(file: &fs::File) -> io::Result<fs::File>

• Proposed Docstring:

  /// # Safety
  /// `file` must be a valid open file handle pointing to a filesystem object that supports reopening for write access without violating OS sharing constraints.
  

7. src/fs/file_io_ext.rs:1031 🔴

• Context: unsafe { _reopen_append(&file) } inside reopen_append.

• Proposed Proof Comment:

  // SAFETY: `file` is a valid open `std::fs::File` handle supporting reopening for append access.

8. src/fs/file_io_ext.rs:1035 🔴

• Context: unsafe fn _reopen_append(file: &fs::File) -> io::Result<fs::File>

• Proposed Docstring:

  /// # Safety
  /// `file` must be a valid open file handle pointing to a filesystem object that supports reopening for append access.
  

9. src/io/io_ext.rs:168 🔴

• Context: unsafe { ptr = ptr.add(advance_by); ... } inside advance.

• Proposed Proof Comment:

  // SAFETY: `first` is a valid `IoSlice` and `advance_by < first.len()`. `ptr.add(advance_by)` remains within the allocated memory object. `slice::from_raw_parts` reconstructs a valid subslice for lifetime `'a`.

10. src/io/io_ext.rs:200 🔴

• Context: unsafe { ptr = ptr.add(advance_by); ... } inside advance_mut.

• Proposed Proof Comment:

  ```rust
  
  // SAFETY: `first` is uniquely borrowed and `advance_by < first.len()`. `ptr.add(advance_by)` remains within the allocated buffer. `slice::from_raw_parts_mut` creates a non-aliasing mutable subslice for lifetime `'a`.
  
  ```
  

11. src/io/io_ext.rs:313 🔴

• Context: unsafe { PeekNamedPipe(...) } inside IoExt::peek for std::fs::File on Windows.

• Proposed Proof Comment:

  ```rust
  
  // SAFETY: `self.as_raw_handle()` is a valid OS handle. `buf.as_mut_ptr()` points to a buffer of at least `buf.len()` bytes, and `len` is clamped to `min(buf.len(), u32::MAX)`. `bytes_read.as_mut_ptr()` points to a valid allocated `MaybeUninit<u32>` on the stack. All null pointers passed for optional parameters are accepted by `PeekNamedPipe`.
  
  ```
  

12. src/io/io_ext.rs:326 🔴

• Context: unsafe { bytes_read.assume_init() } inside IoExt::peek.

• Proposed Proof Comment:

  ```rust
  
  // SAFETY: `res != 0` indicates that `PeekNamedPipe` succeeded, which guarantees that the number of bytes read was written into `bytes_read`.
  
  ```
  

13. src/io/read_ready.rs:75 🔴

• Context: unsafe { std::fs::File::from_raw_filelike(...) } inside ReadReady::num_ready_bytes for std::fs::File.

• Proposed Proof Comment:

  ```rust
  
  // SAFETY: `self.as_raw_filelike()` is a valid open file descriptor/handle. (Note: To prevent I/O safety violations on unwinding before `mem::forget`, this instance must be wrapped in `ManuallyDrop`).
  
  ```
  

14. src/io/read_ready.rs:240 🔴

• Context: unsafe { ioctlsocket(...) } inside ReadReady::num_ready_bytes for TcpStream on Windows.

• Proposed Proof Comment:

  ```rust
  
  // SAFETY: `self.as_raw_socket()` is a valid Winsock `SOCKET`. `arg.as_mut_ptr()` points to a valid allocated `MaybeUninit<c_ulong>` on the stack. `FIONREAD` is a valid ioctl command that writes the readable byte count into `arg`.
  
  ```
  

15. src/io/read_ready.rs:241 🔴

• Context: unsafe { arg.assume_init() } inside ReadReady::num_ready_bytes for TcpStream on Windows.

• Proposed Proof Comment:

  ```rust
  
  // SAFETY: `ioctlsocket` returning 0 indicates success, guaranteeing that `arg` was initialized with the readable byte count.
  
  ```
  

16. src/io/is_read_write.rs:131 🟡

• Context: unsafe { send(...) } inside raw_socket_is_read_write.

• Proposed Proof Comment:

  ```rust
  
  // SAFETY: `socket` is passed by value. Passing a null buffer pointer with length 0 to `send` is permitted by Winsock to probe socket write status without sending data.
  
  ```
  

17. src/io/is_read_write.rs:144 🟡

• Context: unsafe { recv(...) } inside raw_socket_is_read_write.

• Proposed Proof Comment:

  ```rust
  
  // SAFETY: (Note: Unsound as written due to `usize::MAX` pointer). When passing a valid 1-byte buffer pointer `buf.as_mut_ptr()`, calling `recv` with `MSG_PEEK` safely probes socket read availability without consuming data.
  
  ```
  

18. tests/sys_common/io.rs:9 🟡

• Context: unsafe { tempdir(...) } inside tmpdir.

• Proposed Proof Comment:

  ```rust
  
  // SAFETY: Called within test execution context where ambient filesystem authority is available and permitted.
  
  ```