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

On Windows, only return the window handle if it is on the correct thread #3593

Merged
merged 1 commit into from
Apr 26, 2024

Conversation

notgull
Copy link
Member

@notgull notgull commented Mar 17, 2024

On Windows, it is generally unsafe to use the HWND outside of the thread
that it originates from. In reality, the HWND is an index into a
thread-local table, so using it outside of the GUI thread can result in
another window being used instead, following by code unsoundness. This
is why the WindowHandle type is !Send. However, Window is Send and Sync,
which means we have to account for this.

Thus far the best solution seems to be to check if we are not in the GUI
thread. If we aren't, refuse the return the window handle.


  • Tested on all platforms changed
  • Added an entry to the changelog module if knowledge of this change could be valuable to users
  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
  • Created or updated an example program if it would help users understand this functionality
  • Updated feature matrix, if new features were added or implemented

@Stehfyn
Copy link

Stehfyn commented Mar 18, 2024

Confirmed to operate as intended on windows

@kchibisov
Copy link
Member

How does it behave with e.g. EGL where windows are Send(by the spec)? I guess it just creates and then the surface is being send to other thread, and the HWND itself is not touched during that?

@@ -382,6 +382,12 @@ impl Window {
#[cfg(feature = "rwh_06")]
#[inline]
pub fn raw_window_handle_rwh_06(&self) -> Result<rwh_06::RawWindowHandle, rwh_06::HandleError> {
// If we aren't in the GUI thread, we can't return the window.
Copy link
Member

Choose a reason for hiding this comment

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

This should link to docs on why it's like that.

fn issue_3593() {
// We should not be able to get a window handle off of the main thread.
let event_loop = EventLoop::new().unwrap();
event_loop.run(|_, elwt| {
Copy link
Member

Choose a reason for hiding this comment

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

Don't use deprecated API.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is there a better way to create a closure application ad-hoc?

Copy link
Member

Choose a reason for hiding this comment

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

Not really, no. This would have to be done with ApplicationHandler:

struct App;
impl ApplicationHandler for App {
    fn new_events(&mut self, event_loop: &ActiveEventLoop, _cause: StartCause) {
        // Test code in here
    }
}

fn issue_3593() {
    let event_loop = EventLoop::new().unwrap();
    event_loop.run_app(App).unwrap();
}

I guess for the purpose of integration tests, we should probably have some sort of mechanism to do this more easily.

@msiglreith
Copy link
Member

On Windows, it is generally unsafe to use the HWND outside of the thread
that it originates from. In reality, the HWND is an index into a
thread-local table, so using it outside of the GUI thread can result in
another window being used instead, following by code unsoundness.

Is there a reference for this somewhere?

@notgull
Copy link
Member Author

notgull commented Mar 18, 2024

Generally windows are thread unsafe unless otherwise specified. For instance, Raymond Chen says:

Informally, one says that the thread “owns” the window. Messages are dispatched to a window procedure only on the thread that owns it, and generally speaking, modifications to a window should be made only from the thread that owns it.

So HWNDs are !Send but Sync... except not really, since the main thread can set values and cause race conditions in the higher level threads.

From the raw-window-handle docs:

Users can safely assume that non-null/0 fields are valid handles, and it is up to the implementer of this trait to ensure that condition is upheld.

I'm saying that the definition of "valid" is "valid for the current context." As HWNDs are largely useless outside of the thread that they originate from, it is invalid to issue these handles outside of the origin thread.

@madsmtm
Copy link
Member

madsmtm commented Mar 21, 2024

CC rust-windowing/raw-window-handle#154 and rust-windowing/raw-window-handle#85 (comment), talk about making Win32WindowHandle !Send and !Sync (in the next breaking version of raw-window-handle).

@kchibisov
Copy link
Member

I'd really like some confirmation from win32 docs/devs. Since the link is 20 years old, so maybe something changed or there's alternative.

@notgull
Copy link
Member Author

notgull commented Mar 22, 2024

@Stehfyn Can you comment here?

@msiglreith
Copy link
Member

I can see HWND or a wrapper of it being !Send/!Sync in similar fashion as ptr.
I'm not agreeing on the thread-local argument, given one could also query it from another process for example and interact with it as it's rather an index into an OS managed array. Therefore, I also feel like we should artifically forbid access to it from another thread.

@kchibisov
Copy link
Member

kchibisov commented Mar 22, 2024

I mean how OpenGL/Vulkan then operate where every surface resource is Send between threads (by spec), and they build it from the HWND?

notgull added a commit to rust-windowing/raw-window-handle that referenced this pull request Mar 23, 2024
This commit clarifies what is expected by the system for a Win32 handle.
It is expected that the Win32 handle belongs to the current thread. This
is not a breaking change as it was previously necessary for the window
to be considered "valid".

cc rust-windowing/winit#3593

Signed-off-by: John Nunley <dev@notgull.net>
@notgull
Copy link
Member Author

notgull commented Mar 23, 2024

I mean how OpenGL/Vulkan then operate where every surface resource is Send between threads (by spec), and they build it from the HWND?

I've added a couple of escape hatches to allow for thread-safe usage. However for the default usage we should account for thread affinity

notgull added a commit to rust-windowing/raw-window-handle that referenced this pull request Mar 24, 2024
This commit clarifies what is expected by the system for a Win32 handle.
It is expected that the Win32 handle belongs to the current thread. This
is not a breaking change as it was previously necessary for the window
to be considered "valid".

cc rust-windowing/winit#3593

Signed-off-by: John Nunley <dev@notgull.net>
@notgull notgull added the C - nominated Nominated for discussion in the next meeting label Mar 26, 2024
@notgull notgull requested a review from kchibisov March 30, 2024 03:37
@daxpedda daxpedda added this to the Version 0.30.0 milestone Apr 19, 2024
@daxpedda daxpedda removed the C - nominated Nominated for discussion in the next meeting label Apr 19, 2024
On Windows, it is generally unsafe to use the HWND outside of the thread
that it originates from. In reality, the HWND is an index into a
thread-local table, so using it outside of the GUI thread can result in
another window being used instead, following by code unsoundness. This
is why the WindowHandle type is !Send. However, Window is Send and Sync,
which means we have to account for this.

Thus far the best solution seems to be to check if we are not in the GUI
thread. If we aren't, refuse the return the window handle.

For users who want to ensure the safety themselves, the unsafe API
was added.

Signed-off-by: John Nunley <dev@notgull.net>
Copy link
Member

@kchibisov kchibisov left a comment

Choose a reason for hiding this comment

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

Given it just adds functionally, it should be good to go.

@kchibisov kchibisov merged commit 1682703 into master Apr 26, 2024
52 checks passed
@kchibisov kchibisov deleted the notgull/winrwh branch April 26, 2024 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

6 participants