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.

Sign up for GitHub

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] Avoid GetModuleHandle(NULL) #2301

Merged
merged 4 commits into from
May 29, 2022
Merged

[Windows] Avoid GetModuleHandle(NULL) #2301

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5817994
Use get_instance_handle() over GetModuleHandle(NULL)
AronParker May 23, 2022
a9897a6
Merge branch 'master' of https://github.com/rust-windowing/winit
AronParker May 23, 2022
5bc9dcd
Fix formatting
AronParker May 23, 2022
e1d675c
Make ImageBase cast more concise
AronParker May 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 3 additions & 6 deletions src/platform_impl/windows/event_loop.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,7 @@ use windows_sys::Win32::{
RDW_INTERNALPAINT, SC_SCREENSAVE,
},
Media::{timeBeginPeriod, timeEndPeriod, timeGetDevCaps, TIMECAPS, TIMERR_NOERROR},
System::{
LibraryLoader::GetModuleHandleW, Ole::RevokeDragDrop, Threading::GetCurrentThreadId,
WindowsProgramming::INFINITE,
},
System::{Ole::RevokeDragDrop, Threading::GetCurrentThreadId, WindowsProgramming::INFINITE},
UI::{
Controls::{HOVER_DEFAULT, WM_MOUSELEAVE},
Input::{
Expand Down Expand Up @@ -647,7 +644,7 @@ fn create_event_target_window<T: 'static>() -> HWND {
lpfnWndProc: Some(thread_event_target_callback::<T>),
cbClsExtra: 0,
cbWndExtra: 0,
hInstance: GetModuleHandleW(ptr::null()),
hInstance: util::get_instance_handle(),
hIcon: 0,
hCursor: 0, // must be null in order for cursor state to work properly
hbrBackground: 0,
Expand Down Expand Up @@ -681,7 +678,7 @@ fn create_event_target_window<T: 'static>() -> HWND {
0,
0,
0,
GetModuleHandleW(ptr::null()),
util::get_instance_handle(),
ptr::null(),
);

Expand Down
5 changes: 2 additions & 3 deletions src/platform_impl/windows/icon.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
use std::{fmt, io, mem, path::Path, ptr, sync::Arc};
use std::{fmt, io, mem, path::Path, sync::Arc};

use windows_sys::{
core::PCWSTR,
Win32::{
Foundation::HWND,
System::LibraryLoader::GetModuleHandleW,
UI::WindowsAndMessaging::{
CreateIcon, DestroyIcon, LoadImageW, SendMessageW, HICON, ICON_BIG, ICON_SMALL,
IMAGE_ICON, LR_DEFAULTSIZE, LR_LOADFROMFILE, WM_SETICON,
Expand Down Expand Up @@ -111,7 +110,7 @@ impl WinIcon {
let (width, height) = size.map(Into::into).unwrap_or((0, 0));
let handle = unsafe {
LoadImageW(
GetModuleHandleW(ptr::null()),
util::get_instance_handle(),
resource_id as PCWSTR,
IMAGE_ICON,
width as i32,
Expand Down
22 changes: 20 additions & 2 deletions src/platform_impl/windows/util.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,12 @@ use std::{
use windows_sys::{
core::{HRESULT, PCWSTR},
Win32::{
Foundation::{BOOL, HWND, RECT},
Foundation::{BOOL, HINSTANCE, HWND, RECT},
Graphics::Gdi::{ClientToScreen, InvalidateRgn, HMONITOR},
System::LibraryLoader::{GetProcAddress, LoadLibraryA},
System::{
LibraryLoader::{GetProcAddress, LoadLibraryA},
SystemServices::IMAGE_DOS_HEADER,
},
UI::{
HiDpi::{DPI_AWARENESS_CONTEXT, MONITOR_DPI_TYPE, PROCESS_DPI_AWARENESS},
Input::KeyboardAndMouse::GetActiveWindow,
Expand Down Expand Up @@ -205,6 +208,21 @@ pub fn is_focused(window: HWND) -> bool {
window == unsafe { GetActiveWindow() }
}

pub fn get_instance_handle() -> HINSTANCE {
// Gets the instance handle by taking the address of the
// pseudo-variable created by the microsoft linker:
// https://devblogs.microsoft.com/oldnewthing/20041025-00/?p=37483
Copy link
Contributor

@filnet filnet May 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

As mentioned, this works when using a Microsoft linker.
What about MSYS2/Mingw and assimilated environments ?
Will they provide a Microsoft compatible linker ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've put together a little executable that tests this:

#include <Windows.h>
#include <stdio.h>

extern IMAGE_DOS_HEADER __ImageBase;

int main() {
	HINSTANCE imgb = (HINSTANCE)&__ImageBase;
	HINSTANCE modh = GetModuleHandle(NULL);

	printf("ImageBase: %p\n", imgb);
	printf("GetModuleHandle: %p\n", modh);

	if (imgb == modh) {
		printf("SUCCESS: pointers are equivalent");
	} else {
		fprintf(stderr, "ERROR: pointer mismatch");
	}

	return 0;
}

running it on MSYS2 gives me the following output:

Aron@DESKTOP-6CIB9GL MINGW64 ~
$ gcc test.c -o test

Aron@DESKTOP-6CIB9GL MINGW64 ~
$ ./test
ImageBase: 00007ff783760000
GetModuleHandle: 00007ff783760000
SUCCESS: pointers are equivalent
Aron@DESKTOP-6CIB9GL MINGW64 ~
$ gcc --version
gcc.exe (Rev1, Built by MSYS2 project) 12.1.0
Copyright (C) 2022 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

Sadly my knowledge about windows linkers isn't deep enough, but my gut feeling is that everyone who went through building a windows linker directly or indirectly implements this "feature".

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good then. I was mostly concerned about MSSY2/Mingw...

Maybe we could fallback to the old behavior when required (should there be a way to detect that).
Not simple since we don't even know if such environments exist at all...

Copy link
Contributor

Choose a reason for hiding this comment

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

I went ahead and tested a rust equivalent of your test (rustc does not use gcc but llvm/clang afaik).

use windows_sys::Win32::Foundation::HINSTANCE;
use windows_sys::Win32::System::LibraryLoader::GetModuleHandleW;
use windows_sys::Win32::System::SystemServices::IMAGE_DOS_HEADER;

extern "C" {
    static __ImageBase: IMAGE_DOS_HEADER;
}

fn main() {
    let imgb = unsafe { &__ImageBase as *const IMAGE_DOS_HEADER as HINSTANCE };
    let modh = unsafe { GetModuleHandleW(std::ptr::null()) };

    println!("ImageBase: {}", imgb);
    println!("GetModuleHandle: {}", modh);

    if imgb == modh {
        println!("SUCCESS: pointers are equivalent");
    } else {
        println!("ERROR: pointer mismatch");
    }
}

Works on MINGW64 and on CLANG64 (not shown):

Philippe@DESKTOP MINGW64 ~/rust/handle
$ cargo run
   Compiling handle v0.1.0 (C:\msys64\home\Philippe\rust\handle)
    Finished dev [unoptimized + debuginfo] target(s) in 1.82s
     Running `target\debug\handle.exe`
ImageBase: 140698631536640
GetModuleHandle: 140698631536640
SUCCESS: pointers are equivalent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks good then. I was mostly concerned about MSSY2/Mingw...

Maybe we could fallback to the old behavior when required (should there be a way to detect that). Not simple since we don't even know if such environments exist at all...

Sadly I believe implementing a fallback would be quite difficult. There are two cases in which this wouldn't work:

  1. __ImageBase is not defined:

I have never seen this in practice, might be worth to get in touch with some people who know a bit more about linkers on Windows but I've never seen it fail anywhere. As far as I know, there is no way to detect whether FFI externs are defined at compile-time. The good thing is though, should this trick fail clients will be notified by compile-time errors so it would be quickly noticed and fixed.

  1. __ImageBase address is wrong:

This can only happen if someone redefined the variable. This is disallowed by the standard anyway (as variable names prefixed with two underscores are reserved for the compiler/implementation). I suppose in debug mode one could print a warning if __ImageBase diverges from GetModuleHandle(NULL), although in some cases that is to be expected (e.g. when running inside a DLL).

I have a high degree of confidence that this will be functional and an improvement over the current approach, however I am not an expert on Windows linkers so that's why I put it up as a draft for now. Might be worth looking into what other major libraries use this library to increase confidence in using it.

Copy link
Contributor

Choose a reason for hiding this comment

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

There seems to be another technique described in that stackoverflow link that doesn't depend on the linker. Has there been any experimentation with this? (I have not tried it myself)

https://stackoverflow.com/a/56804892

HMODULE getThisModuleHandle()
{
    //Returns module handle where this function is running in: EXE or DLL
    HMODULE hModule = NULL;
    ::GetModuleHandleEx(GET_MODULE_HANDLE_EX_FLAG_FROM_ADDRESS | 
        GET_MODULE_HANDLE_EX_FLAG_UNCHANGED_REFCOUNT, 
        (LPCTSTR)getThisModuleHandle, &hModule);

    return hModule;
}

Copy link
Member

@maroider maroider May 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

__ImageBase has been around for a long time (check out this blog post by Raymond Chen from 2004). I'd be surprised if we encountered any issues with it.

EDIT: I didn't read the contents of the PR all that closely, so I didn't see the blog post being already linked, lol.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The GetModuleHandleEx approach is clever and would probably work but there is another advantage of the __ImageBase: It does not require a syscall, it has the overhead of a global variable access which makes it the most efficient and fastest option.

Should there be a concern about it not working (even though we have yet to find a platform on where this doesn't work on - compatibility layers around windows have existed for decades and as maroider stated it's probably 2 decades old at this point so it's probably stable), we could lock it behind a feature gate. I'd still be in favour of making __ImageBase the default, as it is the more cost efficient option. Should it not be working, the GetModuleHandleEx approach could be used.

One of the big advantages is that this approach cannot silently fail. If __ImageBase is missing there'll be a compiler error, if its address is wrong, window creation will fail due to an invalid HINSTANCE. I believe it would be worth it to test this feature and should there be any issues this commit can be instantly reverted and we can get back to the drawing board and figure out a hybrid approach.


// This is preferred over GetModuleHandle(NULL) because it also works in DLLs:
// https://stackoverflow.com/questions/21718027/getmodulehandlenull-vs-hinstance

extern "C" {
static __ImageBase: IMAGE_DOS_HEADER;
}

unsafe { &__ImageBase as *const _ as _ }
}

impl CursorIcon {
pub(crate) fn to_windows_cursor(self) -> PCWSTR {
match self {
Expand Down
5 changes: 2 additions & 3 deletions src/platform_impl/windows/window.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ use windows_sys::Win32::{
Com::{
CoCreateInstance, CoInitializeEx, CoUninitialize, CLSCTX_ALL, COINIT_APARTMENTTHREADED,
},
LibraryLoader::GetModuleHandleW,
Ole::{OleInitialize, RegisterDragDrop},
},
UI::{
Expand Down Expand Up @@ -974,7 +973,7 @@ where
CW_USEDEFAULT,
parent.unwrap_or(0),
pl_attribs.menu.unwrap_or(0),
GetModuleHandleW(ptr::null()),
util::get_instance_handle(),
&mut initdata as *mut _ as *mut _,
);

Expand Down Expand Up @@ -1013,7 +1012,7 @@ unsafe fn register_window_class<T: 'static>(
lpfnWndProc: Some(super::event_loop::public_window_callback::<T>),
cbClsExtra: 0,
cbWndExtra: 0,
hInstance: GetModuleHandleW(ptr::null()),
hInstance: util::get_instance_handle(),
hIcon: h_icon,
hCursor: 0, // must be null in order for cursor state to work properly
hbrBackground: 0,
Expand Down