From 43d093405e07399e4a66ec901cb3b9b223ad5b7b Mon Sep 17 00:00:00 2001 From: BlackMark Date: Wed, 24 Jun 2026 01:05:47 +0200 Subject: [PATCH] Fix shutdown use-after-free of UiState via the ImGui settings handler register_ui_settings installs an ImGui settings handler whose UserData points at the run()-local UiState. ~ImGuiLayer calls DestroyContext, which flushes the .ini through that handler (ui_settings_write_all dereferences UserData). But UiState was declared after ImGuiLayer in run(), so it (and the panels between them) were destroyed first -- the shutdown save read freed/clobbered stack every clean exit (UB; could corrupt coop_layout.ini / the persisted debug-details flag). Declare UiState before ImGuiLayer so it outlives the context and is destroyed last. Not unit-testable (shutdown lifetime ordering); fixed by inspection. Co-Authored-By: Claude Opus 4.8 --- README.md | 3 --- host/src/main.cpp | 6 +++++- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 61a5dfc..bf13360 100644 --- a/README.md +++ b/README.md @@ -114,9 +114,6 @@ From an in-depth review pass. Each item is fixed test-first (a failing test, the as its own commit; "verify" items are confirmed real before any change, and dropped if not. Confirmed bugs: -- **Shutdown use-after-free of `ui`** — the ImGui settings handler holds `&ui`, but `~ImGuiLayer`'s - `DestroyContext` saves settings *after* `ui` (declared later in `run()`) is destroyed. Reorder / - remove the handler before teardown. - **Host device loss unhandled** — `Present`/`ResizeBuffers`/`CreateRenderTargetView` HRESULTs are ignored and a removed/reset device spins silently. Detect `DEVICE_REMOVED/RESET`, log `GetDeviceRemovedReason`, surface it, and halt the render loop cleanly. diff --git a/host/src/main.cpp b/host/src/main.cpp index bea85e7..3958ccc 100644 --- a/host/src/main.cpp +++ b/host/src/main.cpp @@ -323,6 +323,10 @@ int run() return 1; } + // `ui` must outlive `imgui`: register_ui_settings (below) installs an ImGui settings handler that + // holds &ui, and ~ImGuiLayer's DestroyContext flushes the .ini through that handler on shutdown. + // Declaring ui first means it's destroyed AFTER imgui, so that final save never reads freed state. + coop::UiState ui; coop::ImGuiLayer imgui; if (!imgui.init(window.hwnd(), window.device(), window.context())) { @@ -364,9 +368,9 @@ int run() double overlay_hidden_at = 0.0; double last_shot_at = -10.0; // when the last F10 screenshot was saved (for the toast) std::string last_shot_name; - coop::UiState ui; // Persist the "Debug details" verbosity in the .ini. Register before the first // begin_frame() below, which is when ImGui loads the .ini and replays our handler. + // (ui is declared earlier, before imgui, so it outlives the context -- see the note there.) coop::register_ui_settings(ui); coop::FrameStats stats;