Use a hardcoded constant instead of calling OpenProcessToken. #90144
Conversation
|
(rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
cc @ChrisDenton Is this a reasonable assumption to make? |
|
Also worth noting that Go already does this: https://github.com/golang/sys/blob/master/windows/security_windows.go#L668 |
|
@smmalis37 That's helpful context, thank you. |
|
Yeah, it seems fine as it's hardcoded in the official Win32 API. Though it appears it was introduced in Windows 8 so my only concern is if the value works on Windows 7, |
|
Unfortunately I don't know the answer to that. Will CI test Windows 7? Or does someone have a machine/vm to test on? |
|
Sadly it doesn't work on Windows 7. Test program: token.rsfn main() {
unsafe {
let mut buffer = vec![0_u16; 260];
let mut len: u32 = buffer.len() as _;
let result = GetUserProfileDirectoryW(-4, buffer.as_mut_ptr(), &mut len);
if result != 0 {
println!("{}", String::from_utf16_lossy(&buffer[..len as _]));
} else {
println!("{}", std::io::Error::last_os_error());
}
}
}
extern "system" {
fn GetUserProfileDirectoryW(hToken: isize, lpProfileDir: *mut u16, lpcchSize: *mut u32) -> i32;
}Windows 7 output: I guess this could be worked around by testing for that error and then lazily loading the necessary functions. Obviously the drawback would be that it makes things more complicated. Another option is to wait to see if the min target api version RFC is accepted. This (along with build-std) would allow conditional compilation based on the target OS version. Or wait until Windows 7 support is removed, but that is not likely to happen for a long time yet. |
|
Dang, oh well. |
|
Looks like go restricts this to Windows 8 already: https://github.com/golang/sys/blob/master/windows/syscall_windows_test.go#L119 |
|
It seems there is another option here, calling NtQueryInformationProcess with the ProcessAccessToken info class should return the same token, and allow us to still drop the advapi dependency. However this info class is undocumented, despite existing unchanged since pre-XP. I don't know if that's a dealbreaker or worthwhile or not? |
|
Hm, it's undocumented but if it's only used as a fallback then it could be worth it. On the other hand, |
|
@smmalis37 Would you send a PR adding a comment to the current usage, noting that when we drop support for Windows 7, we can switch to a hardcoded value here? I'd be happy to merge that PR. |
|
@joshtriplett Can do, gimme a few minutes. |
Add comment documenting why we can't use a simpler solution See rust-lang#90144 for context. r? `@joshtriplett`
Add comment documenting why we can't use a simpler solution See rust-lang#90144 for context. r? ``@joshtriplett``
Add comment documenting why we can't use a simpler solution See rust-lang#90144 for context. r? ```@joshtriplett```
Now that Win 7 support is dropped, we can resurrect rust-lang#90144. GetCurrentProcessToken is defined in processthreadsapi.h as: FORCEINLINE HANDLE GetCurrentProcessToken ( VOID ) { return (HANDLE)(LONG_PTR) -4; } Since it's very unlikely that this constant will ever change, let's just use it instead of making calls to get the same information.
Now that Win 7 support is dropped, we can resurrect rust-lang#90144. GetCurrentProcessToken is defined in processthreadsapi.h as: FORCEINLINE HANDLE GetCurrentProcessToken ( VOID ) { return (HANDLE)(LONG_PTR) -4; } Since it's very unlikely that this constant will ever change, let's just use it instead of making calls to get the same information.
Now that Win 7 support is dropped, we can resurrect rust-lang#90144. GetCurrentProcessToken is defined in processthreadsapi.h as: FORCEINLINE HANDLE GetCurrentProcessToken ( VOID ) { return (HANDLE)(LONG_PTR) -4; } Since it's very unlikely that this constant will ever change, let's just use it instead of making calls to get the same information.
Use a hardcoded constant instead of calling OpenProcessToken. Now that Win 7 support is dropped, we can resurrect rust-lang#90144. GetCurrentProcessToken is defined in processthreadsapi.h as: FORCEINLINE HANDLE GetCurrentProcessToken ( VOID ) { return (HANDLE)(LONG_PTR) -4; } Since it's very unlikely that this constant will ever change, let's just use it instead of making calls to get the same information.
Use a hardcoded constant instead of calling OpenProcessToken. Now that Win 7 support is dropped, we can resurrect rust-lang#90144. GetCurrentProcessToken is defined in processthreadsapi.h as: FORCEINLINE HANDLE GetCurrentProcessToken ( VOID ) { return (HANDLE)(LONG_PTR) -4; } Since it's very unlikely that this constant will ever change, let's just use it instead of making calls to get the same information.
Rollup merge of rust-lang#119032 - smmalis37:patch-1, r=ChrisDenton Use a hardcoded constant instead of calling OpenProcessToken. Now that Win 7 support is dropped, we can resurrect rust-lang#90144. GetCurrentProcessToken is defined in processthreadsapi.h as: FORCEINLINE HANDLE GetCurrentProcessToken ( VOID ) { return (HANDLE)(LONG_PTR) -4; } Since it's very unlikely that this constant will ever change, let's just use it instead of making calls to get the same information.
GetCurrentProcessToken is defined in processthreadsapi.h as:
Since it's very unlikely that this constant will ever change, let's just use it instead of making api calls to get the same information.
This also (I think) removes the need for rust binaries to directly link against advapi32. Let me know if some changes are needed in those build-related bits, that I'm much less sure on.