What exactly is the unsoundness of dropping a window handle earlier than the consumer?

[madsmtm]

On a lot of platforms, the window handle is reference-counted:

• UIKit: UIView is reference-counted.
• AppKit: NSView is reference-counted. If you close or destroy the original window, the view can still be used (it just won't show anything).
• OpenHarmony OS NDK: Same as Android?
• WinRT: CoreWindow, seems to be garbage collected / managed by the C# runtime.
• Web: HtmlCanvasElement / OffscreenCanvas (JsValue is garbage-collected IIUC).
• Android NDK: ANativeWindow can be acquired, and remains valid until it's released.
• Orbital: Windows are file descriptors IIUC.

On other platforms, the window is just an integer, and if the window is destroyed you'll either get an error, or attempt to do something with the wrong window:

• Xlib: Window = XID, if you destroy the window, you might just get a BadWindow error in later calls.
• Xcb: xcb_window_t, probably same as above (?)
• Wayland: wl_surface, can be destroyed in safe code, probably also just causes protocol errors later on, but not unsoundness (?)
• Win32: HWND, pretty sure this is "safe" to use after being destroyed, they're indexes into a thread-local table.

(Note that I'm (even more) unsure about the DRM, GBM and Haiku handles).

This difference means that it's impossible for us to provide a function that converts a WindowHandle<'_> to an OwnedWindowHandle (which would be possible if all handles used reference-counting), because we cannot prevent the handle from being destroyed, only the original creator of it can.

But maybe we don't need to? As long as we can create some Rusty wrapper around the WindowHandle<'_> while it is guaranteed to be alive (i.e. bump the reference count on the handles that need that), and that the wrapper can still be soundly used afterwards (even if it ends up erroring)? My desire is to remove the generics from Softbuffer, CC rust-windowing/softbuffer#298.

---

In any case, I think our current documentation around why the design is as it is this is clearly not good enough, because my mind keeps coming back to this issue.

[madsmtm]

Stated differently, why and on which platforms would the following code be unsound (if it even is)?

fn main() {
    let mut surface = None;

    let event_loop = EventLoop::new().unwrap();
    event_loop.run(|| {
        let window = event_loop.create_window(...);
        let context = Context::new(&window);
        let tmp_surface = Surface::new(window, &context);
        // Remove the lifetime information from the surface.
        surface = Some(unsafe {
           std::mem::transmute::<
               Surface<&Window, &Window>,
               Surface<&'static Window, &'static Window>,
           >(tmp_surface)
        });
    });
    drop(event_loop);

    // Event loop and window dropped here! After that, try to use or drop the surface:
    let buffer = surface.unwrap().buffer_mut();
    buffer.present();
    drop(surface);
}

[kchibisov]

I'd assume you also want this #158 , like me?

[madsmtm]

I'd assume you also want this #158 , like me?

Hmm, I think somewhat yes, in that we should have a solution to recommend users (consumer crates) by default that isn't generics, whether that's a (potentially wrapped) Arc<dyn HasWindowHandle> or something else.

[notgull]

In any case, I think our current documentation around why the design is as it is this is clearly not good enough, because my mind keeps coming back to this issue.

The fundamental issue is that raw-window-handle is designed as an intermediary between windowing systems and graphics libraries, in such a way that it's not opinionated on the underlying linking strategy.

I'd be a fan of creating a new owned-window-handle crate (that's not stable) that builds on raw-window-handle to add ref-counted, owned window handles. I would argue that raw-window-handle isn't the place for this, or at least not without an optional feature.

• Web: HtmlCanvasElement / OffscreenCanvas (JsValue is garbage-collected IIUC).

This would stop mattering if we get rid of JsValue as proposed in #177

• Wayland: wl_surface, can be destroyed in safe code, probably also just causes protocol errors later on, but not unsoundness (?)

Are we not at risk of a use-after-free here? The wl_surface is a pointer.

[MarijnS95]

Just adding (from rust-windowing/softbuffer#298) that winit as provider "solved" this for DisplayHandle with their OwnedDisplayHandle. Perhaps it would be its "responsibility" to provide the same thing for Window if at all possible/desired using internal (A)rcs etc though I'm currently unclear of its soundness wrt unique ownerhip/references?

[notgull]

I'd be a fan of creating a new owned-window-handle crate (that's not stable) that builds on raw-window-handle to add ref-counted, owned window handles. I would argue that raw-window-handle isn't the place for this, or at least not without an optional feature.

I've created a demo of what this would look like here. Let me know your thoughts on this.

Not sure if it's worth it to have a DisplayHandle as well. It's probably not worth it; both wgpu and softbuffer create their own internal connections if there's no display. Maybe it's worth it to have some kind of DisplayHandle::empty() function in this crate, that picks a good default based on the platform? Could be a footgun.

[notgull]

I just realized that GBM surfaces are not refcounted. So this wouldn't work for GBM at least.

(Not like it practically matters, I doubt any of our downstream consumers use GBM).

[madsmtm]

wl_surface

It seems that wayland-client/wayland-backend jumps through some hoops to make things work. Does WlSurface::destroy not actually destroy the surface until all references are dropped?

Or is it the lifetime of the wl_resource that's problematic, and it's the destruction of that which is deferred?

Could Softbuffer work around things with wl_resource_add_destroy_listener somehow? Maybe:

struct Surface {
    // `Arc` is kept alive until the `WlSurface`' underlying data is destroyed elsewhere.
    surface: Weak<WlSurface>,
}

impl Surface {
    fn new(handle: WindowHandle<'_>) -> Self {
        // TODO: handle -> surface
        let surface_strong_ref = Arc::new(surface);
        let surface_weak_ref = Arc::downgrade(&surface_strong_ref);
        wl_resource_add_destroy_listener(&surface_strong_ref.resource, || {
            // Wait until the surface' strong refcount is down to just this one.
            //
            // Alternatively, store `Arc<Mutex<Option<WlSurface>>>`, we just
            // need some way to allow blocking in the critical sections where the surface is in use.
            loop {
                match Arc::try_unwrap(surface_strong_ref) {
                    Ok(surface) => {
                        drop(surface);
                        break;
                    },
                    Ok(arc) => surface_strong_ref = arc,
                }
            }
        });
        Self { surface: surface_weak_ref }
    }
}

impl Buffer {
    fn present(self) {
        let surface = self.surface.upgrade()?;
        // Critical section with `surface` "locked" or in use. commit here etc.
        drop(surface);
    }
}

That would cause an error if the Surface is used after the window is dropped, but at least it wouldn't be unsound?

CC @ids1024

[madsmtm]

I've created a demo of what this would look like here. Let me know your thoughts on this.

I think it's very nice! If we can make that sound on Wayland with FFI / with different versions of wayland-backend as well, then I'm in heaven!

[madsmtm]

Nvmd, I understood things wrongly, we're backed by wl_proxy.

But maybe there's a similar approach where we somehow listen to wl_proxy_destroy on the surface, and if that is called, avoid using the wl_proxy*?

[notgull]

I think we could use wl_proxy_get_id followed by wl_proxy_create_for_id to create a fresh proxy that uses the same ID. This turns a memory safety error into a protocol error, which I'm fine with. The main downside is that wl_proxy_create_for_id is dark magic, to the point where wayland-backend doesn't even expose it at this point.

[notgull]

Alternatively, we could just make it so owned-window-handle will only accept objects that are managed by wayland-backend, since that already tracks which objects are alive.

[notgull]

Now that I've worked through it, this is still definitely unsound, unless we have the graphics implementation constantly check whether the wl_proxy is still valid. There's a race where:

• We create an OwnedWindowHandle and pass it into softbuffer.
• Softbuffer only calls window_handle once to get the wl_surface.
• The backing window is closed, which frees the wl_surface.
• The next time Softbuffer tries to write to the window, a use after free occurs.

Not sure if there is a real way around this. We could enforce stronger ownership guarantees in wayland-backend, I guess? Alternatively we find some hack that lets us set the ID in a proxy manually, since it's all numeric in the backend. It's frustrating that the Wayland client doesn't expose a way of doing this.

[notgull]

This doesn't work unless we explicitly don't allow Wayland handles to be used in this way... which defeats the entire point, really.