Detach RED4ext hooks at the top of App::Shutdown

[JustinMDotNet]

Closes #141.

What this changes

Moves the detour-detach block from App::Destruct into a new App::DetachHooks() member that:

1. Is called at the top of App::Shutdown, before any system is shut down.
2. Is also called from App::Destruct as a defence-in-depth no-op for the ungraceful-exit path.
3. Is idempotent via a new m_hooksAttached flag that AttachHooks sets on success and DetachHooks clears.

AttachHooks is no longer const because it now sets m_hooksAttached.

Why

In the original code, _QuickExit_fnc called app->Shutdown() and then chained the original quick_exit. Hooks remained installed for the rest of process teardown because the actual detach happened in App::Destruct, which only runs from DLL_PROCESS_DETACH later.

Three currently-installed hooks call back into App::GetXxxSystem() from their detour bodies:

• Hooks/InitScripts.cpp:27 → App::Get()->GetScriptCompilationSystem()
• Hooks/LoadScripts.cpp:20 → same
• Hooks/CGameApplication.cpp:19-77 → indirectly via States::*::Attach

GetXxxSystem() does m_systems.at(...) (App.cpp:182-210), which throws std::out_of_range on an empty vector. After Shutdown ran m_systems.clear();, an in-flight detour on another thread (or a queued continuation on the same thread) would throw straight into game code. No try/catch on the game side → std::terminate, ugly exit dialog, "RED4ext crashed the game on quit" report.

Detaching the hooks first closes that window. Once DetachHooks() returns, the trampolines are gone and the game code runs unmodified for the rest of teardown.

Design choice

Picked the "detach hooks first" option from the issue rather than "make accessors null-safe" because it requires no changes to any hook body and it preserves the existing contract that GetXxxSystem() returns a valid pointer. Making accessors null-safe would also be defensible defence-in-depth, but it would mean every detour body has to handle a null system gracefully, which is invasive.

The defence-in-depth DetachHooks() call in Destruct preserves the original behaviour for the case where Shutdown was never called (e.g. DLL_PROCESS_DETACH with aReserved == nullptr and no preceding QuickExit). The idempotency check via m_hooksAttached makes the double-call from the normal path safe.

What I tested

Local rebuild. The flow under each path:

• Normal QuickExit: QuickExit → app->Shutdown() → DetachHooks() (commits, m_hooksAttached = false) → systems shut down in reverse → m_systems.clear() → eventually App::Destruct → DetachHooks() (no-op).
• Ungraceful DLL_PROCESS_DETACH with no prior Shutdown: App::Destruct → DetachHooks() (commits, m_hooksAttached = false) → g_app.reset() → systems destroyed via unique_ptr, matching the original behavior.
• DLL_PROCESS_DETACH with aReserved != nullptr: skipped entirely per the existing guard in Main.cpp:39-44.

[wopss]

Thanks for the PR!

This PR introduce a use-after-free bug with the fix you're proposing.

To give a bit of background RED4ext is split into 2 life cycles:

1. Module (RED4ext) life cycle: App::Construct and App::Destroy. This stage attaches and detaches the hooks.
2. Game/Host life cycle: This stage initialize the core systems before the game starts by hooking game's Main function.

Currently, you're proposing to detach the hooks in the second life cycle, which is STILL using the hooks

RED4ext/src/dll/Hooks/QuickExit.cpp

Line 22 in c52c8d8

app->Shutdown();

and it still need to execute the original quick_exit function

RED4ext/src/dll/Hooks/QuickExit.cpp

Line 35 in c52c8d8

return _QuickExit(aExitCode);

If we are removing the hooks in this life cycle, then the memory region that can redirect from our hooked function to the real function will be freed.

Besides this, I do not see any problem with the current implementation, because:

1. if App::Destruct is called with aReserved != nullptr, then at that point we don't care about the clean up, and we shouldn't since the process is terminated and nothing is guaranteed.
2. if App::Destruct is called with aReserved == nullptr, which normally would not happen, but if it does then all mods are unloaded so no mods could call API functions, have game state updates registered, etc. Only problem would be a race condition that the game could still execute the a hooked function (not sure if Detours handle this). Again, this would not happen in a normal use case, because the user has to manually free the library using a debugger or something else that could decrease this module's ref counter, and that's on him.

[wopss]

Is this proposed fix an issue you/someone had?