Skip to content
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.

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

Add helper functions for pipes #17566

Merged
merged 2 commits into from
Jul 17, 2024
Merged

Add helper functions for pipes #17566

merged 2 commits into from
Jul 17, 2024

Conversation

lhecker
Copy link
Member

@lhecker lhecker commented Jul 15, 2024

Split off from #17510:

  • HandleWantsOverlappedIo can be used to check if a handle requires
    overlapped IO. This is important, as ReadFile and WriteFile are
    documented to not work correctly if an overlapped handle is used
    without overlapped IO and vice versa.
    In my tests with pipes, this appears to be true.
  • CreatePipe creates a synchronous, unidirectional pipe.
  • CreateOverlappedPipe does what it says on the tin, while allowing
    you to specify the direction of the pipe (in, out, duplex).
  • GetOverlappedResultSameThread is largely the same as
    GetOverlappedResult, but adds back a neat optimization from
    the time before Windows 7. I thought it was neat.

@lhecker lhecker added the Product-Conpty For console issues specifically related to conpty label Jul 15, 2024
src/types/utils.cpp Outdated Show resolved Hide resolved
__assume(overlapped->hEvent != nullptr);
__assume(bytesTransferred != nullptr);

if (overlapped->Internal == STATUS_PENDING)
Copy link
Member

Choose a reason for hiding this comment

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

Internal - is this documented anywhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep: https://learn.microsoft.com/en-us/windows/win32/api/minwinbase/ns-minwinbase-overlapped
It does say "its behavior may change" but I think that's a minor concern for us, given that we're effectively a system component. It also hasn't changed since "Windows NT 1.0" (I honestly don't know but basically since forever). The reason it's unlikely to change is because the two Internal fields overlap with NT's IO_STATUS_BLOCK struct, whose 2 fields are publicly documented and don't mention this "its behavior may change".

// API doesn't have a parameter where you could pass FILE_FLAG_OVERLAPPED!
// So, we'll simply use the underlying NT APIs instead.
//
// Most code on the internet suggests creating named pipes with a random name,
Copy link
Member

Choose a reason for hiding this comment

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

can you point to official docs explaining why we can do this safely and above-board?

Copy link
Member Author

@lhecker lhecker Jul 15, 2024

Choose a reason for hiding this comment

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

You mean why we can safely create a pipe without a name, now and in the future? Outside of the function description, there are no official docs about how NPFS works, both publicly and internally. It basically works "as designed". I believe it's safe to rely on it for 2 reasons:

  • NPFS last changed in Windows Vista
  • There are no filter driver events for the creation of anonymous pipes. The only way to know about them is by subscribing to FO_NAMED_PIPE and testing for the name length. Anonymous pipes have an empty name. This effectively makes it a public contract.

Copy link
Member

Choose a reason for hiding this comment

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

fine with me!

@lhecker lhecker added this pull request to the merge queue Jul 16, 2024
@DHowett DHowett removed this pull request from the merge queue due to a manual request Jul 17, 2024
@lhecker lhecker merged commit 1d2ffe9 into main Jul 17, 2024
20 checks passed
@lhecker lhecker deleted the dev/lhecker/17510-pipe-utils branch July 17, 2024 01:02
PKRoma pushed a commit to PKRoma/Terminal that referenced this pull request Nov 4, 2024
Split off from microsoft#17510:
* `HandleWantsOverlappedIo` can be used to check if a handle requires
  overlapped IO. This is important, as `ReadFile` and `WriteFile` are
  documented to not work correctly if an overlapped handle is used
  without overlapped IO and vice versa.
  In my tests with pipes, this appears to be true.
* `CreatePipe` creates a synchronous, unidirectional pipe.
* `CreateOverlappedPipe` does what it says on the tin, while allowing
  you to specify the direction of the pipe (in, out, duplex).
* `GetOverlappedResultSameThread` is largely the same as
  `GetOverlappedResult`, but adds back a neat optimization from
  the time before Windows 7. I thought it was neat.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Product-Conpty For console issues specifically related to conpty
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants