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

@@ -333,6 +333,7 @@ if(WIN32)
         windows_crashcatcher
         windows_crashmonitor
         windows_crashshm
+        windowscrashdumppathhelpers
     )
 elseif(NOT ANDROID)
     add_component_dir (crashcatcher

components/crashcatcher/windows_crashcatcher.cpp

@@ -7,12 +7,28 @@
 
 #include "windows_crashmonitor.hpp"
 #include "windows_crashshm.hpp"
+#include "windowscrashdumppathhelpers.hpp"
 #include <SDL_messagebox.h>
 
 #include <components/misc/strings/conversion.hpp>
 
 namespace Crash
 {
+    namespace
+    {
+        template <class T, std::size_t N>
+        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);
     }

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();
 

components/crashcatcher/windows_crashmonitor.cpp

@@ -12,7 +12,9 @@
 
 #include "windows_crashcatcher.hpp"
 #include "windows_crashshm.hpp"
+#include "windowscrashdumppathhelpers.hpp"
 #include <components/debug/debuglog.hpp>
+#include <components/misc/strings/conversion.hpp>
 
 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;
 

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

components/crashcatcher/windowscrashdumppathhelpers.cpp

@@ -0,0 +1,20 @@
+#include "windowscrashdumppathhelpers.hpp"
+
+#include <components/misc/strings/conversion.hpp>
+
+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();
+    }
+}

components/crashcatcher/windowscrashdumppathhelpers.hpp

@@ -0,0 +1,13 @@
+#ifndef COMPONENTS_CRASH_WINDOWSCRASHDUMPPATHHELPERS_H
+#include "windows_crashshm.hpp"
+
+#include <filesystem>
+
+namespace Crash
+{
+    std::filesystem::path getCrashDumpPath(const CrashSHM& crashShm);
+
+    std::filesystem::path getFreezeDumpPath(const CrashSHM& crashShm);
+}
+
+#endif

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.