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

Progress: implement IPC for windows #20114

Closed
wants to merge 1 commit into from
Closed

Conversation

jacobly0
Copy link
Member

@jacobly0 jacobly0 commented May 29, 2024

The nearby local maximum API surface that would have been required to implement this feature would have introduced a bunch of useless new kernel32 dependencies, so I instead fled two undocumented layers deep into ntdll for the identical functionality in a single syscall.

Closes #20105

@jacobly0 jacobly0 force-pushed the win-prog branch 5 times, most recently from 4b96626 to c040764 Compare May 30, 2024 04:08
Copy link
Contributor

@The-King-of-Toasters The-King-of-Toasters left a comment

Choose a reason for hiding this comment

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

This PR is great, but it goes much farther than just implementing progress IPC for Windows: You've basically rewritten all of the OS API bindings in the process. To be clear: that's a good thing! But this PR needs to be split up into smaller commits to facilitate better reviews.

Furthermore, I'd like to see some tests that ensure the new ACCESS_MASK builder gives the same results as a bitflag before I feel comfortable using this.

Finally, see my comment on NtCreateUserProcess.

@@ -464,3 +466,7 @@ pub extern "kernel32" fn RegOpenKeyExW(
) callconv(WINAPI) LSTATUS;

pub extern "kernel32" fn GetPhysicallyInstalledSystemMemory(TotalMemoryInKilobytes: *ULONGLONG) BOOL;

pub extern "kernel32" fn IsDebuggerPresent() BOOL;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't believe these are used anywhere.

}
}
}
switch (windows.ntdll.NtCreateUserProcess(
Copy link
Contributor

Choose a reason for hiding this comment

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

Use NtCreateUserProcess at your own peril. Relevant quote from @zodiacon's "Windows Native API Programming":

There have been several attempts in the Infosec community to utilize RtlCreateUserProcess and/or NtCreateUserProcess to allow running Windows subsystem applications, with varying degrees of success. Part of the problem is the need to communicate with the Windows Subsystem process (csrss.exe) to notify it of the new process and thread. This turns out to be fragile, as different Windows versions may have somewhat different expectations.

Essentially: you're one the hook if this breaks on newer Windows versions and/or Wine updates.

@andrewrk
Copy link
Member

2 months old, no updates. I think we have vague plans to try doing this with shared memory instead, anyway.

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.

implement std.Progress IPC for Windows
3 participants