Use of non-async-signal-safe code in pre_exec

[Manishearth]

Note

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

In ResetSigmask::pre_spawn (and similarly in ProcessSession::pre_spawn), the crate invokes CommandExt::pre_exec to configure the child process between fork() and exec().

process-wrap/src/std/reset_sigmask.rs

Lines 18 to 32 in b83fec4

	unsafe {
	command.pre_exec(|| {
	let mut oldset = SigSet::empty();
	let newset = SigSet::all();
	#[cfg(feature = "tracing")]
	trace!(unblocking=?newset, "resetting process sigmask");
	sigprocmask(SigmaskHow::SIG_UNBLOCK, Some(&newset), Some(&mut oldset))?;
	#[cfg(feature = "tracing")]
	trace!(?oldset, "sigmask reset");
	Ok(())
	});
	}

pre_exec requires the closure to take care of signal safety.

However, this code calls tracing::trace!(), which does perform heap allocations and work with synchronization code.

This is likely to cause deadlocks and maybe even UB.

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: process_wrap (v8)

Overall Safety Assessment

process_wrap (v8) is a library providing composable wrappers over process execution (std::process::Command and tokio::process::Child), designed as a flexible and extensible successor to command-group for managing process groups, sessions, Windows job objects, creation flags, signal masking, and kill-on-drop lifecycles.

The crate exhibits moderate unsafe code density across its core asynchronous I/O hooks (src/std/core.rs), Unix process lifecycle hooks (process_group.rs, process_session.rs, reset_sigmask.rs), and Windows Job Object management (job_object.rs, windows.rs). While its dual std/Tokio architecture is cleanly decoupled via macro generation (generic_wrap::Wrap!), the codebase reveals a severe systemic auditing gap: 100% of its unsafe blocks, unsafe trait implementations, and FFI calls lack // SAFETY: comments or # Safety proof documentation.

More critically, the audit identified a Critical soundness vulnerability: when feature = "tracing" is enabled, ResetSigmask invokes tracing::trace! inside unsafe fn CommandExt::pre_exec. Because pre_exec closures execute in the child process immediately after fork(), POSIX mandates strict async-signal-safety. The locking, formatting, and allocation performed by tracing introduce severe multithreaded deadlock and memory corruption hazards. Furthermore, the audit uncovered a major broken downcasting logic defect in Wrap::get_wrap (src/generic_wrap.rs), causing deterministic panics whenever inter-wrapper configuration is queried (such as composing CreationFlags with JobObject on Windows).

Critical Findings

1. Soundness violation and multithreaded deadlock vulnerability in ResetSigmask::pre_spawn under feature = "tracing" (POSIX async-signal-safety contract violation) 🔴 ⚠️

• Priority: 🔴 High

• Threat Vector: ⚠️ Accidental Misuse

• Bug Type: Async-Signal-Safety Violation

• Location: src/std/reset_sigmask.rs:18-32 and src/tokio/reset_sigmask.rs:19-33.

• Description: std::os::unix::process::CommandExt::pre_exec is marked unsafe fn because the provided closure executes in the context of the child process immediately after fork() and prior to exec(). In multithreaded Unix processes, POSIX.1-2008 (signal-safety(7)) dictates that calling any non-async-signal-safe function between fork() and exec() results in Undefined Behavior. Specifically, if any thread in the parent process held internal mutexes or allocator locks at the moment of fork(), those locks remain permanently locked in the child process. When feature = "tracing" is enabled, ResetSigmask::pre_spawn executes tracing::trace!(...) inside the pre_exec closure. The trace! macro performs string formatting, heap allocations, and acquires internal synchronization primitives within the active tracing subscriber. None of these operations are async-signal-safe. If an asynchronous runtime (such as multithreaded Tokio) or peer thread is active during spawn(), the child process can deadlock indefinitely attempting to acquire locks held during fork.

Fishy Findings

1. Broken downcasting logic in Wrap::get_wrap leading to deterministic panics when composing wrappers on Windows 🟠 ⚠️

• Priority: 🟠 Medium

• Threat Vector: ⚠️ Accidental Misuse

• Bug Type: Downcast Logic Error

• Location: src/generic_wrap.rs:142-150 (invoked at src/std/job_object.rs:43, 59 and src/tokio/job_object.rs:44, 65).

• Description: The core Wrap struct stores command wrappers in self.wrappers: IndexMap<TypeId, Box<dyn $wrapper>> (where $wrapper is StdCommandWrapper or TokioCommandWrapper). The helper method get_wrap<W: $wrapper + 'static>() retrieves Option<&Box<dyn $wrapper>> and casts it to &dyn std::any::Any. Because the trait $wrapper does not inherit from Any, the Rust compiler casts the outer Box<dyn $wrapper> pointer type itself to &dyn Any. When .downcast_ref::<W>() is called (where W is a concrete struct like CreationFlags), downcasting fails because the concrete type behind dyn Any is Box<dyn StdCommandWrapper>, whose TypeId does not match TypeId::of::<CreationFlags>(). The subsequent .expect(...) call then panics deterministically. Consequently, composing CreationFlags with JobObject on Windows always panics at spawn time.

2. Global system thread enumeration hack in resume_threads assumes child thread ownership 🟠 ⚠️

• Priority: 🟠 Medium

• Threat Vector: ⚠️ Accidental Misuse

• Bug Type: TOCTOU Race Condition

• Location: src/windows.rs:126-162.

• Description: To resume processes spawned with CREATE_SUSPENDED, resume_threads takes an OS-wide thread snapshot (CreateToolhelp32Snapshot(TH32CS_SNAPTHREAD, 0)) and iterates through every active thread on the entire Windows system, calling ResumeThread on any thread whose th32OwnerProcessID matches the child PID. Taking a global system thread snapshot on every process spawn imposes unnecessary kernel overhead and is vulnerable to TOCTOU race conditions if PIDs are rapidly recycled. Furthermore, if GetProcessId(child_process) fails and returns 0 (e.g., due to an invalid handle), inner will enumerate and attempt to open threads belonging to PID 0 (the Windows System Idle Process), failing with ERROR_ACCESS_DENIED.

3. Intentional handle leak in JobObjectChild::into_inner to prevent premature job termination 🟡 ⚠️

• Priority: 🟡 Low

• Threat Vector: ⚠️ Accidental Misuse

• Bug Type: Handle Leak

• Location: src/std/job_object.rs:105-113 and src/tokio/job_object.rs:121-129.

• Description: When consuming a JobObjectChild wrapper via into_inner(), the implementation closes the completion port handle but deliberately leaks self.job_port.job. While this prevents Windows from immediately killing the unwrapped child process (due to JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE), long-running applications that repeatedly wrap and unwrap processes via into_inner() will continuously leak Windows kernel Job Object handle table entries until process termination.

4. waitpid loop in ProcessGroupChild::wait_imp converts negative PGID 🟠 ⚠️

• Priority: 🟠 Medium

• Threat Vector: ⚠️ Accidental Misuse

• Bug Type: Unintended Child Reaping

• Location: src/std/process_group.rs:119 and src/tokio/process_group.rs:131.

• Description: wait_imp reaps child processes in a process group by passing -pgid.as_raw() to libc::waitpid. If pgid.as_raw() is 1 (init process group) or 0 (current process group), passing -1 or 0 to waitpid instructs the kernel to wait for any child process belonging to the parent application, potentially consuming exit statuses of unrelated background tasks spawned elsewhere in the host program.

Missing Safety Comments

1. src/std/core.rs:257-258 🟠

Missing // SAFETY: comments before BorrowedFd::borrow_raw. (Line 256 contains a brief comment regarding drop order, but lacks rigorous proof of descriptor validity and non-capture).

Proposed proof comment:

    // SAFETY:
    // - `out_fd` and `err_fd` are raw file descriptors obtained from `out_r` (`ChildStdout`) and `err_r` (`ChildStderr`).
    // - `out_r` and `err_r` are passed by value into `read2` and remain valid in local scope for the entire duration of the function body.
    // - When `read2` returns or unwinds, local variables drop in reverse order of declaration: `fds`, `err_bfd`, `out_bfd` are dropped first, and function arguments `err_r` and `out_r` drop afterwards.
    // - Therefore, the underlying file descriptors remain open, valid, and uncaptured for the entire lifetime of `out_bfd` and `err_bfd`.

2. src/std/core.rs:299 🔴

Missing // SAFETY: comment before libc::ioctl.

Proposed proof comment:

        // SAFETY:
        // - `fd.as_raw_fd()` returns a valid, open file descriptor guaranteed by the `BorrowedFd` invariant.
        // - `libc::FIONBIO` is a valid ioctl request on Unix file descriptors for toggling non-blocking I/O mode.
        // - `&v` is a valid aligned pointer to a stack-allocated `libc::c_int` (`bool as c_int`), which the kernel reads to update the descriptor flags.

3. src/std/process_group.rs:118 & src/tokio/process_group.rs:130 🔴

Missing // SAFETY: comments before libc::waitpid.

Proposed proof comment:

            // SAFETY:
            // - `-pgid.as_raw()` is a valid integer representing the target process group ID to wait on.
            // - `&mut status as *mut libc::c_int` points to a local stack-allocated `i32` variable (`status`), which is properly aligned and valid for writes of `sizeof(int)` bytes by the kernel.
            // - `flag.bits()` is a valid bitmask constructed from `WaitPidFlag` flags.

4. src/std/process_session.rs:31 & src/tokio/process_session.rs:28 🔴

Missing // SAFETY: comments before command.pre_exec(...).

Proposed proof comment:

        // SAFETY:
        // - `CommandExt::pre_exec` executes the closure in the child process immediately after `fork()`, requiring all operations to be async-signal-safe.
        // - `nix::unistd::setsid()` invokes the POSIX `setsid()` system call, which is explicitly guaranteed to be async-signal-safe by POSIX.1-2008 (`signal-safety(7)`).
        // - `map_err(Error::from)` converts raw errno codes (`Errno`) into `std::io::Error` integer representations without performing heap allocations or acquiring locks.

5. src/std/reset_sigmask.rs:18 & src/tokio/reset_sigmask.rs:19 🔴

Missing // SAFETY: comments before command.pre_exec(...).

Proposed proof comment (assuming non-async-signal-safe tracing::trace! calls are removed as required by Critical Findings):

        // SAFETY:
        // - `CommandExt::pre_exec` requires the closure to perform only async-signal-safe operations after `fork()`.
        // - `SigSet::empty()` and `SigSet::all()` perform local stack bitmask initializations.
        // - `nix::sys::signal::sigprocmask` invokes the underlying POSIX `pthread_sigmask` / `sigprocmask` system call, which is async-signal-safe under POSIX.1-2008.

6. src/std/job_object.rs:108 & src/tokio/job_object.rs:124 🔴

Missing // SAFETY: comments before CloseHandle.

Proposed proof comment:

        // SAFETY:
        // - `its.completion_port.0` contains a valid Win32 `HANDLE` to an open I/O completion port.
        // - `self.job_port` is moved into `its: ManuallyDrop<JobPort>`, ensuring that `JobPort::drop` will not execute when `into_inner` returns.
        // - Therefore, closing `its.completion_port.0` is sound and will not result in a double close.

7. src/windows.rs:31, 32, 37, 38 🔴

Missing # Safety docstrings on unsafe impl Send and unsafe impl Sync for JobHandle and PortHandle.

Proposed proof comment:

// # Safety
// Win32 kernel object handles (`HANDLE`) for Job Objects and I/O Completion Ports returned by `CreateJobObjectW` and `CreateIoCompletionPort` are thread-safe kernel references managed by the OS executive. They can be safely transferred across thread boundaries (`Send`) and shared concurrently across threads (`Sync`).

8. src/windows.rs:51-52 🔴

Missing // SAFETY: comments before CloseHandle in JobPort::drop.

Proposed proof comment:

    // SAFETY:
    // - `self.job.0` and `self.completion_port.0` contain valid, open Win32 `HANDLE`s owned by this `JobPort`.
    // - `JobPort::drop` executes exactly once upon destruction, ensuring these handles are closed once and never double-closed.

9. src/windows.rs:62, 67, 76, 98, 111 🔴

Missing // SAFETY: comments across Job Object creation and assignment FFI calls in make_job_object.

Proposed proof comment:

    // SAFETY:
    // - `CreateJobObjectW(None, None)` safely requests a new anonymous Job Object with default security attributes.
    // - `CreateIoCompletionPort(INVALID_HANDLE_VALUE, None, 0, 1)` creates a new standalone I/O completion port allowing 1 concurrent thread.
    // - `SetInformationJobObject` calls pass valid open job handles and struct pointers matching exact Win32 memory layouts (`JOBOBJECT_ASSOCIATE_COMPLETION_PORT` and `JOBOBJECT_EXTENDED_LIMIT_INFORMATION`) with accurate `size_of_val` lengths.
    // - `AssignProcessToJobObject` operates on valid open job and process handles with required quota and termination access rights.

10. src/windows.rs:128 🔴

Missing # Safety docstring on unsafe fn inner.

Proposed proof comment:

    /// # Safety
    /// `tool_handle` must be a valid open Win32 `HANDLE` to a thread snapshot created via `CreateToolhelp32Snapshot(TH32CS_SNAPTHREAD, 0)`.

11. src/windows.rs:139, 143, 144, 145, 148, 151, 157-160 🔴

Missing // SAFETY: comments across Toolhelp32 thread enumeration FFI calls in resume_threads.

Proposed proof comment:

        // SAFETY:
        // - `GetProcessId(child_process)` queries the kernel PID of the provided process handle.
        // - `CreateToolhelp32Snapshot` safely requests an OS-wide snapshot of active threads.
        // - `Thread32First` and `Thread32Next` take a valid snapshot handle and write thread metadata into an initialized `THREADENTRY32` struct with `dwSize = 28`.
        // - `OpenThread(THREAD_SUSPEND_RESUME, false, th32ThreadID)` opens valid enumerated thread IDs.
        // - `ResumeThread(thread_handle)` and `CloseHandle(thread_handle)` operate on valid open thread handles without aliasing or double-closing.

12. src/windows.rs:167 & src/windows.rs:181 🔴

Missing // SAFETY: comments before TerminateJobObject and GetQueuedCompletionStatus.

Proposed proof comment:

    // SAFETY:
    // - `TerminateJobObject` operates on a valid open Job Object handle (`job.0`) with `JOB_OBJECT_TERMINATE` access right.
    // - `GetQueuedCompletionStatus` operates on a valid open completion port handle (`completion_port.0`), writing completion status packets into valid local stack pointers (`&mut code`, `&mut key`, `&mut lp_overlapped`).

[passcod]

Gah! UB from logging. Thanks for the report, will get on that

[Manishearth]

😢😢😢

yeah it's silly that that can happen but probably real