Skip to content

Windows: Cache the pipe filesystem handle #155250

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ChrisDenton wants to merge 2 commits into
base: main
Choose a base branch
from
+23 −6
Open

Windows: Cache the pipe filesystem handle #155250

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5dd0500
Windows: Cache the pipe filesystem handle
ChrisDenton Apr 13, 2026
a6ec294
Explicitly note that we're leaking a handle
ChrisDenton Apr 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 23 additions & 6 deletions library/std/src/sys/process/windows/child_pipe.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
use crate::io::{self, BorrowedCursor, IoSlice, IoSliceMut};
use crate::ops::Neg;
use crate::os::windows::prelude::*;
use crate::sync::atomic::Atomic;
use crate::sync::atomic::Ordering::Relaxed;
use crate::sys::handle::Handle;
use crate::sys::{FromInner, IntoInner, api, c};
use crate::{mem, ptr};
Expand Down Expand Up @@ -70,10 +72,19 @@ pub(super) fn child_pipe(ours_readable: bool, their_handle_inheritable: bool) ->
let mut object_attributes = c::OBJECT_ATTRIBUTES::default();
object_attributes.Length = size_of::<c::OBJECT_ATTRIBUTES>() as u32;

// Open a handle to the pipe filesystem (`\??\PIPE\`).
// This will be used when creating a new annon pipe.
let pipe_fs = {
let path = api::unicode_str!(r"\??\PIPE\");
// Open a handle to the pipe filesystem (`\Device\NamedPipe\`).
// This will be used when creating a new anonymous pipe.
Copy link
Copy Markdown
Member

@Mark-Simulacrum Mark-Simulacrum Apr 18, 2026 •
edited by rustbot
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we going to get complaints from valgrind (not sure that works on Windows) for a handle leak due to this? Anything preemptive that we could do to indicate it's intentional?

View changes since the review

Copy link
Copy Markdown
Member Author

@ChrisDenton ChrisDenton Apr 21, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't tend to get those types of complaints for Windows but. sure, I can add a comment explicitly noting this is intentional.

Edit: Windows doesn't have the tight fd limit that unix systems can have so leaking a single handle is less of an issue (the limit for handles is something north of 16 million).

//
// We cache the handle once so we can reuse it without needing to reopen it each time.
// NOTE: this means the handle may appear to be leaked but that's fine because
// it's only one handle and the OS will clean it up when the process exits.
static PIPE_FS: Atomic<c::HANDLE> = Atomic::<c::HANDLE>::new(ptr::null_mut());
let pipe_fs = if let handle = PIPE_FS.load(Relaxed)
Copy link
Copy Markdown
Member

@Mark-Simulacrum Mark-Simulacrum Apr 18, 2026 •
edited by rustbot
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, this feels like it needs to be Acquire coupled with a Release store below, no? Otherwise we're not necessarily seeing the memory initialization. Though probably OK in practice given the interesting memory is in the kernel... right?

View changes since the review

Copy link
Copy Markdown
Contributor

@lhecker lhecker Apr 18, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah exactly: The NT file handle is kernel-mode only and so access is fully synchronized already.

Copy link
Copy Markdown
Member Author

@ChrisDenton ChrisDenton Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, all we need to do here is ensure the atomic containing the handle synchronises with itself. For which relaxed is sufficient. It would be a (rather serious, imo) bug in the OS if using a handle across threads risked synchronisation issues around the handle itself.

&& !handle.is_null()
{
handle
} else {
let path = api::unicode_str!(r"\Device\NamedPipe\");
object_attributes.ObjectName = path.as_ptr();
let mut pipe_fs = ptr::null_mut();
let status = c::NtOpenFile(
Expand All @@ -85,7 +96,13 @@ pub(super) fn child_pipe(ours_readable: bool, their_handle_inheritable: bool) ->
c::FILE_SYNCHRONOUS_IO_NONALERT, // synchronous access
);
if c::nt_success(status) {
Handle::from_raw_handle(pipe_fs)
match PIPE_FS.compare_exchange(ptr::null_mut(), pipe_fs, Relaxed, Relaxed) {
Ok(_) => pipe_fs,
Err(existing) => {
c::CloseHandle(pipe_fs);
existing
}
}
} else {
return Err(io::Error::from_raw_os_error(c::RtlNtStatusToDosError(status) as i32));
}
Expand All @@ -104,7 +121,7 @@ pub(super) fn child_pipe(ours_readable: bool, their_handle_inheritable: bool) ->
let ours = {
// Use the pipe filesystem as the root directory.
// With no name provided, an anonymous pipe will be created.
object_attributes.RootDirectory = pipe_fs.as_raw_handle();
object_attributes.RootDirectory = pipe_fs;

// A negative timeout value is a relative time (rather than an absolute time).
// The time is given in 100's of nanoseconds so this is 50 milliseconds.
Expand Down
Loading