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

> [!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: https://github.com/bytecodealliance/system-interface/blob/8ee8c825df4c183f06d48ec5d10556a5bf11655b/src/io/read_ready.rs#L75-L78

According to [RFC 3128](https://rust-lang.github.io/rfcs/3128-io-safety.html), `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:

```diff
- 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](https://www.rfc-editor.org/rfc/rfc3128)) 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:**

```rust

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](https://www.rfc-editor.org/rfc/rfc3128)), `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:

```rust

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:**

```rust

// 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:**

```rust

fn reopen(filelike: &Filelike) -> io::Result {
let file = filelike.as_filelike_view::();
unsafe { _reopen(&file) }
}

unsafe fn _reopen(file: &fs::File) -> io::Result {
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:**

```rust

// 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:**

```rust

// 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:**

```rust

// 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`
- **Proposed Docstring:**

```rust

/// # 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:**

```rust

// 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`
- **Proposed Docstring:**

```rust

/// # 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:**

```rust

// 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`
- **Proposed Docstring:**

```rust

/// # 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:**

```rust

// 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` 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` 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.

```