From 33884dba30dd7764e0152400c50bd7acce9bd600 Mon Sep 17 00:00:00 2001 From: AnyOldName3 Date: Thu, 13 Jul 2023 00:13:20 +0100 Subject: [PATCH] Share the dump directory for crash and freeze dumps This means the shared memory struct is just 255 bytes longer than a few commits ago instead of 32K. Also introduce a function for putting path strings in the shared memory as there was too much copied and pasted code and it was error-prone. Also free some handles once we're done with them so they don't leak. Manual port of https://gitlab.com/OpenMW/openmw/-/merge_requests/3221 to OpenMW 0.48 --- components/CMakeLists.txt | 1 + .../crashcatcher/windows_crashcatcher.cpp | 60 +++++++++++-------- .../crashcatcher/windows_crashcatcher.hpp | 8 ++- .../crashcatcher/windows_crashmonitor.cpp | 5 +- components/crashcatcher/windows_crashshm.hpp | 6 +- .../windowscrashdumppathhelpers.cpp | 13 ++++ .../windowscrashdumppathhelpers.hpp | 13 ++++ components/debug/debugging.cpp | 6 +- 8 files changed, 75 insertions(+), 37 deletions(-) create mode 100644 components/crashcatcher/windowscrashdumppathhelpers.cpp create mode 100644 components/crashcatcher/windowscrashdumppathhelpers.hpp diff --git a/components/CMakeLists.txt b/components/CMakeLists.txt index 0b0a39ef926..324d5b410ff 100644 --- a/components/CMakeLists.txt +++ b/components/CMakeLists.txt @@ -284,6 +284,7 @@ if(WIN32) windows_crashcatcher windows_crashmonitor windows_crashshm + windowscrashdumppathhelpers ) elseif(NOT ANDROID) add_component_dir (crashcatcher diff --git a/components/crashcatcher/windows_crashcatcher.cpp b/components/crashcatcher/windows_crashcatcher.cpp index f39accabd23..d9de2da47ef 100644 --- a/components/crashcatcher/windows_crashcatcher.cpp +++ b/components/crashcatcher/windows_crashcatcher.cpp @@ -7,10 +7,23 @@ #include "windows_crashmonitor.hpp" #include "windows_crashshm.hpp" +#include "windowscrashdumppathhelpers.hpp" #include namespace Crash { + namespace + { + template + void writePathToShm(T(&buffer)[N], const std::string& path) + { + memset(buffer, 0, sizeof(buffer)); + size_t length = path.length(); + if (length >= sizeof(buffer)) + length = sizeof(buffer) - 1; + strncpy(buffer, path.c_str(), length); + } + } HANDLE duplicateHandle(HANDLE handle) { @@ -26,7 +39,7 @@ namespace Crash CrashCatcher* CrashCatcher::sInstance = nullptr; - CrashCatcher::CrashCatcher(int argc, char** argv, const std::string& crashDumpPath, const std::string& freezeDumpPath) + CrashCatcher::CrashCatcher(int argc, char** argv, const std::string& dumpPath, const std::string& crashDumpName, const std::string& freezeDumpName) { assert(sInstance == nullptr); // don't allow two instances @@ -48,7 +61,7 @@ namespace Crash if (!shmHandle) { setupIpc(); - startMonitorProcess(crashDumpPath, freezeDumpPath); + startMonitorProcess(dumpPath, crashDumpName, freezeDumpName); installHandler(); } else @@ -75,21 +88,21 @@ namespace Crash CloseHandle(mShmHandle); } - void CrashCatcher::updateDumpPaths(const std::string& crashDumpPath, const std::string& freezeDumpPath) + void CrashCatcher::updateDumpPath(const std::string& dumpPath) { shmLock(); - memset(mShm->mStartup.mCrashDumpFilePath, 0, sizeof(mShm->mStartup.mCrashDumpFilePath)); - size_t length = crashDumpPath.length(); - if (length >= MAX_LONG_PATH) length = MAX_LONG_PATH - 1; - strncpy(mShm->mStartup.mCrashDumpFilePath, crashDumpPath.c_str(), length); - mShm->mStartup.mCrashDumpFilePath[length] = '\0'; + writePathToShm(mShm->mStartup.mDumpDirectoryPath, dumpPath); + + shmUnlock(); + } + + void CrashCatcher::updateDumpNames(const std::string& crashDumpName, const std::string& freezeDumpName) + { + shmLock(); - memset(mShm->mStartup.mFreezeDumpFilePath, 0, sizeof(mShm->mStartup.mFreezeDumpFilePath)); - length = freezeDumpPath.length(); - if (length >= MAX_LONG_PATH) length = MAX_LONG_PATH - 1; - strncpy(mShm->mStartup.mFreezeDumpFilePath, freezeDumpPath.c_str(), length); - mShm->mStartup.mFreezeDumpFilePath[length] = '\0'; + writePathToShm(mShm->mStartup.mCrashDumpFileName, crashDumpName); + writePathToShm(mShm->mStartup.mFreezeDumpFileName, freezeDumpName); shmUnlock(); } @@ -143,7 +156,7 @@ namespace Crash SetUnhandledExceptionFilter(vectoredExceptionHandler); } - void CrashCatcher::startMonitorProcess(const std::string& crashDumpPath, const std::string& freezeDumpPath) + void CrashCatcher::startMonitorProcess(const std::string& dumpPath, const std::string& crashDumpName, const std::string& freezeDumpName) { std::wstring executablePath; DWORD copied = 0; @@ -153,17 +166,9 @@ namespace Crash } while (copied >= executablePath.size()); executablePath.resize(copied); - memset(mShm->mStartup.mCrashDumpFilePath, 0, sizeof(mShm->mStartup.mCrashDumpFilePath)); - size_t length = crashDumpPath.length(); - if (length >= MAX_LONG_PATH) length = MAX_LONG_PATH - 1; - strncpy(mShm->mStartup.mCrashDumpFilePath, crashDumpPath.c_str(), length); - mShm->mStartup.mCrashDumpFilePath[length] = '\0'; - - memset(mShm->mStartup.mFreezeDumpFilePath, 0, sizeof(mShm->mStartup.mFreezeDumpFilePath)); - length = freezeDumpPath.length(); - if (length >= MAX_LONG_PATH) length = MAX_LONG_PATH - 1; - strncpy(mShm->mStartup.mFreezeDumpFilePath, freezeDumpPath.c_str(), length); - mShm->mStartup.mFreezeDumpFilePath[length] = '\0'; + writePathToShm(mShm->mStartup.mDumpDirectoryPath, dumpPath); + writePathToShm(mShm->mStartup.mCrashDumpFileName, crashDumpName); + writePathToShm(mShm->mStartup.mFreezeDumpFileName, freezeDumpName); // note that we don't need to lock the SHM here, the other process has not started yet mShm->mEvent = CrashSHM::Event::Startup; @@ -186,6 +191,9 @@ namespace Crash if (!CreateProcessW(executablePath.data(), arguments.data(), NULL, NULL, TRUE, 0, NULL, NULL, &si, &pi)) throw std::runtime_error("Could not start crash monitor process"); + CloseHandle(pi.hProcess); + CloseHandle(pi.hThread); + waitMonitor(); } @@ -222,7 +230,7 @@ namespace Crash // must remain until monitor has finished waitMonitor(); - std::string message = "OpenMW has encountered a fatal error.\nCrash log saved to '" + std::string(mShm->mStartup.mCrashDumpFilePath) + "'.\nPlease report this to https://gitlab.com/OpenMW/openmw/issues !"; + std::string message = "OpenMW has encountered a fatal error.\nCrash log saved to '" + getCrashDumpPath(*mShm) + "'.\nPlease report this to https://gitlab.com/OpenMW/openmw/issues !"; SDL_ShowSimpleMessageBox(0, "Fatal Error", message.c_str(), nullptr); } diff --git a/components/crashcatcher/windows_crashcatcher.hpp b/components/crashcatcher/windows_crashcatcher.hpp index ea9f8e7ede9..87906f61af9 100644 --- a/components/crashcatcher/windows_crashcatcher.hpp +++ b/components/crashcatcher/windows_crashcatcher.hpp @@ -32,10 +32,12 @@ namespace Crash return sInstance; } - CrashCatcher(int argc, char** argv, const std::string& crashDumpPath, const std::string& freezeDumpPath); + CrashCatcher(int argc, char** argv, const std::string& dumpPath, const std::string& crashDumpName, const std::string& freezeDumpName); ~CrashCatcher(); - void updateDumpPaths(const std::string& crashDumpPath, const std::string& freezeDumpPath); + void updateDumpPath(const std::string& dumpPath); + + void updateDumpNames(const std::string& crashDumpName, const std::string& freezeDumpName); private: @@ -62,7 +64,7 @@ namespace Crash void shmUnlock(); - void startMonitorProcess(const std::string& crashDumpPath, const std::string& freezeDumpPath); + void startMonitorProcess(const std::string& dumpPath, const std::string& crashDumpName, const std::string& freezeDumpName); void waitMonitor(); diff --git a/components/crashcatcher/windows_crashmonitor.cpp b/components/crashcatcher/windows_crashmonitor.cpp index b46efdeda89..48cb38db5ad 100644 --- a/components/crashcatcher/windows_crashmonitor.cpp +++ b/components/crashcatcher/windows_crashmonitor.cpp @@ -12,6 +12,7 @@ #include "windows_crashcatcher.hpp" #include "windows_crashshm.hpp" +#include "windowscrashdumppathhelpers.hpp" #include namespace Crash @@ -207,7 +208,7 @@ namespace Crash { handleCrash(true); TerminateProcess(mAppProcessHandle, 0xDEAD); - std::string message = "OpenMW appears to have frozen.\nCrash log saved to '" + std::string(mShm->mStartup.mFreezeDumpFilePath) + "'.\nPlease report this to https://gitlab.com/OpenMW/openmw/issues !"; + std::string message = "OpenMW appears to have frozen.\nCrash log saved to '" + getFreezeDumpPath(*mShm) + "'.\nPlease report this to https://gitlab.com/OpenMW/openmw/issues !"; SDL_ShowSimpleMessageBox(0, "Fatal Error", message.c_str(), nullptr); } @@ -250,7 +251,7 @@ namespace Crash if (miniDumpWriteDump == NULL) return; - std::wstring utf16Path = utf8ToUtf16(isFreeze ? mShm->mStartup.mFreezeDumpFilePath : mShm->mStartup.mCrashDumpFilePath); + std::wstring utf16Path = utf8ToUtf16(isFreeze ? getFreezeDumpPath(*mShm) : getCrashDumpPath(*mShm)); if (utf16Path.empty()) return; diff --git a/components/crashcatcher/windows_crashshm.hpp b/components/crashcatcher/windows_crashshm.hpp index 1b3150fe847..b3ba38fb2af 100644 --- a/components/crashcatcher/windows_crashshm.hpp +++ b/components/crashcatcher/windows_crashshm.hpp @@ -8,6 +8,7 @@ namespace Crash // Used to communicate between the app and the monitor, fields are is overwritten with each event. static constexpr const int MAX_LONG_PATH = 0x7fff; + static constexpr const int MAX_FILENAME = 0xff; struct CrashSHM { @@ -28,8 +29,9 @@ namespace Crash HANDLE mSignalApp; HANDLE mSignalMonitor; HANDLE mShmMutex; - char mCrashDumpFilePath[MAX_LONG_PATH]; - char mFreezeDumpFilePath[MAX_LONG_PATH]; + char mDumpDirectoryPath[MAX_LONG_PATH]; + char mCrashDumpFileName[MAX_FILENAME]; + char mFreezeDumpFileName[MAX_FILENAME]; } mStartup; struct Crashed diff --git a/components/crashcatcher/windowscrashdumppathhelpers.cpp b/components/crashcatcher/windowscrashdumppathhelpers.cpp new file mode 100644 index 00000000000..f437313c2d1 --- /dev/null +++ b/components/crashcatcher/windowscrashdumppathhelpers.cpp @@ -0,0 +1,13 @@ +#include "windowscrashdumppathhelpers.hpp" + +#include + +std::string Crash::getCrashDumpPath(const CrashSHM& crashShm) +{ + return (boost::filesystem::path(crashShm.mStartup.mDumpDirectoryPath) / crashShm.mStartup.mCrashDumpFileName).string(); +} + +std::string Crash::getFreezeDumpPath(const CrashSHM& crashShm) +{ + return (boost::filesystem::path(crashShm.mStartup.mDumpDirectoryPath) / crashShm.mStartup.mFreezeDumpFileName).string(); +} diff --git a/components/crashcatcher/windowscrashdumppathhelpers.hpp b/components/crashcatcher/windowscrashdumppathhelpers.hpp new file mode 100644 index 00000000000..70afbf69821 --- /dev/null +++ b/components/crashcatcher/windowscrashdumppathhelpers.hpp @@ -0,0 +1,13 @@ +#ifndef COMPONENTS_CRASH_WINDOWSCRASHDUMPPATHHELPERS_H +#include "windows_crashshm.hpp" + +#include + +namespace Crash +{ + std::string getCrashDumpPath(const CrashSHM& crashShm); + + std::string getFreezeDumpPath(const CrashSHM& crashShm); +} + +#endif diff --git a/components/debug/debugging.cpp b/components/debug/debugging.cpp index 2409c79c58a..48a70f51dc5 100644 --- a/components/debug/debugging.cpp +++ b/components/debug/debugging.cpp @@ -300,10 +300,8 @@ void setupLogging(const std::string& logDir, std::string_view appName) #ifdef _WIN32 if (Crash::CrashCatcher::instance()) { - const std::string crashDumpName = Misc::StringUtils::lowerCase(appName) + "-crash.dmp"; - const std::string freezeDumpName = Misc::StringUtils::lowerCase(appName) + "-freeze.dmp"; boost::filesystem::path dumpDirectory(logDir); - Crash::CrashCatcher::instance()->updateDumpPaths((dumpDirectory / crashDumpName).make_preferred().string(), (dumpDirectory / freezeDumpName).make_preferred().string()); + Crash::CrashCatcher::instance()->updateDumpPath(dumpDirectory.make_preferred().string()); } #endif } @@ -335,7 +333,7 @@ int wrapApplication(int (*innerApplication)(int argc, char *argv[]), int argc, c dumpDirectory = userProfile; } CoTaskMemFree(userProfile); - Crash::CrashCatcher crashy(argc, argv, (dumpDirectory / crashDumpName).make_preferred().string(), (dumpDirectory / freezeDumpName).make_preferred().string()); + Crash::CrashCatcher crashy(argc, argv, dumpDirectory.make_preferred().string(), crashDumpName, freezeDumpName); #else const std::string crashLogName = Misc::StringUtils::lowerCase(appName) + "-crash.log"; // install the crash handler as soon as possible. note that the log path