Soundness: Dangling OS Handle Reuse via Unsound Lifetime Escaping in HandleRef

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

This is an IO safety issue. I [personally am not a huge fan](https://github.com/rust-lang/unsafe-code-guidelines/issues/434) of IO safety being considered the same type of safety as regular safety, but I know there are good reasons for it.

### The Issue

The primary handle abstraction in this crate is the handle pair `Handle` (owned) and `HandleRef` (borrowed). `HandleRef` is documented as a borrowed representation of a valid Windows handle. However, `HandleRef` contains no lifetime parameter tracking the duration of the borrow (its struct definition holds only `'static` references).

When safe constructors such as `HandleRef::from_file(file: &File)` or trait conversions via `AsHandleRef::as_handle_ref(&self)` are invoked on owned I/O objects (`File`, `Handle`, `ChildStdin`, etc.), they execute `unsafe { HandleRef::from_raw_handle(file.as_raw_handle()) }`. Inside `HandleRef::from_raw_handle`, `std::fs::File::from_raw_handle(handle)` is invoked to construct an owned `File` object stored inside `HandleRefInner`.

Locations:

- https://github.com/BurntSushi/winapi-util/blob/803874c57dc1f10ecd42f7c86d9de72f53818432/src/win.rs#L145-L147
- https://github.com/BurntSushi/winapi-util/blob/803874c57dc1f10ecd42f7c86d9de72f53818432/src/win.rs#L194-L198
- https://github.com/BurntSushi/winapi-util/blob/803874c57dc1f10ecd42f7c86d9de72f53818432/src/win.rs#L206-L210
- https://github.com/BurntSushi/winapi-util/blob/803874c57dc1f10ecd42f7c86d9de72f53818432/src/win.rs#L230-L246

Based on the [`from_raw_handle`](https://doc.rust-lang.org/stable/std/os/windows/io/trait.FromRawHandle.html#tymethod.from_raw_handle) documentation, callers have to uphold: *"The caller must ensure that the handle is valid and that nobody else will close it for the lifetime of the returned object."*

Because `file` is passed by reference `&File`, safe code can drop `file` while keeping the returned `HandleRef` alive. When `file` drops, its `Drop` implementation invokes OS `CloseHandle`. At this point, the `File` object stored inside `HandleRef` wraps a closed OS handle, violating the `FromRawHandle` safety contract.

If another thread or component concurrently opens a file, pipe, socket, registry key, or IPC mechanism, the Windows kernel may recycle the closed handle value. Safe code calling methods on the escaped `HandleRef` (such as `href.as_file()`, `file::information`, or `console::mode`) will perform I/O operations or attribute mutations on an unrelated OS resource, causing unintended I/O corruption, memory safety violations (for example, if the recycled resource is an internal memory mapping or IPC handle), and UB.

Minimal Reproduction (Miri)

This testcase needs `MIRIFLAGS=-Zmiri-disable-isolation` and `--target x86_64-pc-windows-msvc` (I'm surprised that works)

```rust
use std::fs::File;
use std::io::{Read, Write};
use winapi_util::HandleRef;

fn main() {
let temp_path = "winapi_util_repro_temp.txt";
{
let mut f = File::create(temp_path).unwrap();
writeln!(f, "test data").unwrap();
}

let mut href: HandleRef = {
let f = File::open(temp_path).unwrap();
HandleRef::from_file(&f)
// `f` drops here, closing the OS handle.
};

// `href` now holds a dangling OS handle.
// Try to read from the dangling handle.
let mut buf = [0; 10];
let res = href.as_file_mut().read(&mut buf);
println!("Read result: {:?}", res);
}
```

```text
error: abnormal termination: invalid handle passed to `NtReadFile`
--> /usr/local/google/home/manishearth/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/sys/pal/windows/handle.rs:260:13
|
260 | / c::NtReadFile(
261 | | self.as_raw_handle(),
262 | | ptr::null_mut(),
263 | | None,
... |
269 | | ptr::null(),
270 | | )
| |_____________^ abnormal termination occurred here
|
= note: stack backtrace:
0: std::sys::pal::windows::handle::Handle::synchronous_read
at /usr/local/google/home/manishearth/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/sys/pal/windows/handle.rs:260:13: 270:14
1: std::sys::pal::windows::handle::Handle::read
at /usr/local/google/home/manishearth/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/sys/pal/windows/handle.rs:77:28: 77:91
2: std::sys::fs::windows::File::read
at /usr/local/google/home/manishearth/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/sys/fs/windows.rs:619:9: 619:30
3: <&std::fs::File as std::io::Read>::read
at /usr/local/google/home/manishearth/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/fs.rs:1336:9: 1336:29
4: ::read
at /usr/local/google/home/manishearth/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/fs.rs:1496:9: 1496:27
5: main
at src/bin/repro1.rs:21:15: 21:48
6: >::call_once - shim(fn())
at /usr/local/google/home/manishearth/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:250:5: 250:71
7: std::sys::backtrace::__rust_begin_short_backtrace::
at /usr/local/google/home/manishearth/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/sys/backtrace.rs:166:18: 166:21
8: std::rt::lang_start::<()>::{closure#0}
at /usr/local/google/home/manishearth/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/rt.rs:206:18: 206:75
9: std::ops::function::impls:: for &dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe>::call_once
at /usr/local/google/home/manishearth/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:287:13: 287:31
10: std::panicking::catch_unwind::do_call::<&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe, i32>
at /usr/local/google/home/manishearth/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:581:40: 581:43
11: std::panicking::catch_unwind:: i32 + std::marker::Sync + std::panic::RefUnwindSafe>
at /usr/local/google/home/manishearth/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:544:19: 544:88
12: std::panic::catch_unwind::<&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe, i32>
at /usr/local/google/home/manishearth/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panic.rs:359:14: 359:40
13: std::rt::lang_start_internal::{closure#0}
at /usr/local/google/home/manishearth/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/rt.rs:175:24: 175:49
14: std::panicking::catch_unwind::do_call::<{closure@std::rt::lang_start_internal::{closure#0}}, isize>
at /usr/local/google/home/manishearth/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:581:40: 581:43
15: std::panicking::catch_unwind::
at /usr/local/google/home/manishearth/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:544:19: 544:88
16: std::panic::catch_unwind::<{closure@std::rt::lang_start_internal::{closure#0}}, isize>
at /usr/local/google/home/manishearth/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panic.rs:359:14: 359:40
17: std::rt::lang_start_internal
at /usr/local/google/home/manishearth/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/rt.rs:171:5: 193:7
18: std::rt::lang_start::<()>
at /usr/local/google/home/manishearth/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/rt.rs:205:5: 210:6
```

Suggested Fix

Modern Rust (1.63+) resolves this via RFC 3128 ("I/O Safety"). `HandleRef` should either be redesigned to carry a lifetime parameter `HandleRef<'a>` holding a `std::os::windows::io::BorrowedHandle<'a>`, or deprecated in favor of standard library `BorrowedHandle`.

---

> [!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: `winapi_util` (`v0_1`)

## Overall Safety Assessment

`winapi_util` is a small Windows utility crate designed to provide safe wrappers around common Windows system APIs (`windows-sys`), centralizing `unsafe` code for operations on console attributes, file information, and system properties.

The primary architectural abstraction in the crate is the handle pair `Handle` (owned) and `HandleRef` (borrowed). Under the hood, `HandleRef` wraps `Option` inside a custom wrapper (`HandleRefInner`) whose `Drop` implementation extracts the `File` and calls `into_raw_handle()`, intentionally suppressing OS `CloseHandle` when the borrowed reference is dropped.

While the crate succeeds in offering convenient wrappers for Win32 console and file information APIs, its core `HandleRef` abstraction predates Rust 1.63's I/O Safety stabilization (RFC 3128 `BorrowedHandle<'a>`). Because `HandleRef` is un-lifetimed (`'static`), safe constructors and trait conversions from owned I/O objects (`File`, `Handle`, `ChildStdin`, etc.) allow borrowed handles to escape the lifetime of their owners. This violates the fundamental safety contract of `std::fs::File::from_raw_handle` and introduces a real soundness vulnerability (dangling handle reuse). Furthermore, almost all `unsafe` blocks and functions across `win.rs`, `file.rs`, and `console.rs` lack `// SAFETY:` proof-obligation comments.

## Critical Findings

### 1. Unsound Lifetime Escaping in `HandleRef` and `AsHandleRef` (Dangling OS Handle Reuse) 🔴 🤦

- **Severity**: 🔴 High
- **Threat Vector**: 🤦 Accidental Misuse
- **Bug Type**: `Dangling OS Handle Reuse`

- **Locations**:

- `src/win.rs:145-147` (`HandleRef::from_file`)
- `src/win.rs:194-198` (`impl AsHandleRef for Handle`)
- `src/win.rs:206-210` (`impl AsHandleRef for File`)
- `src/win.rs:230-246` (`impl AsHandleRef for process::ChildStdin`, `ChildStdout`, `ChildStderr`)

- **Description**: `HandleRef` is documented as a borrowed representation of a valid Windows handle. However, `HandleRef` contains no lifetime parameter tracking the duration of the borrow (i.e., its type contains only `'static` references). When `HandleRef::from_file(file: &File)` is called in safe code, it executes `unsafe { HandleRef::from_raw_handle(file.as_raw_handle()) }`. Inside `HandleRef::from_raw_handle`, `std::fs::File::from_raw_handle(handle)` is invoked to construct an owned `File` object stored inside `HandleRefInner`. According to authoritative standard library documentation (`std::os::windows::io::FromRawHandle::from_raw_handle`), callers must uphold a strict safety contract: *"The caller must ensure that the handle is valid and that nobody else will close it for the lifetime of the returned object."* Because `file` is passed by reference `&File`, safe code can drop `file` while keeping the returned `HandleRef` alive. When `file` drops, its `Drop` implementation invokes OS `CloseHandle`. At this point, the `File` object inside `HandleRef` wraps a closed OS handle, violating the `FromRawHandle` safety contract. If another thread or component subsequently opens a file, pipe, registry key, or IPC mechanism, the Windows kernel may recycle the closed handle value. Safe code calling methods on the escaped `HandleRef` (such as `href.as_file_mut()`, `file::information`, or `console::mode`) will perform I/O operations or attribute mutations on an unrelated OS resource, causing unintended I/O corruption, memory safety violations (e.g., if the recycled resource is an internal memory mapping or IPC handle), and undefined behavior.

- **Proof of Vulnerability (Safe Code Trigger)**:

```rust
use std::fs::File;
use winapi_util::HandleRef;

let href: HandleRef = {
let f = File::open("temporary.txt").unwrap();
HandleRef::from_file(&f)
// `f` drops here, closing OS handle H.
};
// `href` holds dangling handle H.
// If OS reallocates H to another resource opened concurrently,
// safe code using `href.as_file()` reads/writes unintended targets.
```

- **Remediation**: Modern Rust (1.63+) resolves this via RFC 3128 ("I/O Safety"). `HandleRef` should either be redesigned to carry a lifetime parameter `HandleRef<'a>` holding a `std::os::windows::io::BorrowedHandle<'a>`, or deprecated in favor of standard library `BorrowedHandle`.

## Fishy Findings

### 1. Safe Trait `AsHandleRef` Allows Override of `as_raw` 🟡 🤸

- **Severity**: 🟡 Low
- **Threat Vector**: 🤸 Deliberate Contortion
- **Bug Type**: `Unsafe FFI Reliance on Safe Trait`

- **Location**: `src/win.rs:177-186`

- **Description**: `AsHandleRef` is a safe trait with a default method `fn as_raw(&self) -> RawHandle { self.as_handle_ref().as_raw_handle() }`. Safe code implementing `AsHandleRef` for a custom type can override `as_raw(&self)` to return an arbitrary pointer value (e.g., `0xdeadbeef as *mut _`). Public safe functions such as `file::information`, `file::typ`, and `console::mode` call `h.as_raw()` and pass the resulting raw handle directly to Win32 FFI functions. While passing invalid handle values to these specific Win32 query/mode APIs safely fails with `ERROR_INVALID_HANDLE` at the OS kernel level (avoiding UB), relying on FFI target robustness against arbitrary safe trait overrides without safety comments or making `AsHandleRef` an `unsafe trait` is a questionable design pattern.

### 2. Incomplete Safety Contract Documentation on `HandleRef::from_raw_handle` 🟡 🤦

- **Severity**: 🟡 Low
- **Threat Vector**: 🤦 Accidental Misuse
- **Bug Type**: `Incomplete Safety Documentation`

- **Location**: `src/win.rs:156-163`

- **Description**: The `# Safety` documentation for `pub unsafe fn from_raw_handle` states: *"This is unsafe because there is no guarantee that the given raw handle is a valid handle. The caller must ensure this is true before invoking this constructor."* However, because `from_raw_handle` wraps the raw handle in `Some(File::from_raw_handle(handle))`, it inherits the safety preconditions of `std::fs::File::from_raw_handle`. Specifically, callers must not only guarantee that the handle is currently valid, but also that *nobody else will close it for the lifetime of the returned `HandleRef`*. Omitting this critical liveness requirement from the `# Safety` docstring misleads callers into thinking they can pass a borrowed raw handle whose owner might close it later.

## Missing Safety Comments

**`src/file.rs`** 🔴

- **Line 21**: `unsafe {` inside `pub fn information`. 🔴
- **Proposed Proof Comment**:

```rust
// SAFETY: BY_HANDLE_FILE_INFORMATION is a Win32 struct composed entirely of plain integer fields (u32), making zero-initialization via mem::zeroed() sound. GetFileInformationByHandle is an FFI call expecting a raw handle and a valid mutable pointer to receive file info; passing &mut info is sound. If h.as_raw() is invalid or closed, the Win32 API safely returns 0 (FALSE) and sets LastError without causing undefined behavior.
```

- **Line 39**: `unsafe {` inside `pub fn typ`. 🔴
- **Proposed Proof Comment**:

```rust
// SAFETY: GetFileType is an FFI call accepting a raw HANDLE. If h.as_raw() is invalid or closed, GetFileType safely returns FILE_TYPE_UNKNOWN (0) without causing undefined behavior.
```

**`src/console.rs`** 🔴

- **Line 34**: `unsafe {` inside `pub fn screen_buffer_info`. 🔴
- **Proposed Proof Comment**:

```rust
// SAFETY: CONSOLE_SCREEN_BUFFER_INFO is a Win32 struct composed entirely of integer types (SHORT, WORD), making zero-initialization via mem::zeroed() sound. GetConsoleScreenBufferInfo expects a HANDLE and a valid mutable pointer to info; passing h.as_raw() and &mut info satisfies FFI contract requirements. Invalid handle values safely return 0 without memory corruption.
```

- **Line 53**: `unsafe { SetConsoleTextAttribute(h.as_raw() as HANDLE, attributes) }`. 🔴
- **Proposed Proof Comment**:

```rust
// SAFETY: SetConsoleTextAttribute is a Win32 FFI routine modifying console display attributes for the provided handle. Passing h.as_raw() and a u16 attribute bitmask is safe; invalid handles return 0 (FALSE).
```

- **Line 70**: `unsafe { GetConsoleMode(h.as_raw() as HANDLE, &mut mode) }`. 🔴
- **Proposed Proof Comment**:

```rust
// SAFETY: GetConsoleMode is a Win32 FFI routine that writes the console mode bitmask to the provided mutable u32 pointer. Passing &mut mode is valid, and invalid handles return 0 (FALSE).
```

- **Line 84**: `unsafe { SetConsoleMode(h.as_raw() as HANDLE, mode) }`. 🔴
- **Proposed Proof Comment**:

```rust
// SAFETY: SetConsoleMode is a Win32 FFI routine setting the input/output mode of the console buffer. Passing h.as_raw() and a u32 mode bitmask is safe; invalid handles return 0 (FALSE).
```

**`src/win.rs`** 🔴

- **Line 24**: `unsafe fn from_raw_handle(handle: RawHandle) -> Handle` (trait impl). 🔴
- **Proposed Proof Comment**:

```rust
// SAFETY: By implementing FromRawHandle, this function inherits the trait's safety contract: the caller guarantees that `handle` is a valid, open Windows handle transferred to the returned `Handle`, and that no other code will close it. File::from_raw_handle relies on this exact precondition.
```

- **Line 116**: `unsafe { HandleRef::from_raw_handle(self.as_raw_handle()) }` inside `Clone for HandleRef`. 🔴
- **Proposed Proof Comment**:

```rust
// SAFETY: `self` is a valid HandleRef whose underlying handle is guaranteed to remain open for its lifetime. Creating a cloned HandleRef from `self.as_raw_handle()` is sound because HandleRef's custom Drop implementation prevents closing the OS handle when either HandleRef instance is dropped. Note: this relies on `self` outliving the clone, or the handle remaining open indefinitely.
```

- **Line 125**: `unsafe { HandleRef::from_raw_handle(io::stdin().as_raw_handle()) }`. 🔴
- **Proposed Proof Comment**:

```rust
// SAFETY: io::stdin() returns a handle to the standard input stream, which is valid and remains open for the duration of the process.
```

- **Line 132**: `unsafe { HandleRef::from_raw_handle(io::stdout().as_raw_handle()) }`. 🔴
- **Proposed Proof Comment**:

```rust
// SAFETY: io::stdout() returns a handle to the standard output stream, which is valid and remains open for the duration of the process.
```

- **Line 139**: `unsafe { HandleRef::from_raw_handle(io::stderr().as_raw_handle()) }`. 🔴
- **Proposed Proof Comment**:

```rust
// SAFETY: io::stderr() returns a handle to the standard error stream, which is valid and remains open for the duration of the process.
```

- **Line 146**: `unsafe { HandleRef::from_raw_handle(file.as_raw_handle()) }` inside `HandleRef::from_file`. 🔴
- **Proposed Proof Comment**:

```rust
// SAFETY: Note: This call is currently UNSOUND because HandleRef has a 'static lifetime and can outlive `file`. To be sound, caller/type system must guarantee `file` outlives the returned HandleRef so the raw handle remains open.
```

- **Line 162**: Inside `pub unsafe fn from_raw_handle(handle: RawHandle) -> HandleRef`. 🔴
- **Proposed Proof Comment**:

```rust
// SAFETY: The caller guarantees via the safety contract of `from_raw_handle` that `handle` is a valid handle and that nobody will close it for the duration of the returned HandleRef's liveness. File::from_raw_handle requires this liveness guarantee.
```

- **Line 196**: `unsafe { HandleRef::from_raw_handle(self.as_raw_handle()) }` inside `AsHandleRef for Handle`. 🔴
- **Proposed Proof Comment**:

```rust
// SAFETY: Note: This call is currently UNSOUND because the returned HandleRef is un-lifetimed and can outlive `self`. To be sound, `self` must outlive the returned HandleRef.
```

- **Line 214**: `unsafe { HandleRef::from_raw_handle(self.as_raw_handle()) }` inside `AsHandleRef for io::Stdin`. 🔴
- **Proposed Proof Comment**:

```rust
// SAFETY: io::Stdin wraps the process global standard input stream, which remains open for the lifetime of the process.
```

- **Line 220**: `unsafe { HandleRef::from_raw_handle(self.as_raw_handle()) }` inside `AsHandleRef for io::Stdout`. 🔴
- **Proposed Proof Comment**:

```rust
// SAFETY: io::Stdout wraps the process global standard output stream, which remains open for the lifetime of the process.
```

- **Line 226**: `unsafe { HandleRef::from_raw_handle(self.as_raw_handle()) }` inside `AsHandleRef for io::Stderr`. 🔴
- **Proposed Proof Comment**:

```rust
// SAFETY: io::Stderr wraps the process global standard error stream, which remains open for the lifetime of the process.
```

- **Line 232**: `unsafe { HandleRef::from_raw_handle(self.as_raw_handle()) }` inside `AsHandleRef for process::ChildStdin`. 🔴
- **Proposed Proof Comment**:

```rust
// SAFETY: Note: This call is currently UNSOUND because the returned HandleRef can outlive the ChildStdin instance and hold a closed handle.
```

- **Line 238**: `unsafe { HandleRef::from_raw_handle(self.as_raw_handle()) }` inside `AsHandleRef for process::ChildStdout`. 🔴
- **Proposed Proof Comment**:

```rust
// SAFETY: Note: This call is currently UNSOUND because the returned HandleRef can outlive the ChildStdout instance and hold a closed handle.
```

- **Line 244**: `unsafe { HandleRef::from_raw_handle(self.as_raw_handle()) }` inside `AsHandleRef for process::ChildStderr`. 🔴
- **Proposed Proof Comment**:

```rust
// SAFETY: Note: This call is currently UNSOUND because the returned HandleRef can outlive the ChildStderr instance and hold a closed handle.
```

Start in src/win.rs at HandleRef::from_file, HandleRef::from_raw_handle, and the AsHandleRef implementations listed in the issue. Compare the current API with Windows BorrowedHandle and the minimal reproduction; done means a HandleRef cannot outlive its source handle, or the unsafe abstraction is deprecated in favor of the standard lifetime-tracked type.

Written by the indexing model from the issue text.