From 1bdcb5d6d9e8fa2846749d31b9bb65102c26aa4d Mon Sep 17 00:00:00 2001 From: AnyOldName3 Date: Tue, 11 Jul 2023 22:22:26 +0100 Subject: [PATCH 1/4] 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. --- components/CMakeLists.txt | 1 + .../crashcatcher/windows_crashcatcher.cpp | 83 +++++++++---------- .../crashcatcher/windows_crashcatcher.hpp | 12 +-- .../crashcatcher/windows_crashmonitor.cpp | 7 +- components/crashcatcher/windows_crashshm.hpp | 6 +- .../windowscrashdumppathhelpers.cpp | 20 +++++ .../windowscrashdumppathhelpers.hpp | 13 +++ components/debug/debugging.cpp | 7 +- 8 files changed, 92 insertions(+), 57 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 7d96794a98..f64f41b198 100644 --- a/components/CMakeLists.txt +++ b/components/CMakeLists.txt @@ -333,6 +333,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 6236db19c3..a791290328 100644 --- a/components/crashcatcher/windows_crashcatcher.cpp +++ b/components/crashcatcher/windows_crashcatcher.cpp @@ -7,12 +7,28 @@ #include "windows_crashmonitor.hpp" #include "windows_crashshm.hpp" +#include "windowscrashdumppathhelpers.hpp" #include #include namespace Crash { + namespace + { + template + void writePathToShm(T(&buffer)[N], const std::filesystem::path& path) + { + memset(buffer, 0, sizeof(buffer)); + const auto str = path.u8string(); + size_t length = str.length(); + if (length >= sizeof(buffer)) + length = sizeof(buffer) - 1; + strncpy_s(buffer, sizeof(buffer), + Misc::StringUtils::u8StringToString(str).c_str(), length); + buffer[length] = '\0'; + } + } HANDLE duplicateHandle(HANDLE handle) { @@ -27,8 +43,8 @@ namespace Crash CrashCatcher* CrashCatcher::sInstance = nullptr; - CrashCatcher::CrashCatcher( - int argc, char** argv, const std::filesystem::path& crashDumpPath, const std::filesystem::path& freezeDumpPath) + CrashCatcher::CrashCatcher(int argc, char** argv, const std::filesystem::path& dumpPath, + const std::filesystem::path& crashDumpName, const std::filesystem::path& freezeDumpName) { assert(sInstance == nullptr); // don't allow two instances @@ -50,7 +66,7 @@ namespace Crash if (!shmHandle) { setupIpc(); - startMonitorProcess(crashDumpPath, freezeDumpPath); + startMonitorProcess(dumpPath, crashDumpName, freezeDumpName); installHandler(); } else @@ -77,28 +93,22 @@ namespace Crash CloseHandle(mShmHandle); } - void CrashCatcher::updateDumpPaths( - const std::filesystem::path& crashDumpPath, const std::filesystem::path& freezeDumpPath) + void CrashCatcher::updateDumpPath(const std::filesystem::path& dumpPath) + { + shmLock(); + + writePathToShm(mShm->mStartup.mDumpDirectoryPath, dumpPath); + + shmUnlock(); + } + + void CrashCatcher::updateDumpNames( + const std::filesystem::path& crashDumpName, const std::filesystem::path& freezeDumpName) { shmLock(); - memset(mShm->mStartup.mCrashDumpFilePath, 0, sizeof(mShm->mStartup.mCrashDumpFilePath)); - const auto str = crashDumpPath.u8string(); - size_t length = str.length(); - if (length >= MAX_LONG_PATH) - length = MAX_LONG_PATH - 1; - strncpy_s(mShm->mStartup.mCrashDumpFilePath, sizeof mShm->mStartup.mCrashDumpFilePath, - Misc::StringUtils::u8StringToString(str).c_str(), length); - mShm->mStartup.mCrashDumpFilePath[length] = '\0'; - - memset(mShm->mStartup.mFreezeDumpFilePath, 0, sizeof(mShm->mStartup.mFreezeDumpFilePath)); - const auto strFreeze = freezeDumpPath.u8string(); - length = strFreeze.length(); - if (length >= MAX_LONG_PATH) - length = MAX_LONG_PATH - 1; - strncpy_s(mShm->mStartup.mFreezeDumpFilePath, sizeof mShm->mStartup.mFreezeDumpFilePath, - Misc::StringUtils::u8StringToString(strFreeze).c_str(), length); - mShm->mStartup.mFreezeDumpFilePath[length] = '\0'; + writePathToShm(mShm->mStartup.mCrashDumpFileName, crashDumpName); + writePathToShm(mShm->mStartup.mFreezeDumpFileName, freezeDumpName); shmUnlock(); } @@ -153,8 +163,8 @@ namespace Crash SetUnhandledExceptionFilter(vectoredExceptionHandler); } - void CrashCatcher::startMonitorProcess( - const std::filesystem::path& crashDumpPath, const std::filesystem::path& freezeDumpPath) + void CrashCatcher::startMonitorProcess(const std::filesystem::path& dumpPath, + const std::filesystem::path& crashDumpName, const std::filesystem::path& freezeDumpName) { std::wstring executablePath; DWORD copied = 0; @@ -165,23 +175,9 @@ namespace Crash } while (copied >= executablePath.size()); executablePath.resize(copied); - memset(mShm->mStartup.mCrashDumpFilePath, 0, sizeof(mShm->mStartup.mCrashDumpFilePath)); - const auto str = crashDumpPath.u8string(); - size_t length = str.length(); - if (length >= MAX_LONG_PATH) - length = MAX_LONG_PATH - 1; - strncpy_s(mShm->mStartup.mCrashDumpFilePath, sizeof mShm->mStartup.mCrashDumpFilePath, - Misc::StringUtils::u8StringToString(str).c_str(), length); - mShm->mStartup.mCrashDumpFilePath[length] = '\0'; - - memset(mShm->mStartup.mFreezeDumpFilePath, 0, sizeof(mShm->mStartup.mFreezeDumpFilePath)); - const auto strFreeze = freezeDumpPath.u8string(); - length = strFreeze.length(); - if (length >= MAX_LONG_PATH) - length = MAX_LONG_PATH - 1; - strncpy_s(mShm->mStartup.mFreezeDumpFilePath, sizeof mShm->mStartup.mFreezeDumpFilePath, - Misc::StringUtils::u8StringToString(strFreeze).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; @@ -204,6 +200,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(); } @@ -241,7 +240,7 @@ namespace Crash waitMonitor(); std::string message = "OpenMW has encountered a fatal error.\nCrash log saved to '" - + std::string(mShm->mStartup.mCrashDumpFilePath) + + Misc::StringUtils::u8StringToString(getCrashDumpPath(*mShm).u8string()) + "'.\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 98c5a3bce6..385567545f 100644 --- a/components/crashcatcher/windows_crashcatcher.hpp +++ b/components/crashcatcher/windows_crashcatcher.hpp @@ -29,11 +29,13 @@ namespace Crash public: static CrashCatcher* instance() { return sInstance; } - CrashCatcher(int argc, char** argv, const std::filesystem::path& crashDumpPath, - const std::filesystem::path& freezeDumpPath); + CrashCatcher(int argc, char** argv, const std::filesystem::path& dumpPath, + const std::filesystem::path& crashDumpName, const std::filesystem::path& freezeDumpName); ~CrashCatcher(); - void updateDumpPaths(const std::filesystem::path& crashDumpPath, const std::filesystem::path& freezeDumpPath); + void updateDumpPath(const std::filesystem::path& dumpPath); + + void updateDumpNames(const std::filesystem::path& crashDumpName, const std::filesystem::path& freezeDumpName); private: static CrashCatcher* sInstance; @@ -59,8 +61,8 @@ namespace Crash void shmUnlock(); - void startMonitorProcess( - const std::filesystem::path& crashDumpPath, const std::filesystem::path& freezeDumpPath); + void startMonitorProcess(const std::filesystem::path& dumpPath, const std::filesystem::path& crashDumpName, + const std::filesystem::path& freezeDumpName); void waitMonitor(); diff --git a/components/crashcatcher/windows_crashmonitor.cpp b/components/crashcatcher/windows_crashmonitor.cpp index ea316c8a5c..0c268e1661 100644 --- a/components/crashcatcher/windows_crashmonitor.cpp +++ b/components/crashcatcher/windows_crashmonitor.cpp @@ -12,7 +12,9 @@ #include "windows_crashcatcher.hpp" #include "windows_crashshm.hpp" +#include "windowscrashdumppathhelpers.hpp" #include +#include namespace Crash { @@ -214,7 +216,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) + + Misc::StringUtils::u8StringToString(getFreezeDumpPath(*mShm).u8string()) + "'.\nPlease report this to https://gitlab.com/OpenMW/openmw/issues !"; SDL_ShowSimpleMessageBox(0, "Fatal Error", message.c_str(), nullptr); } @@ -256,8 +258,7 @@ namespace Crash if (miniDumpWriteDump == NULL) return; - std::wstring utf16Path - = utf8ToUtf16(isFreeze ? mShm->mStartup.mFreezeDumpFilePath : mShm->mStartup.mCrashDumpFilePath); + std::wstring utf16Path = (isFreeze ? getFreezeDumpPath(*mShm) : getCrashDumpPath(*mShm)).native(); if (utf16Path.empty()) return; diff --git a/components/crashcatcher/windows_crashshm.hpp b/components/crashcatcher/windows_crashshm.hpp index 1b3150fe84..b3ba38fb2a 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 0000000000..ecb6b38d86 --- /dev/null +++ b/components/crashcatcher/windowscrashdumppathhelpers.cpp @@ -0,0 +1,20 @@ +#include "windowscrashdumppathhelpers.hpp" + +#include + +namespace Crash +{ + std::filesystem::path getCrashDumpPath(const CrashSHM& crashShm) + { + return (std::filesystem::path(Misc::StringUtils::stringToU8String(crashShm.mStartup.mDumpDirectoryPath)) + / Misc::StringUtils::stringToU8String(crashShm.mStartup.mCrashDumpFileName)) + .make_preferred(); + } + + std::filesystem::path getFreezeDumpPath(const CrashSHM& crashShm) + { + return (std::filesystem::path(Misc::StringUtils::stringToU8String(crashShm.mStartup.mDumpDirectoryPath)) + / Misc::StringUtils::stringToU8String(crashShm.mStartup.mFreezeDumpFileName)) + .make_preferred(); + } +} diff --git a/components/crashcatcher/windowscrashdumppathhelpers.hpp b/components/crashcatcher/windowscrashdumppathhelpers.hpp new file mode 100644 index 0000000000..fa64969301 --- /dev/null +++ b/components/crashcatcher/windowscrashdumppathhelpers.hpp @@ -0,0 +1,13 @@ +#ifndef COMPONENTS_CRASH_WINDOWSCRASHDUMPPATHHELPERS_H +#include "windows_crashshm.hpp" + +#include + +namespace Crash +{ + std::filesystem::path getCrashDumpPath(const CrashSHM& crashShm); + + std::filesystem::path getFreezeDumpPath(const CrashSHM& crashShm); +} + +#endif diff --git a/components/debug/debugging.cpp b/components/debug/debugging.cpp index b4e433273a..0bec74ce73 100644 --- a/components/debug/debugging.cpp +++ b/components/debug/debugging.cpp @@ -318,10 +318,7 @@ void setupLogging(const std::filesystem::path& 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"; - std::filesystem::path dumpDirectory = logDir; - Crash::CrashCatcher::instance()->updateDumpPaths(dumpDirectory / crashDumpName, dumpDirectory / freezeDumpName); + Crash::CrashCatcher::instance()->updateDumpPath(logDir); } #endif } @@ -351,7 +348,7 @@ int wrapApplication(int (*innerApplication)(int argc, char* argv[]), int argc, c dumpDirectory = userProfile; } CoTaskMemFree(userProfile); - Crash::CrashCatcher crashy(argc, argv, dumpDirectory / crashDumpName, dumpDirectory / freezeDumpName); + Crash::CrashCatcher crashy(argc, argv, dumpDirectory, crashDumpName, freezeDumpName); #else const std::string crashLogName = Misc::StringUtils::lowerCase(appName) + "-crash.log"; // install the crash handler as soon as possible. From 677c17530e469df31fc5d1698f5a6a7b7621340e Mon Sep 17 00:00:00 2001 From: AnyOldName3 Date: Tue, 11 Jul 2023 22:58:44 +0100 Subject: [PATCH 2/4] I don't like reformatting things over and over. --- components/crashcatcher/windows_crashcatcher.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/components/crashcatcher/windows_crashcatcher.cpp b/components/crashcatcher/windows_crashcatcher.cpp index a791290328..254a36f0f0 100644 --- a/components/crashcatcher/windows_crashcatcher.cpp +++ b/components/crashcatcher/windows_crashcatcher.cpp @@ -17,15 +17,14 @@ namespace Crash namespace { template - void writePathToShm(T(&buffer)[N], const std::filesystem::path& path) + void writePathToShm(T (&buffer)[N], const std::filesystem::path& path) { memset(buffer, 0, sizeof(buffer)); const auto str = path.u8string(); size_t length = str.length(); if (length >= sizeof(buffer)) length = sizeof(buffer) - 1; - strncpy_s(buffer, sizeof(buffer), - Misc::StringUtils::u8StringToString(str).c_str(), length); + strncpy_s(buffer, sizeof(buffer), Misc::StringUtils::u8StringToString(str).c_str(), length); buffer[length] = '\0'; } } From 899f0a46337930e8dbe2a6054ebff7797cfd5b93 Mon Sep 17 00:00:00 2001 From: AnyOldName3 Date: Wed, 12 Jul 2023 14:46:32 +0100 Subject: [PATCH 3/4] Remove redundant explicit null terminator --- components/crashcatcher/windows_crashcatcher.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/components/crashcatcher/windows_crashcatcher.cpp b/components/crashcatcher/windows_crashcatcher.cpp index 254a36f0f0..23dfac549c 100644 --- a/components/crashcatcher/windows_crashcatcher.cpp +++ b/components/crashcatcher/windows_crashcatcher.cpp @@ -25,7 +25,6 @@ namespace Crash if (length >= sizeof(buffer)) length = sizeof(buffer) - 1; strncpy_s(buffer, sizeof(buffer), Misc::StringUtils::u8StringToString(str).c_str(), length); - buffer[length] = '\0'; } } From f239988c067657884bc6a48f7052118ade3c159a Mon Sep 17 00:00:00 2001 From: AnyOldName3 Date: Wed, 12 Jul 2023 14:48:09 +0100 Subject: [PATCH 4/4] Remove unused function --- components/crashcatcher/windows_crashmonitor.cpp | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/components/crashcatcher/windows_crashmonitor.cpp b/components/crashcatcher/windows_crashmonitor.cpp index 0c268e1661..dc76ce3456 100644 --- a/components/crashcatcher/windows_crashmonitor.cpp +++ b/components/crashcatcher/windows_crashmonitor.cpp @@ -228,18 +228,6 @@ namespace Crash signalApp(); } - static std::wstring utf8ToUtf16(const std::string& utf8) - { - const int nLenWide = MultiByteToWideChar(CP_UTF8, 0, utf8.c_str(), utf8.size(), nullptr, 0); - - std::wstring utf16; - utf16.resize(nLenWide); - if (MultiByteToWideChar(CP_UTF8, 0, utf8.c_str(), utf8.size(), utf16.data(), nLenWide) != nLenWide) - return {}; - - return utf16; - } - void CrashMonitor::handleCrash(bool isFreeze) { DWORD processId = GetProcessId(mAppProcessHandle);