fix: five critical bugs surfaced by team review (targets v0.1.21)

[JustinMDotNet]

What

Five bug fixes surfaced by the earlier 5-teammate code review. Does not tag a release. v0.1.21 tag is a separate step after merge.

The 5 fixes

#	File:lines	Severity	What was wrong	Fix
1	Hooks/UpdateNavPath.cpp:33-37	🔴 High	targetType is unsigned __int8 (1 byte) passed via pointer to CStackType("Int32", ...) — Redscript VM read 3 bytes of stack garbage in the upper bits	Widen to a local int32_t first, pass its address
2	InWorldNavigation.reds:UpdateNavPath	🔴 High	Empty points array → i = ArraySize - 1 = -1, then points[i] OOB	Guard the populate-dots block with if i >= 0; cleanup loops below still run and correctly kill stale FX
3	InWorldNavigation.reds (5 sites)	🔴 High	fx.BreakLoop() called without IsDefined(fx) guard — crash when engine has already killed/expired the instance	Wrap each call in if IsDefined(fx). Five sites (the original four + the cleanup loop in UpdateNavPath that GPT's review flagged as also vulnerable)
4	InWorldNavigation.{hpp,cpp} + Main.cpp	🟠 Med	File-scope Handle<InWorldNavigation>'s ~Handle() calls DecRef → InterlockedExchangeAdd into a RefCnt block in possibly-already-freed game memory at DLL unload. Suspected root cause of historical exit-crashes jackhumbert#27/jackhumbert#35/jackhumbert#43.	Add static void Shutdown() called from Main Unload. Bypasses DecRef entirely by zeroing the handle's pointers; SharedPtrBase::DecRef() short-circuits on null refCount at later automatic destruction. Order: detach hooks FIRST (ModModuleFactory::Unload), then Shutdown(), so no game thread can re-arm the singleton via GetInstance() between the two. Trade-off: leaks ~16 bytes; OS reclaims at process exit. CanBeDestructed() returns false so no destructor path is bypassed in any meaningful way.
5	Main.cpp:114-130	🟡 Low	std::filesystem::remove_all unguarded in extern "C" Main — throw → std::terminate kills the game	try / catch (const std::exception& e) / catch (...) per claude-opus-4.7-high's recommendation (covers std::bad_alloc and other surprises from path internals, not just filesystem_error)

Triple review (per memory policy for memory-safety + cross-process boundary)

Reviewer	Verdict	Key finding	Addressed
claude-opus-4.8	SHIP-with-fixes	Fix #4's timing assumption (game memory alive at Unload) is unverifiable statically; also contradicts pre-existing Main.cpp:144 comment	✅ Switched Shutdown to bypass DecRef entirely (zero pointers directly), sidestepping the timing question. Reconciled the contradictory comment.
gpt-5.3-codex	SHIP-with-fixes	Cleanup loop at lines 277-280 also calls BreakLoop/SetBlackboardValue/Kill without IsDefined — same crash class as Fix #3	✅ Wrapped in if IsDefined
claude-opus-4.7-high	SHIP-with-fixes	Main.cpp Unload order was wrong — Shutdown() before ModModuleFactory::Unload() left a re-init race window via UpdateNavPath hook → GetInstance() → fresh allocation that would then UAF at DLL unload	✅ Swapped order: hooks detached first, then Shutdown

All three reviewers' blockers + concerns are addressed.

Local build

73/73 ninja targets clean. MSVC 14.51.36231 / Windows 11 SDK 10.0.28000.0 / VS-bundled CMake 4.2.3 / bundled Ninja 1.11.1. Both in_world_navigation.dll and mod_settings.dll linked successfully.

What still requires runtime verification

The static reasoning for Fix #4 is internally consistent (the new Shutdown bypasses DecRef entirely, so we no longer touch RefCnt regardless of whether it's alive). The only way to confirm the historical exit crashes are gone is to play with the modded DLL installed and exit the game repeatedly. The PR doesn't claim "this fixes jackhumbert#27/jackhumbert#35/jackhumbert#43"; it claims "this removes the documented UAF mechanism."

Out of scope (deliberately not in this PR)

• Pre-existing race in GetInstance() lazy init — no synchronization; multiple threads can simultaneously observe handle.instance == nullptr and both attempt to create-and-assign. Noted by claude-opus-4.7-high as concern fix: five critical bugs surfaced by team review (targets v0.1.21) #6. Not introduced by this PR; would benefit from a separate fix using std::call_once or an atomic CAS.

After merge

Tagging v0.1.21 (separate git tag v0.1.21 && git push --tags) would fire release.yaml and publish the GitHub Release.

[Copilot]

Pull request overview

Bundles five defensive bug fixes surfaced by a prior team code review: a stack-width fix in the C++ hook trampoline, an empty-array guard and IsDefined guards in the Redscript path, a safe DLL-unload teardown for the singleton handle to avoid an exit-time UAF, and exception guards around filesystem::remove_all in Main.

Changes:

• Widen targetType to int32_t before passing its address to CStackType("Int32", …) so the Redscript VM no longer reads stack garbage.
• Add if i >= 0 guard around the dots-population block in UpdateNavPath and wrap all five FxInstance access sites (Update × 2, Stop × 2, and the cleanup loop) in IsDefined.
• Introduce InWorldNavigation::Shutdown() that bypasses ~Handle()/DecRef by zeroing pointers, invoked from Main Unload after hooks are detached; also wrap remove_all calls in try/catch.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/red4ext/Hooks/UpdateNavPath.cpp	Widens 1-byte targetType enum into a local int32_t before passing its address to CStackType("Int32", …).
src/red4ext/InWorldNavigation.hpp	Declares static void Shutdown() with rationale for bypassing ~Handle() at DLL unload.
src/red4ext/InWorldNavigation.cpp	Implements Shutdown() by zeroing the file-scope handle's instance and refCount.
src/red4ext/Main.cpp	Wraps remove_all in try/catch; adds InWorldNavigation::Shutdown() after ModModuleFactory::Unload in the Unload handler.
src/redscript/in_world_navigation/InWorldNavigation.reds	Adds if i >= 0 guard around dots population and IsDefined(fx) guards around five BreakLoop/Kill sites.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[JustinMDotNet]

Fixed in 1415f96. Pure comment-only correction; no code change.