A Window constructor that throws leaves this in Device::windows and as listener data, so the next window touches freed memory #169

New issue

Open

opened 2026-10-31 14:57:15 +00:00 by jorijnvdgraaf · 1 comment

jorijnvdgraaf commented

2026-10-31 14:57:15 +00:00

Owner

Summary

On Wayland the Window constructor registers the window before the steps that can throw: Device::windows holds this, and the xdg surface, the toplevel and the fractional scale have it as listener data. When a later step throws, ~Window does not run, so nothing takes this back or destroys those objects, and the storage they point at is released. An application that catches the error and carries on reads and writes freed memory. Line numbers refer to 670df31.

Details

implementations/Crafter.Graphics-Window.cpp:358: Device::windows.push_back(this); :363-364 and :378: the listeners get this.
Steps after that which throw: Device::CheckVkResult (implementations/Crafter.Graphics-Device.cpp:134) on surface creation (Window.cpp:459) and the surface queries that follow; no surface format a storage image can be written through (:499); CreateSwapchain (:531), which also throws when the driver makes the swapchain with other than numFrames images (:1640).
~Window (:633) is what erases the window from Device::windows, clears the focus pointers and destroys the proxies. A constructor that throws never reaches it.

Afterwards Device::windows holds a dangling pointer. The event loop walks it (Window.cpp:928, :967, :992, :1007), and the text-input, pointer, keyboard and touch enters dereference every entry while matching their surface (Device.cpp:383, :502, :569, :684). The leaked toplevel can still be sent configures, which xdg_toplevel_configure writes into the freed object (Window.cpp:1766-1767).

Found by reading the code. The likeliest trigger is a surface without a storage-writable format, in an application that catches the error to report it or to try again.

Suggested fix

Make the constructor exception-safe: factor ~Window's teardown into a function and run it from a guard local to the constructor unless construction completes, or register this in Device::windows last and destroy what was made before rethrowing. A function-try-block won't do, since its handler may not touch the members.

## Summary On Wayland the `Window` constructor registers the window before the steps that can throw: `Device::windows` holds `this`, and the xdg surface, the toplevel and the fractional scale have it as listener data. When a later step throws, `~Window` does not run, so nothing takes `this` back or destroys those objects, and the storage they point at is released. An application that catches the error and carries on reads and writes freed memory. Line numbers refer to `670df31`. ## Details - `implementations/Crafter.Graphics-Window.cpp:358`: `Device::windows.push_back(this)`; `:363-364` and `:378`: the listeners get `this`. - Steps after that which throw: `Device::CheckVkResult` (`implementations/Crafter.Graphics-Device.cpp:134`) on surface creation (`Window.cpp:459`) and the surface queries that follow; no surface format a storage image can be written through (`:499`); `CreateSwapchain` (`:531`), which also throws when the driver makes the swapchain with other than `numFrames` images (`:1640`). - `~Window` (`:633`) is what erases the window from `Device::windows`, clears the focus pointers and destroys the proxies. A constructor that throws never reaches it. Afterwards `Device::windows` holds a dangling pointer. The event loop walks it (`Window.cpp:928`, `:967`, `:992`, `:1007`), and the text-input, pointer, keyboard and touch enters dereference every entry while matching their surface (`Device.cpp:383`, `:502`, `:569`, `:684`). The leaked toplevel can still be sent configures, which `xdg_toplevel_configure` writes into the freed object (`Window.cpp:1766-1767`). Found by reading the code. The likeliest trigger is a surface without a storage-writable format, in an application that catches the error to report it or to try again. ## Suggested fix Make the constructor exception-safe: factor `~Window`'s teardown into a function and run it from a guard local to the constructor unless construction completes, or register `this` in `Device::windows` last and destroy what was made before rethrowing. A function-try-block won't do, since its handler may not touch the members.

jorijnvdgraaf commented

2026-10-31 17:23:02 +00:00

Author

Owner

The Win32 constructor has the same problem. Line numbers refer to implementations/Crafter.Graphics-Window.cpp at 852ae8a.

CreateWindowEx (:417) passes this as the create parameter, WM_NCCREATE stores it in GWLP_USERDATA (:140-148), and ShowWindow (:434) puts the window on screen.
The steps after that throw as on Wayland: vkCreateWin32SurfaceKHR through CheckVkResult (:451), the storage-format check (:499), CreateSwapchain (:531).
~Window is what detaches the HWND and destroys it (:687-688). A constructor that throws never gets there.

So the window stays on screen with GWLP_USERDATA pointing at the freed Window. Once anything pumps messages on that thread again (the application's own loop, another window's StartSync, a MessageBox), hovering, clicking, resizing or closing it sends WM_SETCURSOR, WM_MOUSEMOVE, the button messages, WM_SIZE or WM_CLOSE into WndProc, which writes into the freed object and invokes its destroyed events. The VkSurfaceKHR leaks too.

The guard suggested above covers it if it also detaches and destroys the HWND and destroys the surface. Found while reviewing claude/win32-title.

The Win32 constructor has the same problem. Line numbers refer to `implementations/Crafter.Graphics-Window.cpp` at `852ae8a`. - `CreateWindowEx` (`:417`) passes `this` as the create parameter, `WM_NCCREATE` stores it in `GWLP_USERDATA` (`:140-148`), and `ShowWindow` (`:434`) puts the window on screen. - The steps after that throw as on Wayland: `vkCreateWin32SurfaceKHR` through `CheckVkResult` (`:451`), the storage-format check (`:499`), `CreateSwapchain` (`:531`). - `~Window` is what detaches the HWND and destroys it (`:687-688`). A constructor that throws never gets there. So the window stays on screen with `GWLP_USERDATA` pointing at the freed `Window`. Once anything pumps messages on that thread again (the application's own loop, another window's `StartSync`, a `MessageBox`), hovering, clicking, resizing or closing it sends `WM_SETCURSOR`, `WM_MOUSEMOVE`, the button messages, `WM_SIZE` or `WM_CLOSE` into `WndProc`, which writes into the freed object and invokes its destroyed events. The `VkSurfaceKHR` leaks too. The guard suggested above covers it if it also detaches and destroys the HWND and destroys the surface. Found while reviewing `claude/win32-title`.