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

Jump to bottom

Windows main thread ID check does not work when library is compiled as dylib + process is loaded with DLL injection #3999

Open
PJB3005 opened this issue Nov 20, 2024 · 6 comments · May be fixed by #4003 or #4006
Open

Windows main thread ID check does not work when library is compiled as dylib + process is loaded with DLL injection #3999

PJB3005 opened this issue Nov 20, 2024 · 6 comments · May be fixed by #4003 or #4006
Labels
B - bug Dang, that shouldn't have happened DS - windows

Comments

@PJB3005
Copy link

PJB3005 commented Nov 20, 2024

Description

The Windows platform layer has a hacky method to infer the "main thread ID", to avoid people developing on Windows having their code not work on other platforms:

winit/src/platform_impl/windows/event_loop.rs

Lines 563 to 584 in f6b2085

fn main_thread_id() -> u32 {
static mut MAIN_THREAD_ID: u32 = 0;
/// Function pointer used in CRT initialization section to set the above static field's value.
// Mark as used so this is not removable.
#[used]
#[allow(non_upper_case_globals)]
// Place the function pointer inside of CRT initialization section so it is loaded before
// main entrypoint.
//
// See: https://doc.rust-lang.org/stable/reference/abi.html#the-link_section-attribute
#[link_section = ".CRT$XCU"]
static INIT_MAIN_THREAD_ID: unsafe fn() = {
unsafe fn initer() {
unsafe { MAIN_THREAD_ID = GetCurrentThreadId() };
}
initer
};
unsafe { MAIN_THREAD_ID }
}

This check does not work in the following scenario:

  • winit is compiled as a dylib (for example, when compiling bevy as a dylib)
  • the process is launched with a DLL injection

When winit is statically compiled into the main exe, this gets ran only from the actual CRT startup in the exe's main. But when the library is compiled into a dylib, the CRT actually calls it from DllMain. This doesn't normally matter because that usually goes hand in hand...

Except with the standard DLL injection technique (Steam, Renderdoc, ...): start the process suspended, run a new thread to load the injected DLL, resume the main thread. In this scenario, DllMain actually gets ran from the thread used to load the injected DLL, which is not the "main thread"...

Windows version

Edition	Windows 11 Pro
Version	23H2
Installed on	‎17/‎12/‎2023
OS build	22631.4460
Experience	Windows Feature Experience Pack 1000.22700.1047.0

Winit version

0.30.5
@PJB3005 PJB3005 added B - bug Dang, that shouldn't have happened DS - windows labels Nov 20, 2024
@madsmtm
Copy link
Member

madsmtm commented Nov 20, 2024

What do you propose we do instead?

Except with the standard DLL injection technique (Steam, Renderdoc, ...): start the process suspended, run a new thread to load the injected DLL, resume the main thread. In this scenario, DllMain actually gets ran from the thread used to load the injected DLL, which is not the "main thread"...

I no longer own a Windows machine, so I don't really know the details of DLLs, but could you show how that would work in Rust?

@PJB3005
Copy link
Author

PJB3005 commented Nov 20, 2024

What do you propose we do instead?

Quite frankly I doubt there is a proper way to make this check work. At the end of the day this is at most a debug assert that hackily tries to replicate behavior that Windows does not have for very good reasons.

I fully understand why winit tries but it's clear it's actually just not something that can ever be fully reliable. Personally I'm less of a perfectionist so I'd just remove it, but I'm assuming that won't fly I'd say at the very least:

  • Do not run this check on release builds, period. This is a debug assert for development, nothing more.
  • Have an env var to turn it off on debug builds. That way I don't have to dig through Bevy's middleware to reconfigure this myself when running into an issue with a tool like RenderDoc.

For what it's worth this check would also break if you do something weird like compile your entire game to a library and then dynamically load it from some launcher application from another thread. Or something. I don't know why you would, but I think it's best nobody else has to lose sleep debugging this in the future.

but could you show how that would work in Rust?

That was just outlining how a DLL injection is generally done on Windows, it's not really something we need to care about.

@PJB3005
Copy link
Author

PJB3005 commented Nov 20, 2024

I guess another option (and certainly my preferred one) would be to turn it into a warning log, but I'm not sure if winit has a system for that?

@madsmtm
Copy link
Member

madsmtm commented Nov 20, 2024

Sorry, I was confused how you were hitting this, since I misremembered how it works, but I now see that it's initialized by the system runtime.

In any case, we do have EventLoopBuilderExtWindows::with_any_thread to allow you to turn it off, would that work for you?

If not, I'd be fine with making it a warning (via tracing::warn!).

@PJB3005
Copy link
Author

PJB3005 commented Nov 20, 2024

would that work for you?

It does work, yes. I already set the relevant property in my Bevy project. I'm just filing this bug because I'm certainly not the last person that's going to get bitten and have to turn this on manually.

More insidiously it's not impossible for somebody to publish a Bevy game in the future with dylib mode (in spite of this being recommended against) and then the game "works fine" and mysteriously breaks for users trying to run it with the Steam overlay. Not very pleasant to just have lurking in a library like this!

@PJB3005
Copy link
Author

PJB3005 commented Nov 20, 2024

If not, I'd be fine with making it a warning (via tracing::warn!).

Sounds good, I'll send a PR with that + putting it behind debug_assertions.

PJB3005 added a commit to PJB3005/winit that referenced this issue Nov 21, 2024
@PJB3005
Make Windows main thread checks more forgiving
d690880 
The check on Windows to enforce that the event loop is created from the main thread is not fully reliable (rust-windowing#3999). Given that this check is not necessary on Windows and only exists more as an assert to enforce platform consistency in development, I have made it more lax:

* It now produces a tracing::warn!() message instead of an outright panic.
* The check is only compiled in with cfg(debug_assertions)
@PJB3005 PJB3005 linked a pull request Nov 21, 2024 that will close this issue
Open
5 tasks
@notgull notgull linked a pull request Nov 23, 2024 that will close this issue
Open
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
B - bug Dang, that shouldn't have happened DS - windows
Milestone
No milestone
2 participants
@PJB3005 @madsmtm