From 339dc2313bf8dcf54a07a252bdde4bf151a79dea Mon Sep 17 00:00:00 2001 From: Stenzek Date: Tue, 7 May 2024 01:42:59 +1000 Subject: [PATCH] CrashHandler: Use SetUnhandledExceptionFilter() and terminate on crash Fixes zombie processes sticking around. --- common/CrashHandler.cpp | 81 +++++++++------------------------------ common/CrashHandler.h | 5 +-- common/DynamicLibrary.cpp | 9 +++++ common/DynamicLibrary.h | 6 +++ 4 files changed, 35 insertions(+), 66 deletions(-) diff --git a/common/CrashHandler.cpp b/common/CrashHandler.cpp index 665dc23640..74b9930401 100644 --- a/common/CrashHandler.cpp +++ b/common/CrashHandler.cpp @@ -1,8 +1,9 @@ -// SPDX-FileCopyrightText: 2002-2023 PCSX2 Dev Team +// SPDX-FileCopyrightText: 2002-2024 PCSX2 Dev Team // SPDX-License-Identifier: LGPL-3.0+ #include "Pcsx2Defs.h" #include "CrashHandler.h" +#include "DynamicLibrary.h" #include "FileSystem.h" #include "StringUtil.h" #include @@ -75,8 +76,7 @@ static bool WriteMinidump(HMODULE hDbgHelp, HANDLE hFile, HANDLE hProcess, DWORD } static std::wstring s_write_directory; -static HMODULE s_dbghelp_module = nullptr; -static PVOID s_veh_handle = nullptr; +static DynamicLibrary s_dbghelp_module; static bool s_in_crash_handler = false; static void GenerateCrashFilename(wchar_t* buf, size_t len, const wchar_t* prefix, const wchar_t* extension) @@ -114,7 +114,7 @@ static void WriteMinidumpAndCallstack(PEXCEPTION_POINTERS exi) MiniDumpWithThreadInfo | MiniDumpWithIndirectlyReferencedMemory); const HANDLE hMinidumpFile = CreateFileW(filename, GENERIC_WRITE, 0, nullptr, CREATE_ALWAYS, 0, nullptr); if (hMinidumpFile == INVALID_HANDLE_VALUE || - !WriteMinidump(s_dbghelp_module, hMinidumpFile, GetCurrentProcess(), GetCurrentProcessId(), + !WriteMinidump(static_cast(s_dbghelp_module.GetHandle()), hMinidumpFile, GetCurrentProcess(), GetCurrentProcessId(), GetCurrentThreadId(), exi, minidump_type)) { static const char error_message[] = "Failed to write minidump file.\n"; @@ -136,32 +136,13 @@ static void WriteMinidumpAndCallstack(PEXCEPTION_POINTERS exi) static LONG NTAPI ExceptionHandler(PEXCEPTION_POINTERS exi) { - if (s_in_crash_handler) - return EXCEPTION_CONTINUE_SEARCH; + // if the debugger is attached, or we're recursively crashing, let it take care of it. + if (!s_in_crash_handler) + WriteMinidumpAndCallstack(exi); - switch (exi->ExceptionRecord->ExceptionCode) - { - case EXCEPTION_ACCESS_VIOLATION: - case EXCEPTION_BREAKPOINT: - case EXCEPTION_ARRAY_BOUNDS_EXCEEDED: - case EXCEPTION_INT_DIVIDE_BY_ZERO: - case EXCEPTION_INT_OVERFLOW: - case EXCEPTION_PRIV_INSTRUCTION: - case EXCEPTION_ILLEGAL_INSTRUCTION: - case EXCEPTION_NONCONTINUABLE_EXCEPTION: - case EXCEPTION_STACK_OVERFLOW: - case EXCEPTION_GUARD_PAGE: - break; - - default: - return EXCEPTION_CONTINUE_SEARCH; - } - - // if the debugger is attached, let it take care of it. - if (IsDebuggerPresent()) - return EXCEPTION_CONTINUE_SEARCH; - - WriteMinidumpAndCallstack(exi); + // returning EXCEPTION_CONTINUE_SEARCH makes sense, except for the fact that it seems to leave zombie processes + // around. instead, force ourselves to terminate. + TerminateProcess(GetCurrentProcess(), 0xFEFEFEFEu); return EXCEPTION_CONTINUE_SEARCH; } @@ -169,17 +150,16 @@ bool CrashHandler::Install() { // load dbghelp at install/startup, that way we're not LoadLibrary()'ing after a crash // .. because that probably wouldn't go down well. - s_dbghelp_module = StackWalker::LoadDbgHelpLibrary(); + HMODULE mod = StackWalker::LoadDbgHelpLibrary(); + if (mod) + s_dbghelp_module.Adopt(mod); - s_veh_handle = AddVectoredExceptionHandler(0, ExceptionHandler); - return (s_veh_handle != nullptr); + SetUnhandledExceptionFilter(ExceptionHandler); + return true; } -void CrashHandler::SetWriteDirectory(const std::string_view& dump_directory) +void CrashHandler::SetWriteDirectory(std::string_view dump_directory) { - if (!s_veh_handle) - return; - s_write_directory = FileSystem::GetWin32Path(dump_directory); } @@ -188,21 +168,6 @@ void CrashHandler::WriteDumpForCaller() WriteMinidumpAndCallstack(nullptr); } -void CrashHandler::Uninstall() -{ - if (s_veh_handle) - { - RemoveVectoredExceptionHandler(s_veh_handle); - s_veh_handle = nullptr; - } - - if (s_dbghelp_module) - { - FreeLibrary(s_dbghelp_module); - s_dbghelp_module = nullptr; - } -} - #elif defined(HAS_LIBBACKTRACE) #include "FileSystem.h" @@ -382,7 +347,7 @@ bool CrashHandler::Install() return true; } -void CrashHandler::SetWriteDirectory(const std::string_view& dump_directory) +void CrashHandler::SetWriteDirectory(std::string_view dump_directory) { } @@ -390,12 +355,6 @@ void CrashHandler::WriteDumpForCaller() { } -void CrashHandler::Uninstall() -{ - // We can't really unchain the signal handlers... so, YOLO. -} - - #else bool CrashHandler::Install() @@ -403,7 +362,7 @@ bool CrashHandler::Install() return false; } -void CrashHandler::SetWriteDirectory(const std::string_view& dump_directory) +void CrashHandler::SetWriteDirectory(std::string_view dump_directory) { } @@ -411,8 +370,4 @@ void CrashHandler::WriteDumpForCaller() { } -void CrashHandler::Uninstall() -{ -} - #endif \ No newline at end of file diff --git a/common/CrashHandler.h b/common/CrashHandler.h index f626da087c..333d612324 100644 --- a/common/CrashHandler.h +++ b/common/CrashHandler.h @@ -1,4 +1,4 @@ -// SPDX-FileCopyrightText: 2002-2023 PCSX2 Dev Team +// SPDX-FileCopyrightText: 2002-2024 PCSX2 Dev Team // SPDX-License-Identifier: LGPL-3.0+ #include @@ -6,7 +6,6 @@ namespace CrashHandler { bool Install(); - void SetWriteDirectory(const std::string_view& dump_directory); + void SetWriteDirectory(std::string_view dump_directory); void WriteDumpForCaller(); - void Uninstall(); } // namespace CrashHandler diff --git a/common/DynamicLibrary.cpp b/common/DynamicLibrary.cpp index 3a017b1d21..9f8c2229ff 100644 --- a/common/DynamicLibrary.cpp +++ b/common/DynamicLibrary.cpp @@ -100,6 +100,15 @@ bool DynamicLibrary::Open(const char* filename, Error* error) #endif } +void DynamicLibrary::Adopt(void* handle) +{ + pxAssertRel(handle, "Handle is valid"); + + Close(); + + m_handle = handle; +} + void DynamicLibrary::Close() { if (!IsOpen()) diff --git a/common/DynamicLibrary.h b/common/DynamicLibrary.h index 65082912ab..8767366f5b 100644 --- a/common/DynamicLibrary.h +++ b/common/DynamicLibrary.h @@ -45,6 +45,9 @@ public: /// Returns true if the library was loaded and can be used. bool Open(const char* filename, Error* error); + /// Adopts, or takes ownership of an existing opened library. + void Adopt(void* handle); + /// Unloads the library, any function pointers from this library are no longer valid. void Close(); @@ -61,6 +64,9 @@ public: return *ptr != nullptr; } + /// Returns the opaque OS-specific handle. + void* GetHandle() const { return m_handle; } + /// Move assignment, transfer ownership. DynamicLibrary& operator=(DynamicLibrary&& move);