fix symbols downloading logic (fixes #4254)

sumatrapdfreader · May 22, 2024 · a5c21d5 · a5c21d5
1 parent 41dc65a
commit a5c21d5
Show file tree

Hide file tree

Showing 4 changed files with 89 additions and 53 deletions.
diff --git a/src/CrashHandler.cpp b/src/CrashHandler.cpp
@@ -273,37 +273,31 @@ static bool DownloadAndUnzipSymbols(const char* symDir) {
     return ok;
 }
 
-bool CrashHandlerDownloadSymbols() {
-    log("CrashHandlerDownloadSymbols()\n");
-    if (!dir::CreateAll(gSymbolsDir)) {
-        log("CrashHandlerDownloadSymbols: couldn't create symbols dir\n");
-        return false;
-    }
-
+bool InitializeDbgHelp() {
     TempWStr ws = ToWStrTemp(gSymbolPath);
     if (!dbghelp::Initialize(ws, false)) {
-        log("CrashHandlerDownloadSymbols: dbghelp::Initialize() failed\n");
+        logf("InitializeDbgHelp: dbghelp::Initialize('%s') failed\n", gSymbolPath);
         return false;
     }
 
-    if (dbghelp::HasSymbols()) {
-        log("CrashHandlerDownloadSymbols(): skipping because dbghelp::HasSymbols()\n");
-        return true;
-    }
-
-    if (!DownloadAndUnzipSymbols(gSymbolsDir)) {
-        log("CrashHandlerDownloadSymbols: failed to download symbols\n");
+    if (!dbghelp::HasSymbols()) {
+        log("InitializeDbgHelp(): dbghelp::HasSymbols() failed\n");
         return false;
     }
+    log("InitializeDbgHelp(): did initialize ok\n");
+    return true;
+}
 
-    if (!dbghelp::Initialize(ws, true)) {
-        log("CrashHandlerDownloadSymbols: second dbghelp::Initialize() failed\n");
+bool CrashHandlerDownloadSymbols() {
+    log("CrashHandlerDownloadSymbols()\n");
+
+    if (!dir::CreateAll(gSymbolsDir)) {
+        logf("CrashHandlerDownloadSymbols: couldn't create symbols dir '%s'\n", gSymbolsDir);
         return false;
     }
 
-    if (!dbghelp::HasSymbols()) {
-        logf("CrashHandlerDownloadSymbols: HasSymbols() false after downloading symbols, gSymbolPath:'%s'\n",
-             gSymbolPath);
+    if (!DownloadAndUnzipSymbols(gSymbolsDir)) {
+        log("CrashHandlerDownloadSymbols: failed to download symbols\n");
         return false;
     }
     return true;
@@ -332,6 +326,7 @@ void _uploadDebugReport(const char* condStr, bool noCallstack) {
         // reports from official release
         return;
     }
+
     if (gIsDebugBuild) {
         // exclude debug builds. Those are most likely other people modyfing Sumatra
         return;
@@ -342,13 +337,17 @@ void _uploadDebugReport(const char* condStr, bool noCallstack) {
         return;
     }
 
-    logf("_uploadDebugReport: noCallstack: %d, gSymbolPathW: '%s'\n", (int)noCallstack, gSymbolPath);
+    logf("_uploadDebugReport: noCallstack: %d, gSymbolPath: '%s'\n", (int)noCallstack, gSymbolPath);
 
-    if (!noCallstack) {
-        bool ok = CrashHandlerDownloadSymbols();
+    bool needsSymbols = !noCallstack;
+    if (needsSymbols) {
+        // we proceed even if we fail to download symbols
+        bool ok = InitializeDbgHelp();
         if (!ok) {
-            log("_uploadDebugReport(): CrashHandlerDownloadSymbols() failed\n");
-            return;
+            ok = CrashHandlerDownloadSymbols();
+            if (ok) {
+                InitializeDbgHelp();
+            }
         }
     }
 
@@ -377,9 +376,15 @@ void TryUploadCrashReport() {
 
     logf("TryUploadCrashReport: gSymbolPathW: '%s'\n", gSymbolPath);
 
-    bool ok = CrashHandlerDownloadSymbols();
-    if (!ok) {
-        log("TryUploadCrashReport(): CrashHandlerDownloadSymbols() failed\n");
+    bool ok = InitializeDbgHelp();
+    if (ok) {
+        // we preceed even if we can't download symbols or initialize dbghelp
+        ok = CrashHandlerDownloadSymbols();
+        if (!ok) {
+            log("TryUploadCrashReport(): CrashHandlerDownloadSymbols() failed\n");
+        } else {
+            InitializeDbgHelp();
+        }
     }
 
     char* sv = BuildCrashInfoText(nullptr, true);
@@ -616,6 +621,8 @@ static void BuildSystemInfo() {
 http://p-nand-q.com/python/procmon.html
 https://learn.microsoft.com/en-us/windows-hardware/drivers/debugger/debugger-download-tools
 
+
+
 Setting symbol path:
 add GetEnvironmentVariableA("_NT_SYMBOL_PATH", ..., ...)
 add GetEnvironmentVariableA("_NT_ALT_SYMBOL_PATH ", ..., ...)
@@ -627,10 +634,25 @@ for symbols server symsrv.dll is needed, installed with debug tools for windows
 
 static bool gAddNtSymbolPath = true;
 static bool gAddSymbolServer = true;
+static bool gAddExeDir = true;
 
-static void BuildSymbolPath() {
+static void BuildSymbolPath(const char* symDir) {
     str::Str path(1024);
 
+    bool symDirExists = dir::Exists(symDir);
+
+    if (symDirExists) {
+        path.Append(symDir);
+        path.Append(";");
+    }
+
+    // in debug builds the symbols are in the same directory as .exe
+    if (gIsDebugBuild || gAddExeDir) {
+        TempStr dir = GetExeDirTemp();
+        path.Append(dir);
+        path.Append(";");
+    }
+
     if (gAddNtSymbolPath) {
         TempStr ntSymPath = GetEnvVariableTemp("_NT_SYMBOL_PATH");
         // internet talks about both _NT_ALT_SYMBOL_PATH and _NT_ALTERNATE_SYMBOL_PATH
@@ -645,28 +667,25 @@ static void BuildSymbolPath() {
             path.Append(";");
         }
     }
-    path.Append(gSymbolsDir);
-    if (gAddSymbolServer) {
-        // this probably typicall doesn't work as it needs symsrv.dll
-        path.AppendFmt("cache*%s;srv*http://msdl.microsoft.com/download/symbols;", gSymbolsDir);
+    if (gAddSymbolServer && symDirExists) {
+        // this probably won't work as it needs symsrv.dll and that's not included with Windows
+        // TODO: maybe try to scan system directories for symsrv.dll and somehow add it?
+        path.AppendFmt("cache*%s;srv*https://msdl.microsoft.com/download/symbols;", symDir);
     }
 
-#if 0
-    // when running local builds, *.pdb is in the same dir as *.exe
-    WCHAR* exePath = GetExePathTemp().Get();
-    path.Append(L";");
-    path.Append(exePath);
-#endif
+    // remove ";" from the end
+    path.RemoveLast();
 
-    str::ReplaceWithCopy(&gSymbolPath, path.Get());
+    char* p = path.CStr();
+    str::ReplaceWithCopy(&gSymbolPath, p);
 }
 
 bool SetSymbolsDir(const char* symDir) {
     if (!symDir) {
         return false;
     }
     str::ReplaceWithCopy(&gSymbolsDir, symDir);
-    BuildSymbolPath();
+    BuildSymbolPath(gSymbolsDir);
     return true;
 }
 

diff --git a/src/CrashHandler.h b/src/CrashHandler.h
@@ -7,4 +7,5 @@ extern char* gSymbolsDir;
 void InstallCrashHandler(const char* crashDumpPath, const char* crashFilePath, const char* symDir);
 void UninstallCrashHandler();
 bool CrashHandlerDownloadSymbols();
+bool InitializeDbgHelp();
 bool SetSymbolsDir(const char* symDir);
diff --git a/src/SumatraPDF.cpp b/src/SumatraPDF.cpp
@@ -140,7 +140,6 @@ static bool gDontSavePrefs = false;
 static void CloseDocumentInCurrentTab(MainWindow*, bool keepUIEnabled, bool deleteModel);
 static void OnSidebarSplitterMove(SplitterMoveEvent*);
 static void OnFavSplitterMove(SplitterMoveEvent*);
-static void DownloadDebugSymbols();
 
 LoadArgs::LoadArgs(const char* origPath, MainWindow* win) {
     this->fileArgs = ParseFileArgs(origPath);
@@ -4866,6 +4865,27 @@ void ClearHistory(MainWindow* win) {
     // TODO: deletion takes time so run it async with RunAsync()
 }
 
+static void DownloadDebugSymbols() {
+    TempStr msg = (TempStr) "Symbols were already downloaded";
+    bool ok = InitializeDbgHelp();
+    if (ok) {
+        goto ShowMessage;
+    }
+    ok = CrashHandlerDownloadSymbols();
+    if (!ok) {
+        msg = (TempStr) "Failed to download symbols";
+        goto ShowMessage;
+    }
+    msg = str::FormatTemp("Downloaded symbols to %s", gSymbolsDir);
+    if (ok) {
+        bool didInitializeDbgHelp = InitializeDbgHelp();
+        ReportIfQuick(!didInitializeDbgHelp);
+    }
+ShowMessage:
+    uint flags = MB_ICONINFORMATION | MB_OK | MbRtlReadingMaybe();
+    MessageBoxA(nullptr, msg, "Downloading symbols", flags);
+}
+
 // try to trigger a crash due to corrupting allocator
 // this is a different kind of a crash than just referencing invalid memory
 // as corrupted memory migh prevent crash handler from working
@@ -6149,16 +6169,6 @@ void ShowCrashHandlerMessage() {
     LaunchFileShell(url, nullptr, "open");
 }
 
-static void DownloadDebugSymbols() {
-    bool ok = CrashHandlerDownloadSymbols();
-    TempStr msg = (TempStr) "Failed to download symbols.";
-    if (ok) {
-        msg = str::FormatTemp("Downloaded symbols to %s", gSymbolsDir);
-    }
-    uint flags = MB_ICONINFORMATION | MB_OK | MbRtlReadingMaybe();
-    MessageBoxA(nullptr, msg, "Downloading symbols", flags);
-}
-
 void ShutdownCleanup() {
     gAllowedFileTypes.Reset();
     gAllowedLinkProtocols.Reset();

diff --git a/src/utils/DbgHelpDyn.cpp b/src/utils/DbgHelpDyn.cpp
@@ -149,9 +149,15 @@ NO_INLINE bool CanSymbolizeAddress(DWORD64 addr) {
 
     DWORD64 symDisp = 0;
     BOOL ok = DynSymFromAddr(GetCurrentProcess(), addr, &symDisp, symInfo);
+    if (!ok) {
+        return false;
+    }
     int symLen = symInfo->NameLen;
+    if (symLen < 4) {
+        return false;
+    }
     char* name = symInfo->Name;
-    return ok && symLen > 4 && (name[0] != 0);
+    return *name != 0;
 }
 
 // a heuristic to test if we have symbols for our own binaries by testing if