RawWindowHandle's HasRawWindowHandle implementation is unsound

[Osspial]

The impl HasRawWindowHandle for RawWindowHandle added in #29 is a soundness hole, since it allows safe code to instantiate an invalid handle and pass it to a function that relies on HasRawWindowHandle being implemented correctly:

fn use_raw_window_handle<H: HasRawWindowHandle>(h: H) {/*impl*/}

use_raw_window_handle(RawWindowHandle::Windows(WindowsHandle {
    hwnd: 41 as *mut c_void,
    hinstance: 43 as *mut c_void,
    ..WindowsHandle::empty()
}));

Removing that implementation is trivial, but there are two unresolved questions:

• Should we expose some sort of pub RawWindowHandleActive(RawWindowHandle) struct that can only be initialized with an unsafe function?
• Does removing the impl require a bump to 0.4.0? The official Rust semver guidelines say that soundness fixes don't count as breaking changes, so I'm leaning towards no.

[icefoxen]

Far, far from an expert but if you provide a trait that provides soundness guarantees for other people to implement and document how to impl it correctly, then it's their problem if they don't impl it correctly. It's your job to try to work with both upstream and downstream to make things work but you can't force either to comply.

[Lokathor]

No, the fault is that an implementing struct in this crate does it wrong

just delete the struct. Few if any people will ever need to add this trait to their type, and they're pro enough to be able to at least read some docs.

[Ralith]

Is this hypothetical safety guarantee actually a practically one to begin with? What do graphics APIs do if you delete the window out from under them after setup? My intuition here is that anything working with raw window handles is necessarily unsafe.

[Ralith]

I think releasing 0.3.1 and yanking 0.3.0 would be acceptable if deemed necessary, but I'm not sure there are meaningful safety gains to be had, presuming that most things that take a HasRawWindowHandle will ultimately capture the resulting handle without lifetime guarantees.

[Lokathor]

The issue is that the contract so far is that by implementing the trait you're assuring that whatever you return is valid and that unsafe code can trust you since you're also unsafe code.

And then this breaks that because unsafe code shouldn't generally trust safe code.

[Ralith]

Any guarantees you might get by that go out the window the moment you store the RawWindowHandle even implicitly. Given the asynchronous and thread-safe behavior of some windowing systems, those guarantees might even go out the window before you finish calling raw_window_handle.

[Osspial]

Thank you all for weighing in. I'm going to go ahead and remove the implementation in 0.3.1.

@Ralith You make some interesting points about the amount of safety this crate can conceivably provide.

Given the asynchronous and thread-safe behavior of some windowing systems, those guarantees might even go out the window before you finish calling raw_window_handle.

I refuse to entertain this as a possibility. Windowing APIs do some questionable things, but forcibly invalidating a window handle pointer without notifying the application seems beyond the realm of sanity. That being said, HasRawWindowHandle doesn't currently provide any explicit guarantees about the lifetime of the handle it provides; it should be reasonable for us to guarantee that the RawWindowHandle will be valid for the lifetime of the HasRawWindowHandle implementer, which we currently do not do*.

@kvark Out of curiosity, does Vulkan (or any of the other graphics APIs, for that matter) specify what happens if the underlying window handle for a surface is destroyed before the corresponding graphics API surface object is destroyed?

---

^{* I'd argue that it implicitly guarantees that, since handles returned by raw_window_handle must be valid when returned and the exact handle cannot change during the lifetime of the HasRawWindowHandle implementer. It doesn't hurt to make that explicit, though.}

[Ralith]

forcibly invalidating a window handle pointer without notifying the application seems beyond the realm of sanity.

Not if the application explicitly deletes the window itself! Admittedly a soundness guarantee in terms of the lifetime of the handle's producer is probably still useful.

what happens if the underlying window handle for a surface is destroyed before the corresponding graphics API surface object is destroyed?

For example, the Wayland WSI states:

Some Vulkan functions may send protocol over the specified wl_display connection when using a swapchain or presentable images created from a VkSurfaceKHR referring to a wl_surface. Applications must therefore ensure that both the wl_display and the wl_surface remain valid for the lifetime of any VkSwapchainKHR objects created from a particular wl_display and wl_surface.

"must" should be read as "are unsound if failing to".