Skip to content

Commit fcce26d

Browse files
authored
Crash handler fixes (#4866)
#### Summary * Skip `OutputDebugStringA` when no debugger is attached. * Minimal teardown in `~CleanUpCrashHandler` to avoid touching global handler state late in process exit / DLL unload. * Reset the in-handler guards on early-return paths so they don't stay stuck after benign exceptions or stack-overflow recovery. * Drop unused exception-code constants from the header. #### Motivation Defensive cleanups around the crash-handling path. #### Test plan Launch / quit cycles on a local build; no regressions observed in normal exit, stack-overflow recovery, or benign-exception paths. #### Checklist * [x] Your code should follow the [coding guidelines](https://wiki.multitheftauto.com/index.php?title=Coding_guidelines). * [x] Smaller pull requests are easier to review. If your pull request is beefy, your pull request should be reviewable commit-by-commit.
1 parent 9e1f4b4 commit fcce26d

3 files changed

Lines changed: 28 additions & 64 deletions

File tree

Client/core/CCrashDumpWriter.cpp

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1479,6 +1479,7 @@ long WINAPI CCrashDumpWriter::HandleExceptionGlobal(_EXCEPTION_POINTERS* pExcept
14791479
{
14801480
std::array<char, DEBUG_BUFFER_SIZE> szDebug;
14811481
SAFE_DEBUG_PRINT_C(szDebug.data(), szDebug.size(), "CCrashDumpWriter: Non-fatal exception 0x%08X - ignoring\n", exceptionCode);
1482+
ms_bInCrashHandler.store(false, std::memory_order_release);
14821483
return EXCEPTION_CONTINUE_SEARCH;
14831484
}
14841485

@@ -1557,6 +1558,7 @@ long WINAPI CCrashDumpWriter::HandleExceptionGlobal(_EXCEPTION_POINTERS* pExcept
15571558
{
15581559
SAFE_DEBUG_OUTPUT("CCrashDumpWriter: Client handled exception - continuing execution\n");
15591560
delete pExceptionInformation;
1561+
ms_bInCrashHandler.store(false, std::memory_order_release);
15601562
return EXCEPTION_CONTINUE_SEARCH;
15611563
}
15621564

Client/core/CrashHandler.cpp

Lines changed: 24 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,15 @@ void OutputDebugStringSafeImpl(const char* message)
106106
if (message == nullptr)
107107
return;
108108

109+
// Skip OutputDebugStringA when no debugger is attached.
110+
//
111+
// OutputDebugStringA raises an exception under the hood so a debugger
112+
// can read the message; with no debugger attached the call is just
113+
// unnecessary work and an extra exception-dispatch site. Suppressing it
114+
// here is a robustness improvement on a path that runs frequently.
115+
if (!IsDebuggerPresent())
116+
return;
117+
109118
__try
110119
{
111120
OutputDebugStringA(message);
@@ -1173,11 +1182,16 @@ class CleanUpCrashHandler
11731182
CleanUpCrashHandler() = default;
11741183
~CleanUpCrashHandler()
11751184
{
1176-
std::scoped_lock lock{g_handlerStateMutex};
1185+
// Minimal teardown for process-exit/DLL-unload path.
1186+
// Avoid restoring detours/CRT handlers here: teardown order during
1187+
// shutdown is unstable, and touching patched global handler state
1188+
// late can itself fault. Keep SymCleanup for dbghelp resources, and
1189+
// avoid taking g_handlerStateMutex in this destructor.
11771190

11781191
g_bStackCookieCaptureEnabled.store(false, std::memory_order_release);
11791192
g_pfnCrashCallback.store(nullptr, std::memory_order_release);
11801193

1194+
// Release dbghelp symbol resources
11811195
if (g_symbolsInitialized.exchange(false, std::memory_order_acq_rel))
11821196
{
11831197
if (HANDLE hProcess = g_symbolProcess.exchange(nullptr, std::memory_order_acq_rel); hProcess != nullptr)
@@ -1187,44 +1201,14 @@ class CleanUpCrashHandler
11871201
}
11881202
}
11891203

1190-
if (auto terminateHandler = g_pfnOrigTerminate.exchange(nullptr, std::memory_order_acq_rel); terminateHandler != nullptr)
1191-
{
1192-
std::set_terminate(terminateHandler);
1193-
}
1194-
1195-
if (auto newHandler = g_pfnOrigNewHandler.exchange(nullptr, std::memory_order_acq_rel); newHandler != nullptr)
1196-
{
1197-
#if defined(_MSC_VER)
1198-
_set_new_handler(newHandler);
1199-
#else
1200-
std::set_new_handler(newHandler);
1201-
#endif
1202-
}
1203-
1204-
if (auto abortHandler = g_pfnOrigAbortHandler.exchange(nullptr, std::memory_order_acq_rel); abortHandler != nullptr)
1205-
{
1206-
signal(SIGABRT, abortHandler);
1207-
}
1208-
1209-
if (auto pureCallHandler = g_pfnOrigPureCall.exchange(nullptr, std::memory_order_acq_rel); pureCallHandler != nullptr)
1210-
{
1211-
_set_purecall_handler(pureCallHandler);
1212-
}
1213-
1214-
if (auto previousFilter = g_pfnOrigFilt.exchange(nullptr, std::memory_order_acq_rel); previousFilter != nullptr)
1215-
{
1216-
SetUnhandledExceptionFilter(previousFilter);
1217-
}
1218-
1219-
if (auto kernelSetUnhandled = g_pfnKernelSetUnhandledExceptionFilter.exchange(nullptr, std::memory_order_acq_rel); kernelSetUnhandled != nullptr)
1220-
{
1221-
g_kernelSetUnhandledExceptionFilterTrampoline = kernelSetUnhandled;
1222-
if (!SharedUtil::UndoFunctionDetour(g_kernelSetUnhandledExceptionFilterTrampoline, reinterpret_cast<void*>(RedirectedSetUnhandledExceptionFilter)))
1223-
{
1224-
SafeDebugOutput("CrashHandler: WARNING - Failed to undo SEH detour during cleanup\n");
1225-
}
1226-
g_kernelSetUnhandledExceptionFilterTrampoline = nullptr;
1227-
}
1204+
// Clear cached state without restoration in teardown path
1205+
g_pfnOrigTerminate.store(nullptr, std::memory_order_release);
1206+
g_pfnOrigNewHandler.store(nullptr, std::memory_order_release);
1207+
g_pfnOrigAbortHandler.store(nullptr, std::memory_order_release);
1208+
g_pfnOrigPureCall.store(nullptr, std::memory_order_release);
1209+
g_pfnOrigFilt.store(nullptr, std::memory_order_release);
1210+
g_pfnKernelSetUnhandledExceptionFilter.store(nullptr, std::memory_order_release);
1211+
g_kernelSetUnhandledExceptionFilterTrampoline = nullptr;
12281212
}
12291213
};
12301214

@@ -2818,6 +2802,7 @@ LONG __stdcall CrashHandlerExceptionFilter(EXCEPTION_POINTERS* pExPtrs)
28182802
SafeDebugOutput("SEH: FATAL - Stack overflow detected - _resetstkoflw() failed\n");
28192803
// If we can't reset the stack, we can't safely do anything else.
28202804
// Attempting to log or call handlers will likely trigger another overflow immediately.
2805+
bHandlerActive.store(false, std::memory_order_release);
28212806
return EXCEPTION_CONTINUE_SEARCH;
28222807
}
28232808
}

Client/core/CrashHandler.h

Lines changed: 2 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -44,14 +44,8 @@ constexpr DWORD CPP_EXCEPTION_CODE = 0xE06D7363;
4444
constexpr DWORD STATUS_INVALID_CRUNTIME_PARAMETER_CODE = 0xC0000417;
4545
constexpr DWORD STATUS_STACK_BUFFER_OVERRUN_CODE = 0xC0000409;
4646
constexpr DWORD STATUS_HEAP_CORRUPTION_CODE = 0xC0000374;
47-
// STATUS_FATAL_USER_CALLBACK_EXCEPTION (0xC000041D):
48-
// This exception type occurs when an exception happens inside a Windows system callback
49-
// (e.g., window procedures, DLL callbacks, kernel-to-user transitions) and cannot be
50-
// properly unwound. Special handling is required as:
51-
// - The stack may be corrupted or incomplete
52-
// - Context may not have full information
53-
// - Stack walking may fail or cause secondary exceptions
54-
// - Module resolution may point to trampolines instead of actual code
47+
// STATUS_FATAL_USER_CALLBACK_EXCEPTION (0xC000041D) can occur in Windows callback
48+
// transitions with unreliable unwind/context data, so crash handling treats it as fatal.
5549
constexpr DWORD STATUS_FATAL_USER_CALLBACK_EXCEPTION = 0xC000041D;
5650
constexpr DWORD CUSTOM_EXCEPTION_CODE_WATCHDOG_TIMEOUT = 0xE0000001;
5751

@@ -63,23 +57,6 @@ constexpr int EXIT_CODE_CRASH = 3;
6357

6458
constexpr std::size_t DEBUG_BUFFER_SIZE_LARGE = 512;
6559

66-
constexpr DWORD EXCEPTION_BREAKPOINT_NONFATAL = 0x80000003;
67-
constexpr DWORD EXCEPTION_SINGLE_STEP_NONFATAL = 0x80000004;
68-
constexpr DWORD EXCEPTION_GUARD_PAGE_NONFATAL = 0x80000001;
69-
constexpr DWORD EXCEPTION_INVALID_HANDLE_NONFATAL = 0xC0000008;
70-
constexpr DWORD EXCEPTION_ARRAY_BOUNDS_EXCEEDED_NONFATAL = 0xC000008C;
71-
constexpr DWORD EXCEPTION_FLT_DENORMAL_OPERAND_NONFATAL = 0xC000008D;
72-
constexpr DWORD EXCEPTION_FLT_DIVIDE_BY_ZERO_NONFATAL = 0xC000008E;
73-
constexpr DWORD EXCEPTION_FLT_INEXACT_RESULT_NONFATAL = 0xC000008F;
74-
constexpr DWORD EXCEPTION_FLT_INVALID_OPERATION_NONFATAL = 0xC0000090;
75-
constexpr DWORD EXCEPTION_FLT_OVERFLOW_NONFATAL = 0xC0000091;
76-
constexpr DWORD EXCEPTION_FLT_STACK_CHECK_NONFATAL = 0xC0000092;
77-
constexpr DWORD EXCEPTION_FLT_UNDERFLOW_NONFATAL = 0xC0000093;
78-
constexpr DWORD EXCEPTION_INT_DIVIDE_BY_ZERO_NONFATAL = 0xC0000094;
79-
constexpr DWORD EXCEPTION_INT_OVERFLOW_NONFATAL = 0xC0000095;
80-
constexpr DWORD EXCEPTION_PRIV_INSTRUCTION_NONFATAL = 0xC0000096;
81-
constexpr DWORD EXCEPTION_NONCONTINUABLE_EXCEPTION_NONFATAL = 0xC0000025;
82-
8360
inline constexpr std::string_view DEBUG_PREFIX_CRASH = "CrashHandler: ";
8461
inline constexpr std::string_view DEBUG_PREFIX_SEH = "SEH: ";
8562
inline constexpr std::string_view DEBUG_PREFIX_CPP = "C++ TERMINATE: ";

0 commit comments

Comments
 (0)