alexcrichton / alexcrichton/wait-timeout

Soundness: SIGCHLD implementation does not handle SIG_IGN

未关闭
#44 0 条评论 0 个 reaction 已指派 0 人 在 GitHub 查看
主要语言
Rust
星标
76
派生
24
PR 合并指标
30 天内没有已合并 PR

描述

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

### The Issue

In `src/unix.rs`, when `sigchld_handler` handles an incoming `SIGCHLD` signal, it notifies the internal notification pipe and subsequently attempts to chain execution to the host application's previously installed signal handler (`state.prev.sa_sigaction`):

https://github.com/alexcrichton/wait-timeout/blob/7bb31d4901baa5196bd554e9e332bd9534eeb93b/src/unix.rs#L288-L303

In standard POSIX signal definitions, `SIG_DFL` (default action) is represented as `0`, and `SIG_IGN` (ignore action) is represented as `1` (`libc::SIG_IGN`). The handler checks `if fnptr == 0` to properly return early when the previous action was `SIG_DFL`, but fails to check for `SIG_IGN` (`1`).

If the host application or an upstream library configured `SIGCHLD` to be ignored (`SIG_IGN`) prior to invoking `wait_timeout`, `state.prev.sa_sigaction` is saved as `1`. When a child process terminates, `sigchld_handler` transmutes the integer `1` into an executable function pointer (`FnHandler` or `FnSigaction`) and invokes `action(signum)`. This jumps execution to memory address `0x1`, triggering an immediate segmentation fault (`SIGSEGV`).

Minimal Reproduction

```rust
// Reproduction test case for Segmentation Fault when Chaining SIGCHLD to SIG_IGN
// (Demonstrates jumping to invalid memory address 0x1 upon process exit)

use std::process::Command;
use std::time::Duration;
use wait_timeout::ChildExt;

fn main() {
unsafe {
// Configure SIGCHLD to POSIX SIG_IGN (ignore action, represented as function pointer 0x1)
libc::signal(libc::SIGCHLD, libc::SIG_IGN);
}

// Spawn a child process and wait on it with a timeout.
// State::init saves SIG_IGN (0x1) into state.prev.sa_sigaction.
println!("Spawning child...");
let mut child = Command::new("sleep").arg("0.5").spawn().expect("failed to spawn child");
println!("Calling wait_timeout...");
let res = child.wait_timeout(Duration::from_secs(1));
println!("wait_timeout returned: {:?}", res);
}
```

```text
# Verified on the host (Linux) by running the compiled binary directly:

Spawning child...
Calling wait_timeout...
Segmentation fault (core dumped)
Exit code: 139
```

Suggested Fix

Explicitly check whether `fnptr` equals `libc::SIG_IGN` (or `1`) alongside `SIG_DFL` (`0`) before transmuting and invoking the handler:

```rust
let fnptr = state.prev.sa_sigaction;
if fnptr == 0 || fnptr == libc::SIG_IGN {
return
}
```

---

> [!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: `wait_timeout` (`v0_2`)

## Overall Safety Assessment
The `wait_timeout` crate (version `v0_2`) provides extension methods on `std::process::Child` to allow waiting for process termination with a specified timeout. On Windows (`src/windows.rs`), the implementation is straightforward and relies on a single FFI call to `WaitForSingleObject`. On Unix (`src/unix.rs`), however, process reaping with timeouts is complex because POSIX `wait`/`waitpid` APIs do not accept timeouts. To solve this, the crate implements a multi-threaded asynchronous waiting mechanism using global state (`static mut STATE`), a global `SIGCHLD` signal handler, and the "self-pipe trick".

The crate contains a moderate density of `unsafe` code (6 `unsafe` blocks across 2 files). While the Windows implementation is sound, the Unix implementation exhibits several severe concurrency and memory model soundness defects. Specifically, the crate contains:

1. A critical initialization race condition where the `SIGCHLD` signal handler is installed before global state is assigned, allowing incoming signals to trigger null pointer dereferences.
2. A critical signal handler chaining flaw that attempts to call `SIG_IGN` (`0x1`) as a function pointer, leading to segmentation faults.
3. A formal Stacked Borrows aliasing violation where threads maintain active `&mut Child` references on their call stacks while signal handler threads concurrently reap those children via raw pointers.
4. Async-signal-safety violations where standard library `panic!` machinery is invoked inside signal handlers upon pipe write errors.

Furthermore, none of the 6 `unsafe` blocks in the crate contain `// SAFETY:` comments documenting proof obligations. Overall, while the crate functions in typical single-threaded or low-signal scenarios, its Unix unsafe foundation is unsound under adversarial or edge-case signal conditions and violates formal Rust memory model rules.

## Critical Findings

### 1. Race Condition in Signal Handler Registration (`src/unix.rs:94-96`) 🔴 🚨

- **Severity**: 🔴 High
- **Threat Vector**: 🚨 Untrusted Input
- **Bug Type**: Signal Handler Race Condition

In `State::init()`, the crate registers `sigchld_handler` via `libc::sigaction(libc::SIGCHLD, &new, &mut state.prev)` at line 94. However, the global static pointer `STATE` is not initialized until line 96 (`STATE = mem::transmute(state)`). As soon as `sigaction` succeeds at line 94, the operating system may asynchronously deliver `SIGCHLD` signals to `sigchld_handler`. If any child process on the system terminates or a `SIGCHLD` signal arrives during the window between line 94 and line 96, `sigchld_handler` executes `let state = &*STATE;`, dereferencing `STATE` while it is still null (`0`). This results in an immediate null pointer dereference crash. To be sound, `STATE` must be fully assigned before `sigaction` is called.

### 2. Chaining `SIGCHLD` to `SIG_IGN` Causes Segmentation Fault (`src/unix.rs:288-303`) 🔴 🤦

- **Severity**: 🔴 High
- **Threat Vector**: 🤦 Accidental Misuse
- **Bug Type**: Invalid Function Pointer Dereference

In `sigchld_handler`, after notifying the internal self-pipe, the crate attempts to chain the signal to the application's previously installed signal handler (`state.prev.sa_sigaction`). It checks `if fnptr == 0` (which correctly handles `SIG_DFL`), but fails to check for `SIG_IGN` (POSIX ignore action, represented as `1` or `libc::SIG_IGN`). If the host application or another dependency previously configured `SIGCHLD` to `SIG_IGN`, `fnptr` equals `1`. The handler transmutes `1` to `FnHandler` (or `FnSigaction`) and executes `action(signum)`, jumping to memory address `0x1` and triggering a segmentation fault (SIGSEGV).

### 3. Stacked Borrows Aliasing Violation on Process Reaping (`src/unix.rs:121-136, 229`) 🔴 🤦

- **Severity**: 🔴 High
- **Threat Vector**: 🤦 Accidental Misuse
- **Bug Type**: Stacked Borrows Aliasing Violation

When a thread calls `wait_timeout(child: &mut Child, dur)`, it inserts `child as *mut Child` into the global `StateMap` (line 121) and then constructs `Remove { state: self, child }` (line 136), reborrowing `child` as `&mut Child`. While the thread blocks in `libc::poll`, `remove` remains active on its stack frame. When `SIGCHLD` arrives, another thread executing `process_sigchlds` dereferences the stored raw pointer `*k` to execute `(*k).try_wait()` (line 229). Under formal Rust aliasing semantics (Stacked Borrows and Tree Borrows), performing a mutable access via a raw pointer (`*k`) derived prior to the active reborrow (`remove.child`) invalidates `remove.child`. When the waiting thread resumes and drops `Remove`, accessing `self.child` triggers undefined behavior.

### 4. Panic in Async Signal Handler Leads to Deadlock or Heap Corruption (`src/unix.rs:255-264, 286`) 🔴 🚨

- **Severity**: 🔴 High
- **Threat Vector**: 🚨 Untrusted Input
- **Bug Type**: Async-Signal-Safety Violation

`sigchld_handler` executes asynchronously in signal context and invokes `notify(&state.write)`. If writing to the Unix stream socket fails with any error other than `WouldBlock` (e.g., `EINTR` or `EIO`), `notify` executes `panic!("bad error on write fd: ...")`. Rust's panic runtime and formatting infrastructure are not async-signal-safe (they may acquire internal mutexes or allocate heap memory). If `SIGCHLD` interrupted a thread while it held the global allocator lock or panic runtime lock, panicking inside the signal handler will permanently deadlock the application or corrupt heap structures.

## Fishy Findings

### 1. Bypassing `Sync` Bounds on Global State (`src/unix.rs:33, 48`) 🟡 🤦

- **Severity**: 🟡 Low
- **Threat Vector**: 🤦 Accidental Misuse
- **Bug Type**: Sync Trait Bound Bypass

`StateMap` is defined as `HashMap<*mut Child, ...>`. Because raw pointers (`*mut Child`) are `!Send` and `!Sync`, `StateMap` is `!Send`, making `Mutex` `!Sync`, and consequently `State` is `!Sync`. In safe Rust, storing a type in a shared static requires `T: Sync`. By placing `State` behind a `static mut STATE: *mut State` raw pointer and accessing `&*STATE` across threads inside `unsafe` blocks, the author bypassed compiler enforcement of `Sync`. While `Child: Send` makes cross-thread reaping safe at the OS level, using raw pointers in statics to bypass type-system `Sync` verification is architecturally fishy.

### 2. Poisoning `StateMap` Mutex on `try_wait` Failure (`src/unix.rs:229`) 🟠 🤦

- **Severity**: 🟠 Medium
- **Threat Vector**: 🤦 Accidental Misuse
- **Bug Type**: Mutex Poisoning

In `process_sigchlds`, the crate reaps exiting processes using `(*k).try_wait().unwrap()`. If `try_wait` returns an `Err` (for instance, if another library in the process invoked `waitpid(-1, ...)` and reaped the child process, causing `ECHILD`), `.unwrap()` panics while holding the `map` Mutex lock. This permanently poisons the mutex (`PoisonError`), causing all future `wait_timeout` calls across all threads in the process to panic.

### 3. Use of Deprecated `static mut` (`src/unix.rs:33`) 🟡 🤦

- **Severity**: 🟡 Low
- **Threat Vector**: 🤦 Accidental Misuse
- **Bug Type**: Deprecated Static Mut Usage

`STATE` is declared as `static mut STATE: *mut State`. In modern Rust editions, creating shared or mutable references to `static mut` items is deprecated and widely recognized as an anti-pattern due to data race risks. Safe concurrency primitives (such as `std::sync::OnceLock` or `std::sync::atomic::AtomicPtr`) should be used instead.

## Missing Safety Comments
None of the `unsafe` blocks in the crate have `// SAFETY:` comments. Below are the exact file and line locations requiring safety comments along with rigorous proof obligations:

### 1. `src/windows.rs:26` 🔴

```rust
// SAFETY: `child.as_raw_handle()` returns a valid process handle owned by `child: &mut Child`.
// `WaitForSingleObject` blocks waiting for the process object to signal or for `ms` milliseconds to elapse.
// The handle remains open and valid for the duration of the FFI call, and Win32 synchronization APIs do not violate Rust aliasing rules.
unsafe {
match WaitForSingleObject(child.as_raw_handle() as *mut _, ms) {
```

### 2. `src/unix.rs:47` 🔴

```rust
// SAFETY: `INIT.call_once(State::init)` guarantees that `State::init()` has completed exactly once prior to this block.
// `State::init()` allocates a valid `State` on the heap and stores its raw pointer in `STATE`, valid for `'static`.
// Dereferencing `STATE` is thread-safe because `State` synchronizes concurrent accesses to internal state via `Mutex`.
unsafe {
(*STATE).wait_timeout(child, dur)
}
```

### 3. `src/unix.rs:66` 🔴

```rust
// SAFETY: `mem::zeroed()` produces a valid all-zero bit pattern for C `libc::sigaction` structs.
// `libc::sigaction` registers `sigchld_handler` with the OS for `SIGCHLD`.
// (Note: To be completely sound, `STATE` must be assigned before `sigaction` is called to avoid race conditions with incoming signals).
// `mem::transmute(state)` converts `Box` into a non-null `*mut State` pointer valid for the program lifetime.
unsafe {
```

### 4. `src/unix.rs:173` 🔴

```rust
// SAFETY: `fds` is a local 2-element array `[libc::pollfd; 2]` containing valid open file descriptors (`self.read` and `read`).
// `fds.as_mut_ptr()` points to these 2 elements, matching the `nfds` argument of 2.
// `timeout` is bounded to fit within `c_int::max_value()`. `libc::poll` safely inspects these descriptors without memory corruption.
let r = unsafe {
libc::poll(fds.as_mut_ptr(), 2, timeout)
};
```

### 5. `src/unix.rs:229` 🔴

```rust
// SAFETY: `k` is a `*mut Child` pointer inserted into `map` by a waiting thread currently inside `wait_timeout`.
// The caller holds `map.lock()`, ensuring mutual exclusion over `StateMap`.
// (Note: Under strict Rust aliasing models like Stacked Borrows, dereferencing `*k` conflicts with the active `&mut Child` borrow inside `Remove` on the waiting thread's stack).
*status = unsafe { (*k).try_wait().unwrap() };
```

### 6. `src/unix.rs:284` 🔴

```rust
// SAFETY: `STATE` is initialized during `State::init()`.
// `notify` performs a non-blocking write to the self-pipe descriptor (POSIX async-signal-safe).
// Chaining via `mem::transmute` converts the saved C function pointer `sa_sigaction` to a Rust function pointer and invokes the previous handler.
// (Note: Requires verifying `fnptr != SIG_IGN` and `STATE != null` to be sound).
unsafe {
let state = &*STATE;
```

贡献指南

这个仓库没有索引到贡献指南

评估

这个 Issue 还没有评估数据。

把新 issue 发到你的邮箱

精选适合新手参与的 GitHub issue 摘要。