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: Prefer ntdll named pipe APIs over kernel32 #19037

Open
The-King-of-Toasters opened this issue Feb 22, 2024 · 1 comment
Open

Windows: Prefer ntdll named pipe APIs over kernel32 #19037

The-King-of-Toasters opened this issue Feb 22, 2024 · 1 comment
Labels
enhancement Solving this issue will likely involve adding new logic or components to the codebase. os-windows
Milestone

Comments

@The-King-of-Toasters
Copy link
Contributor

The-King-of-Toasters commented Feb 22, 2024

CreatePipe and CreateNamedPipe are used by child_process to... create anonymous and named pipes for redirecting i/o. From my research, these are abstractions around NtCreateNamedPipeFile and NtOpenFile for the writing end. As per #1840, we should prefer using these APIs over kernel32 ones.

CreatePipe

Here is wine's implementation. Using NtTrace we see the same calls being made, just out of order:

Example Program
var in: HANDLE = undefined;
var out: HANDLE = undefined;

const res = CreatePipe(&in, &out, null, 1024);
if (res == 0)
    std.debug.panic("huh, {}", .{kernel32.GetLastError()});

defer {
    std.os.close(in);
    std.os.close(out);
}
NtOpenFile( FileHandle=0x7ff8c4c2b114 [0x00841f0fc32ecdcc], DesiredAccess=READ_CONTROL|WRITE_OWNER|SYNCHRONIZE|GENERIC_READ|GENERIC_WRITE|0x800258, ObjectAttributes="\Device\NamedPipe\", IoStatusBlock=0x153fcb0 [0/1], ShareAccess=3, OpenOptions=0x20 ) => 0
NtCreateNamedPipeFile( NamedPipeHandle=0x7ff8c4c2c1d4 [0x00841f0fc32ecdcc], DesiredAccess=READ_CONTROL|WRITE_OWNER|SYNCHRONIZE|GENERIC_READ|GENERIC_WRITE|0x800434, ObjectAttributes=0xf4:"", IoStatusBlock=0x153fcb0 [0/2], ShareAccess=3, CreateDisposition=2, CreateOptions=0x20, MessageType=false, MessageRead=false, NonBlocking=false, MaxInstances=1, InBufferSize=0x400, OutBufferSize=0x400, Timeout=0x153fca0 [-1.2e+09] ) => 0
NtOpenFile( FileHandle=0x7ff8c4c2b114 [0x00841f0fc32ecdcc], DesiredAccess=READ_CONTROL|WRITE_OWNER|SYNCHRONIZE|GENERIC_READ|GENERIC_WRITE|0x800258, ObjectAttributes=0xf8:"", IoStatusBlock=0x153fcb0 [0/1], ShareAccess=3, OpenOptions=0x60 ) => 0
NtClose( Handle=0x00007ff8c4c2ac94 ) => 0
NtClose( Handle=0x00007ff8c4c2ac94 ) => 0

Some improvements we could make:

  • Use an atomic int for the pipe number; Removes the need for looping.
  • Replace FALSE/0 with proper constants (e.g. FILE_PIPE_BYTE_STREAM_MODE).

Not that I cannot find these pipes under the pipe device directory (ls \\.\pipe\ in powershell), unlike proper named pipes.

CreateNamedPipe

Again, here is Wine's impl and a trace log.

Sample
const file = std.unicode.utf8ToUtf16LeStringLiteral("\\\\.\\pipe\\my_pipe");
const read_handle = windows.kernel32.CreateNamedPipeW(
    file.ptr,
    windows.PIPE_ACCESS_INBOUND | windows.FILE_FLAG_OVERLAPPED,
    windows.PIPE_TYPE_BYTE,
    1,
    4096,
    4096,
    0,
    null,
);
defer std.os.close(read_handle);
NtCreateNamedPipeFile( NamedPipeHandle=0x7ff8c4c2c1d4 [0x00841f0fc32ecdcc], DesiredAccess=READ_CONTROL|WRITE_OWNER|SYNCHRONIZE|GENERIC_READ|GENERIC_WRITE|0x800434, ObjectAttributes="\??\pipe\my_pipe", IoStatusBlock=0x14cfc78 [0/2], ShareAccess=2, CreateDisposition=3, CreateOptions=0, MessageType=false, MessageRead=false, NonBlocking=false, MaxInstances=1, InBufferSize=0x1000, OutBufferSize=0x1000, Timeout=0x14cfc20 [-500000] ) => 0
NtClose( Handle=0x00007ff8c4c2ac94 ) => 0

Pretty close to the real thing.

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

The-King-of-Toasters commented Mar 12, 2024

I have a prototype of a CreatePipe impl that uses the Native API, works great when single-threaded. Unfortunately everything fails hard when using it via the zig build runner. I'll put it up on a draft PR and hopefully I can crack this case; my windbg-foo is failing me.

Edit: I'm silly and passed the wrong flags.

@andrewrk andrewrk added enhancement Solving this issue will likely involve adding new logic or components to the codebase. os-windows labels Mar 16, 2024
@andrewrk andrewrk added this to the 1.0.0 milestone Mar 16, 2024
The-King-of-Toasters added a commit to The-King-of-Toasters/zig that referenced this issue Apr 13, 2024
To facilitate ziglang#19037, this commit slims `std.windows.kernel32` to only
have the functions needed by the standard library. Since this will break
projects that relied on these, I offer two solutions:

- Make an argument as to why certain functions should be added back in.
  Note that they may just be wrappers around `ntdll` APIs, which would
  go against ziglang#19037.
  If necessary I'll add them back in *and* make wrappers in
  `std.windows` for it.
- Maintain your own list of APIs. This is the option taken by bun[1],
  where they wrap functions with tracing.
- Use `zigwin32`.

I've also added TODO comments that specify which functions can be
reimplemented using `ntdll` APIs in the future.

Other changes:
- Group functions into groups (I/O, process management etc.).
- Synchronize definitions against Microsoft documentation to use the
  proper parameter types/names.
- Break all functions with parameters over multiple lines.
The-King-of-Toasters added a commit to The-King-of-Toasters/zig that referenced this issue Apr 13, 2024
To facilitate ziglang#19037, this commit slims `std.windows.kernel32` to only
have the functions needed by the standard library. Since this will break
projects that relied on these, I offer two solutions:

- Make an argument as to why certain functions should be added back in.
  Note that they may just be wrappers around `ntdll` APIs, which would
  go against ziglang#19037.
  If necessary I'll add them back in *and* make wrappers in
  `std.windows` for it.
- Maintain your own list of APIs. This is the option taken by bun[1],
  where they wrap functions with tracing.
- Use `zigwin32`.

I've also added TODO comments that specify which functions can be
reimplemented using `ntdll` APIs in the future.

Other changes:
- Group functions into groups (I/O, process management etc.).
- Synchronize definitions against Microsoft documentation to use the
  proper parameter types/names.
- Break all functions with parameters over multiple lines.
The-King-of-Toasters added a commit to The-King-of-Toasters/zig that referenced this issue Apr 19, 2024
To facilitate ziglang#19037, this commit slims `std.windows.kernel32` to only
have the functions needed by the standard library. Since this will break
projects that relied on these, I offer two solutions:

- Make an argument as to why certain functions should be added back in.
  Note that they may just be wrappers around `ntdll` APIs, which would
  go against ziglang#19037.
  If necessary I'll add them back in *and* make wrappers in
  `std.windows` for it.
- Maintain your own list of APIs. This is the option taken by bun[1],
  where they wrap functions with tracing.
- Use `zigwin32`.

I've also added TODO comments that specify which functions can be
reimplemented using `ntdll` APIs in the future.

Other changes:
- Group functions into groups (I/O, process management etc.).
- Synchronize definitions against Microsoft documentation to use the
  proper parameter types/names.
- Break all functions with parameters over multiple lines.
The-King-of-Toasters added a commit to The-King-of-Toasters/zig that referenced this issue Apr 20, 2024
To facilitate ziglang#19037, this commit slims `std.windows.kernel32` to only
have the functions needed by the standard library. Since this will break
projects that relied on these, I offer two solutions:

- Make an argument as to why certain functions should be added back in.
  Note that they may just be wrappers around `ntdll` APIs, which would
  go against ziglang#19037.
  If necessary I'll add them back in *and* make wrappers in
  `std.windows` for it.
- Maintain your own list of APIs. This is the option taken by bun[1],
  where they wrap functions with tracing.
- Use `zigwin32`.

I've also added TODO comments that specify which functions can be
reimplemented using `ntdll` APIs in the future.

Other changes:
- Group functions into groups (I/O, process management etc.).
- Synchronize definitions against Microsoft documentation to use the
  proper parameter types/names.
- Break all functions with parameters over multiple lines.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Solving this issue will likely involve adding new logic or components to the codebase. os-windows
Projects
None yet
Development

No branches or pull requests

2 participants