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

Windows: Replace CreatePipe with ntdll implementation #19271

Merged
merged 3 commits into from
Mar 16, 2024

Conversation

The-King-of-Toasters
Copy link
Contributor

One half of #19037.

lib/std/os/windows.zig Outdated Show resolved Hide resolved
@e4m2
Copy link
Contributor

e4m2 commented Mar 13, 2024

Some less important additional questions:

  1. Unlike the Windows version, Zig's current signature does not allow passing a custom nSize, the defaulted size would remain 4096. Do we want to add that parameter?
  2. Unlike the Windows version, Zig's current signature does not allow passing null security attributes. Doing so just defaults the security descriptor to null (and obviously leaves the attributes alone). Do we want to change this?

Something slightly more interesting:

On a Windows 11 22631 system, CreatePipe() completely sidesteps having to come up with a unique name for the pipe.

Instead, it first checks a global variable to see whether it has already opened a handle to \Device\NamedPipe\ (with nothing after it, just the directory). If not, the handle is opened and stored atomically to that global variable (in other words, the handle is opened exactly once when CreatePipe() is first called).

Then, the read handle is created using NtCreateNamedPipeFile() with an empty (i.e. zero length, zero maximum length, null buffer) name and the previously opened global handle as its root directory.

Finally, the write handle is opened using NtOpenFile() with almost the same object attributes, except its root directory is actually set to the previously opened read handle.

IMO this approach is better than having to come up with a unique name each time a pipe is created. It should be investigated whether this works on older Windows versions too, and if it does, we should probably switch to this implementation.

@andrewrk
Copy link
Member

Nice. Thanks for the review assistance, @e4m2

@squeek502
Copy link
Collaborator

squeek502 commented Mar 14, 2024

IMO this approach is better than having to come up with a unique name each time a pipe is created. It should be investigated whether this works on older Windows versions too, and if it does, we should probably switch to this implementation.

One potential problem is that this special casing of zero-length object names doesn't seem to exist in Wine's NtCreateNamedPipeFile as far as I can tell, so closely emulating Windows might mean that Zig's CreatePipe would fail when run via Wine. This may be able to be considered a Wine bug, though.

(this was discussed a bit in Discord here: https://discord.com/channels/605571803288698900/1217407987149307914)

@The-King-of-Toasters
Copy link
Contributor Author

I'll do some research re: Wine compatibility soon. Would also be interesting to see which Windows version introduced this ability.

@e4m2
Copy link
Contributor

e4m2 commented Mar 14, 2024

Would also be interesting to see which Windows version introduced this ability.

Good news on this front. The new method is already being used by CreatePipe() in version 6.0.6000 (early Vista).

So I think we're mostly good to go on the Zig side, just need to know how Wine handles it. If they indeed don't support it, it's probably best to file an upstream bug, as squeek said.

@The-King-of-Toasters
Copy link
Contributor Author

Some more good news: It seems that at least Wine 8.2 supports these kinds of named pipes. There's even a test added in 9.4 that checks for this.

lib/std/os/windows.zig Outdated Show resolved Hide resolved
lib/std/os/windows.zig Outdated Show resolved Hide resolved
lib/std/os/windows.zig Outdated Show resolved Hide resolved
lib/std/os/windows.zig Outdated Show resolved Hide resolved
lib/std/os/windows.zig Outdated Show resolved Hide resolved
@The-King-of-Toasters The-King-of-Toasters force-pushed the falling-metal-pipe branch 2 times, most recently from 5fe9539 to e5d976f Compare March 16, 2024 04:12
@The-King-of-Toasters
Copy link
Contributor Author

I've cached a handle to \Device\NamedPipe globally using std.once. Due to the way it works though, I can't catch any error codes if it fails. Lemme know if this is suitable or not.

@andrewrk
Copy link
Member

andrewrk commented Mar 16, 2024

I think it's adequate how you implemented it, but does it really need to use a mutex? The strategy hinted at by @e4m2 seems more optimal:

  1. initialize the handle to null
  2. atomic load
  3. if it's null, do the dll call to open it
  4. cmpxchg
  5. if the value you get back is non-null, close the handle you got from step 3 and use the value from step 5 instead.

This implementation is now a direct replacement for the `kernel32` one.
New bitflags for named pipes and other generic ones were added based on
browsing the ReactOS sources.

`UNICODE_STRING.Buffer` has also been changed to be nullable, as
this is what makes the implementation work.
This required some changes to places accesssing the buffer after a
`SUCCESS`ful return, most notably `QueryObjectName` which even referred
to it being nullable.
This is how they've been implemented in `kernel32` since NT 3.1.
Fixes a TODO referencing the ancient issue ziglang#305.
@The-King-of-Toasters
Copy link
Contributor Author

@andrewrk Ok, I've modified the code to use atomics loads and lock cmpxchg. I'm not really experienced in this area, so feel free to comment or make some edits if need be.

@andrewrk
Copy link
Member

Looks perfect!

@andrewrk andrewrk merged commit 1b8d1b1 into ziglang:master Mar 16, 2024
10 checks passed
@The-King-of-Toasters The-King-of-Toasters deleted the falling-metal-pipe branch April 1, 2024 02:28
andrewrk pushed a commit that referenced this pull request Jul 24, 2024
PR [19271](#19271) added some static function implementations from kernel32, but some parts of the library still used the dynamically loaded versions.
SammyJames pushed a commit to SammyJames/zig that referenced this pull request Aug 7, 2024
PR [19271](ziglang#19271) added some static function implementations from kernel32, but some parts of the library still used the dynamically loaded versions.
igor84 pushed a commit to igor84/zig that referenced this pull request Aug 11, 2024
PR [19271](ziglang#19271) added some static function implementations from kernel32, but some parts of the library still used the dynamically loaded versions.
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.

4 participants