BlackMark/CoopAllTheThings
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Fix shutdown use-after-free of UiState via the ImGui settings handler

Browse Source 
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 <noreply@anthropic.com>
This commit is contained in:
BlackMark
2026-06-24 01:05:47 +02:00
parent 3f1ce0c78a
commit 43d093405e
2 changed files with 5 additions and 4 deletions
Download Patch File Download Diff File Expand all files Collapse all files

3
README.md
View File

@@ -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. as its own commit; "verify" items are confirmed real before any change, and dropped if not.
Confirmed bugs: 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 - **Host device loss unhandled** — `Present`/`ResizeBuffers`/`CreateRenderTargetView` HRESULTs are
ignored and a removed/reset device spins silently. Detect `DEVICE_REMOVED/RESET`, log ignored and a removed/reset device spins silently. Detect `DEVICE_REMOVED/RESET`, log
`GetDeviceRemovedReason`, surface it, and halt the render loop cleanly. `GetDeviceRemovedReason`, surface it, and halt the render loop cleanly.

6
host/src/main.cpp
View File

@@ -323,6 +323,10 @@ int run()
return 1; 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; coop::ImGuiLayer imgui;
if (!imgui.init(window.hwnd(), window.device(), window.context())) if (!imgui.init(window.hwnd(), window.device(), window.context()))
{ {
@@ -364,9 +368,9 @@ int run()
double overlay_hidden_at = 0.0; double overlay_hidden_at = 0.0;
double last_shot_at = -10.0; // when the last F10 screenshot was saved (for the toast) double last_shot_at = -10.0; // when the last F10 screenshot was saved (for the toast)
std::string last_shot_name; std::string last_shot_name;
coop::UiState ui;
// Persist the "Debug details" verbosity in the .ini. Register before the first // 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. // 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::register_ui_settings(ui);
coop::FrameStats stats; coop::FrameStats stats;