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

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

components/crashcatcher/windows_crashcatcher.cpp

@@ -7,10 +7,23 @@
 
 #include "windows_crashmonitor.hpp"
 #include "windows_crashshm.hpp"
+#include "windowscrashdumppathhelpers.hpp"
 #include <SDL_messagebox.h>
 
 namespace Crash
 {
+    namespace
+    {
+        template <class T, std::size_t N>
+        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);
     }
 

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

components/crashcatcher/windows_crashmonitor.cpp

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

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,13 @@
+#include "windowscrashdumppathhelpers.hpp"
+
+#include <boost/filesystem/path.hpp>
+
+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();
+}

components/crashcatcher/windowscrashdumppathhelpers.hpp

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

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