Skip to content

windows: close NamedPipe handle on drop - #2003

Open
MsfPablo wants to merge 1 commit into
tokio-rs:masterfrom
MsfPablo:named-pipe-close-handle-on-drop
Open

windows: close NamedPipe handle on drop#2003
MsfPablo wants to merge 1 commit into
tokio-rs:masterfrom
MsfPablo:named-pipe-close-handle-on-drop

Conversation

@MsfPablo

Copy link
Copy Markdown

Closes #1944.

The leak

Inner owns the pipe HANDLE, so the handle is closed only when the last Arc<Inner> reference goes away. Every in-flight overlapped operation holds one of those references — leaked deliberately via mem::forget(me.clone()) in schedule_read / maybe_schedule_write — and only gives it back once its completion is witnessed.

SelectorInner::drop tries to reap those completions with zero-timeout polls of the IOCP, but as @1c7718e7 points out in the issue, there is no way to guarantee a still-pending read or write completes in that window. When one doesn't, the reference is never returned, Inner never drops, and the HANDLE is leaked for the lifetime of the process.

The change

Wrap the handle as Mutex<Option<Handle>> and have NamedPipe::drop close it synchronously, independently of the Arc refcount. This is @1c7718e7's suggested fix from the issue (1c7718e7@8df3ebc); the regression test is theirs too (1c7718e7@94e6627). Credit to them — happy to close this if they'd rather open it themselves.

Operations racing with the close now observe None:

  • the syscall wrappers (connect_overlapped, disconnect, read_overlapped, write_overlapped) return a BrokenPipe error;
  • Inner::result returns Option<io::Result<usize>>, and connect_done / read_done / write_done discard the completion rather than emitting an event for a pipe nobody holds anymore.

One deliberate difference from the linked commit: lock ordering. Every existing path takes io before handleschedule_read and the *_done handlers hold the io guard across the syscall wrappers, and Source::register holds it across add_handleAsRawHandle. The draft's NamedPipe::drop took handle first, which inverts that order and can deadlock. This version takes io first to match.

Semantics change worth flagging

The old Drop comment says writes are deliberately not cancelled "to ensure that everything is flushed out". Closing the handle ends any in-flight write, so a write still pending at drop time is no longer guaranteed to flush. I've updated the comment accordingly.

In practice this only affects the case that currently leaks: a write that completes synchronously (anything fitting in the pipe's buffer, e.g. the 4 bytes in write_then_drop) is already in the kernel buffer before the close and is still delivered, followed by EOF. Previously, a genuinely-pending write kept the handle alive forever rather than completing in any observable way, so I don't think anything useful is lost — but this is the point @1c7718e7 wanted maintainer input on, and @Darksonn's "at the very least we must close the handle" is what I took as the go-ahead for this narrower fix. If you'd prefer the fuller flush()-based design discussed in the issue instead, say so and I'll drop this.

What I verified, and what I couldn't

I don't have a Windows machine — please treat the runtime behaviour as unverified.

Verified:

  • cargo check --target x86_64-pc-windows-msvc --all-features --all-targets — clean.
  • cargo clippy --target x86_64-pc-windows-msvc --all-features --all-targets — no new lints; the handful of remaining ones are pre-existing (impl<'a> Read for &'a NamedPipe etc.).
  • cargo fmt --check — clean.
  • cargo build --all-features and cargo test --all-features on macOS — pass, though these don't touch any of the changed code.
  • Lock-ordering audit of every io / handle acquisition path, as described above.

Not verified:

  • That handle_closed_on_drop actually fails before this change and passes after. I'm relying on @1c7718e7's report that it reproduces on Windows 11 and Wine 11.5.
  • The rest of tests/win_named_pipe.rs still passing. I also picked up the connect_twice restructuring from the linked fix commit — it scans all events for a readable Token(0) instead of only the first Token(0) event — on the assumption it's needed because closing the handle shifts event delivery timing. I can't confirm that, so it's worth a look; I'll drop it if it's unnecessary.

Windows CI on this PR is the real test.


Courtesy note: this patch was prepared with AI assistance (Claude Code). The design is @1c7718e7's; I reviewed the lock ordering, the completion-handler paths and the flush-semantics implications myself, and the verification claims above are accurate about what was and wasn't run. I didn't see a CONTRIBUTING.md or AI policy in the repo — let me know if you'd like this handled differently.

`Inner` owns the pipe `HANDLE` and closes it when the last `Arc<Inner>`
reference goes away. Every in-flight overlapped operation holds one of
those references (leaked via `mem::forget` in `schedule_read` /
`maybe_schedule_write`) and only returns it when its completion is
witnessed.

`SelectorInner::drop` tries to reap those completions with zero-timeout
polls of the IOCP, but there is no way to guarantee that a still-pending
read or write completes in that window. When one does not, the reference
is never returned, `Inner` is never dropped, and the underlying `HANDLE`
is leaked for the lifetime of the process.

Wrap the handle in a `Mutex<Option<Handle>>` and have `NamedPipe::drop`
close it synchronously, independently of the `Arc` refcount. Operations
that race with the close now observe `None`: syscall wrappers return a
`BrokenPipe` error, and `Inner::result` returns `None` so the completion
handlers discard the completion instead of emitting an event for a pipe
nobody holds anymore.

Note the lock ordering: `io` is always acquired before `handle`, matching
the existing call paths (`schedule_read` and the `*_done` handlers hold
the `io` guard across the syscall wrappers), so `NamedPipe::drop` takes
them in that same order.

This means a write that is still pending when the `NamedPipe` is dropped
is no longer guaranteed to be flushed, since closing the handle ends it.
Previously such a write kept the handle alive forever rather than
completing in any observable way.

Closes tokio-rs#1944
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

NamedPipe permanently leaks HANDLEs

1 participant