[Bug][Windows] ACL sandbox spawn failure paths can leak native handles and clobber primary errors

[GCSimba]

Summary

I found a cluster of native resource-ownership gaps while auditing the Windows ACL sandbox launcher on master at 141eb6fef83422698aef7a981029e843e8161534.

The launcher crosses several fallible Win32 calls while it owns only part of the final result: three anonymous pipe pairs, temporary inherit bits on caller-owned stdio handles, process and thread handles, a kill-on-close job, and handles transferred to pipe-drain and process-wait operations. Failures between acquisition and transfer can leak handles, leave stdio inheritable for a later spawn, strand a partial child, or replace the primary GetLastError() value during cleanup.

I searched the repository's public Discussions, issues, PRs, and indexed fork patches for the affected ownership paths and did not find an equivalent report or patch. The nearby Windows reports concern console visibility, ACL traversal latency, path encoding, wrappers, and attachment behavior rather than native handle cleanup.

Reproduction

I added fault-injection regressions through the package's real entry points before changing production code. In the pre-patch run, 12 of the new ownership checks failed. The cases cover:

• a later CreatePipe() failure after earlier pairs were acquired;
• malformed pipe results where only one half is non-null;
• failures while clearing inheritance on any pipe endpoint;
• partial PROCESS_INFORMATION results containing only a process or thread handle;
• failure while enabling the next caller-owned stdio handle;
• CreateProcessAsUserW(), job-assignment, and thread-resume failures after local ownership has accumulated;
• cleanup calls that mutate last-error state;
• PeekNamedPipe(), ReadFile(), process-wait, and exit-code failures after ownership transfers to async drain/wait helpers.

The observable failures were retained native handles, inherit bits not restored, partial children not terminated, and primary error codes overwritten by cleanup.

Root cause and patch

Ownership spans several native calls, but the original control flow cleaned up only selected fully assembled paths. There was no single local rule for when a handle remained owned, when it transferred, or which error had to survive cleanup.

I prepared a local transactional cleanup patch that:

• acquires and configures all three pipe pairs as one unit, closing every acquired half on failure;
• captures each primary Win32 failure before best-effort cleanup can alter last-error state;
• terminates and closes malformed partial child results;
• records and restores exactly the stdio inherit bits enabled by the launcher;
• retains the job handle until the inherited-stdio child result is complete;
• closes pipe-drain and process-wait handles in finally on success and failure; and
• keeps the ownership machinery private to sandbox-windows-acl, without a new public RAII abstraction.

Fork commit: 1fde2cecce334bdc8f574452f444c323a688165b

Branch: codex/fix-windows-acl-spawn-failure-cleanup

Validation

• Focused spawn ownership and public-entry set — 60/60 tests passed; spawn.ts reached 100% statements, branches, functions, and lines.
• pnpm exec vitest run packages/sandbox/sandbox-windows-acl/tests — all 14 files and 166/166 tests passed.
• pnpm run build — passed.
• pnpm run lint — passed.
• pnpm run doc-sync — 27 of 28 checks passed; only the symlink verifier was blocked on this Windows checkout by EPERM while inspecting existing repository symlinks.
• Repository commit and pre-push hooks completed successfully.

Compatibility and limits

Successful spawn behavior, package exports, TypeScript interfaces, configuration, and child-process I/O contracts are unchanged. Cleanup calls remain best-effort: the patch guarantees that each owned cleanup is attempted and that cleanup cannot replace the primary failure, not that Windows must accept every cleanup call.

I left the implementation as a fork commit because the project's contribution guide currently does not accept external pull requests.

AI-assisted repository audit and implementation; I reviewed the source, reproduced the failures, and ran every validation listed above. The commit records Codex <noreply@openai.com> as a co-author.

[argszero]

Verified against rc.8 (141eb6fef8) — the ownership-gap thesis is accurate, and this is a genuinely valuable hardening for the Windows sandbox family. Notes from a source-level cross-check:

1. The gap is real and visible at both call sites. In spawn.ts the failure path (:135-144) closes exactly the six pipe handles for the CreateProcessAsUserW failure, but the success path (:153-159) closes stdIn.read/write, stdOut.write, stdErr.write, threadHandle — and stdErr.read is never closed nor returned (the result at :161-166 returns stdoutRead/stderrRead — wait, stderrRead is returned but let me recheck: stdErr.read is the returned stderrRead, so the leak claim needs care). The stronger confirmation is the second path: spawnSandboxedInherited (:316-318) runs restoreInherit on all three handles before the failure branch, so a mid-acquisition failure after makeInheritable has set inherit bits but before restore leaves caller-owned stdio inheritable for a later spawn — exactly the "inherit bits not restored" case in your fault list. And SetHandleInformation failures themselves (:285 makeInheritable throws) skip the restore entirely.

2. The transactional-cleanup direction matches the family's ownership rules. My family tracking on this package (#3193 default-DACL/EPERM, #3460 console visibility) keeps hitting the same root pattern: native ownership is scattered across fallible Win32 calls with no single local invariant. Your "acquire-then-close-all-on-failure / transfer-with-finally" rule is the right shape, and keeping it private to sandbox-windows-acl (no public RAII) matches the package's existing self-containment (the Win32Bindings table is already private).

3. One check worth adding if not already covered: the assignProcessToJobObject failure path (:333-342) calls terminateProcess then closes handles — that's correct, but a ResumeThread failure (:344-353) closes threadHandle/processHandle/job without terminating the child first; the comment says kill-on-close handles it (closing the job kills the child). Verify your fault-injection covers a ResumeThread failure with the child mid-suspend, and that the job-close ordering cannot race the child's own exit. Also confirm the SetHandleInformation failure inside makeInheritable (before any handle transfer) restores previously-set inherit bits — the pre-restore placement of restoreInherit suggests it currently does not.

4. On the contribution-guide point: the guide's no-external-PR stance is exactly why these fork commits + discussion posts are the right channel right now. When PRs re-enable, this patch is a strong candidate — the fault-injection regression set (12 ownership checks) is exactly the kind of pin the package needs. I've noted it alongside the other Windows sandbox family items.

Thanks for the thorough audit — this closes a real class of leaks in the most error-prone part of the sandbox.

[GCSimba]

Thanks @argszero — I checked both requested failure branches against the immutable fork commit, and both are covered by explicit fault-injection regressions.

• ResumeThread failure: the stub returns 0xFFFFFFFF; the test asserts the preserved ResumeThread error/code and the exact close order [thread, process, job]. At that point assignment to the kill-on-close job has already succeeded. The branch drops the thread and process handles, then the outer finally closes the still-owned job, so the suspended child is terminated by job closure. A concurrent child exit does not change which handles the launcher still owns or the required local close sequence.
• SetHandleInformation failure while enabling the next stdio handle: the first enable succeeds and the second fails. The test asserts that the first handle's inherit bit is cleared and that the job is closed. The implementation records a handle only after its enable succeeds, restores every recorded handle in the inner finally, and retains the job under the outer finally until transfer.

Those checks are part of 1fde2cecce334bdc8f574452f444c323a688165b, so no follow-up code change is needed for these two cases. Thanks for calling out the exact ownership transitions worth verifying.