fix(win): make symbolication and modulefinder independent of the system ANSI code page. (#1389)

* fix(win): make symbolication independent of the system ANSI code page.

* allow `NULL` module paths and symbol names.

* add an integration test that runs the example from a cyrillic directory and validates the package paths

* resolve relative paths + clean up subdir

* remove the assertion that a frame must have a function

* only assert on the frame_package being a file if it exists...

...however, no longer assert that a frame_package exists.

* isolate package assertions to new test

* don't conflate checking any function/package with checking package file validity

* also adapt the windows modulefinder to be independent system ACP.

The szExePath generated for actual UTF-8 paths was already filled with mojibake :-) so LoadLibrary couldn't find any local modules.

This is actually connected to the symbolication:

* in the server, if a module was found, the backend would assign packages to frames based on the instruction address and the module address range
* if the module couldn't be found, as was the case previously, it had to use the frame package provided

So, now we fixed both and they should overlap.

* update the CHANGELOG

* explicitly specify `PSAPI_VERSION` because we only want to link kernel32

* check string_from_wstr return values

* use a heap-allocated 32K buffer for module paths

* use a heap-allocated 32K buffer for symbol paths

* move allocation into wrapping if

* format after webui edit

Author: supervacuus

CHANGELOG.md

@@ -1,5 +1,11 @@
 # Changelog
 
+## Unreleased
+
+**Fixes**:
+
+- Windows: Make symbolication and the modulefinder independent of the system ANSI code page. ([#1389](https://github.com/getsentry/sentry-native/pull/1389))
+
 ## 0.11.1
 
 **Features**:

src/modulefinder/sentry_modulefinder_windows.c

@@ -6,17 +6,21 @@
 
 #ifndef SENTRY_PLATFORM_XBOX
 #    include <dbghelp.h>
-#else
-#    include <Psapi.h>
 #endif
-#include <tlhelp32.h>
+#define PSAPI_VERSION 2
+#include <Psapi.h>
+#include <TlHelp32.h>
 
 static bool g_initialized = false;
 static sentry_mutex_t g_mutex = SENTRY__MUTEX_INIT;
 static sentry_value_t g_modules = { 0 };
 
 #define CV_SIGNATURE 0x53445352
 
+// follow the maximum path length documented here:
+// https://learn.microsoft.com/en-us/windows/win32/fileio/maximum-file-path-limitation
+#define MAX_PATH_BUFFER_SIZE 32768
+
 struct CodeViewRecord70 {
     uint32_t signature;
     GUID pdb_signature;
@@ -92,6 +96,35 @@ extract_pdb_info(uintptr_t module_addr, sentry_value_t module)
     }
 }
 
+static void
+log_library_load_error(const wchar_t *module_filename_w)
+{
+    const DWORD ec = GetLastError();
+    LPWSTR msg_w = NULL;
+    FormatMessageW(FORMAT_MESSAGE_ALLOCATE_BUFFER | FORMAT_MESSAGE_FROM_SYSTEM
+            | FORMAT_MESSAGE_IGNORE_INSERTS,
+        NULL, ec, 0, (LPWSTR)&msg_w, 0, NULL);
+    char *msg = sentry__string_from_wstr(msg_w);
+    char *module_filename = sentry__string_from_wstr(module_filename_w);
+    if (!msg || !module_filename) {
+        if (module_filename) {
+            sentry_free(module_filename);
+        }
+        if (msg) {
+            sentry_free(msg);
+        }
+        if (msg_w) {
+            LocalFree(msg_w);
+        }
+        return;
+    }
+    SENTRY_ERRORF("LoadLibraryExW failed (%lu): %s (\"%s\")\n", ec,
+        msg ? msg : "(no message)", module_filename);
+    sentry_free(module_filename);
+    sentry_free(msg);
+    LocalFree(msg_w);
+}
+
 static void
 load_modules(void)
 {
@@ -101,14 +134,32 @@ load_modules(void)
     MODULEENTRY32W module = { 0 };
     module.dwSize = sizeof(MODULEENTRY32W);
     g_modules = sentry_value_new_list();
+    wchar_t *module_filename_w = NULL;
 
-    if (Module32FirstW(snapshot, &module)) {
+    if (Module32FirstW(snapshot, &module)
+        && (module_filename_w
+            = sentry_malloc(sizeof(wchar_t) * MAX_PATH_BUFFER_SIZE))) {
         do {
-            HMODULE handle = LoadLibraryExW(
-                module.szExePath, NULL, LOAD_LIBRARY_AS_DATAFILE);
+            HMODULE module_handle = NULL;
+            if (GetModuleFileNameExW(GetCurrentProcess(), module.hModule,
+                    module_filename_w, MAX_PATH_BUFFER_SIZE)) {
+                module_handle = LoadLibraryExW(
+                    module_filename_w, NULL, LOAD_LIBRARY_AS_DATAFILE);
+            } else {
+                char *module_name = sentry__string_from_wstr(module.szModule);
+                if (module_name) {
+                    SENTRY_ERRORF(
+                        "Failed to get module filename for %s", module_name);
+                    sentry_free(module_name);
+                }
+                continue;
+            }
+            if (!module_handle) {
+                log_library_load_error(module_filename_w);
+                continue;
+            }
             MEMORY_BASIC_INFORMATION vmem_info = { 0 };
-            if (handle
-                && sizeof(vmem_info)
+            if (sizeof(vmem_info)
                     == VirtualQuery(
                         module.modBaseAddr, &vmem_info, sizeof(vmem_info))
                 && vmem_info.State == MEM_COMMIT) {
@@ -120,12 +171,14 @@ load_modules(void)
                 sentry_value_set_by_key(rv, "image_size",
                     sentry_value_new_int32((int32_t)module.modBaseSize));
                 sentry_value_set_by_key(rv, "code_file",
-                    sentry__value_new_string_from_wstr(module.szExePath));
+                    sentry__value_new_string_from_wstr(module_filename_w));
                 extract_pdb_info((uintptr_t)module.modBaseAddr, rv);
                 sentry_value_append(g_modules, rv);
             }
-            FreeLibrary(handle);
+            FreeLibrary(module_handle);
         } while (Module32NextW(snapshot, &module));
+
+        sentry_free(module_filename_w);
     }
 
     CloseHandle(snapshot);

src/symbolizer/sentry_symbolizer_windows.c

@@ -1,12 +1,15 @@
 #include "sentry_boot.h"
+#include "sentry_string.h"
 
 #include "sentry_symbolizer.h"
 #include "sentry_windows_dbghelp.h"
 
 #include <dbghelp.h>
 #include <malloc.h>
 
-#define MAX_SYM 1024
+// follow the maximum path length documented here:
+// https://learn.microsoft.com/en-us/windows/win32/fileio/maximum-file-path-limitation
+#define MAX_PATH_BUFFER_SIZE 32768
 
 bool
 sentry__symbolize(
@@ -17,29 +20,46 @@ sentry__symbolize(
     (void)func;
     (void)addr;
 #else
+    if (!addr || !func) {
+        return false;
+    }
     HANDLE proc = sentry__init_dbghelp();
+    size_t symbol_info_size
+        = sizeof(SYMBOL_INFOW) + MAX_SYM_NAME * sizeof(WCHAR);
+    SYMBOL_INFOW *symbol_info = _alloca(symbol_info_size);
+    memset(symbol_info, 0, symbol_info_size);
+    symbol_info->MaxNameLen = MAX_SYM_NAME;
+    symbol_info->SizeOfStruct = sizeof(SYMBOL_INFOW);
 
-    SYMBOL_INFO *sym = (SYMBOL_INFO *)_alloca(sizeof(SYMBOL_INFO) + MAX_SYM);
-    memset(sym, 0, sizeof(SYMBOL_INFO) + MAX_SYM);
-    sym->MaxNameLen = MAX_SYM;
-    sym->SizeOfStruct = sizeof(SYMBOL_INFO);
+    if (!SymFromAddrW(proc, (uintptr_t)addr, NULL, symbol_info)) {
+        return false;
+    }
 
-    if (!SymFromAddr(proc, (DWORD64)addr, 0, sym)) {
+    wchar_t *mod_path_w = sentry_malloc(sizeof(wchar_t) * MAX_PATH_BUFFER_SIZE);
+    if (!mod_path_w) {
+        return false;
+    }
+    const DWORD n = GetModuleFileNameW((HMODULE)(uintptr_t)symbol_info->ModBase,
+        mod_path_w, MAX_PATH_BUFFER_SIZE);
+    if (n == 0 || n >= MAX_PATH_BUFFER_SIZE) {
+        sentry_free(mod_path_w);
         return false;
     }
 
-    char mod_name[MAX_PATH];
-    GetModuleFileNameA(
-        (HMODULE)(size_t)sym->ModBase, mod_name, sizeof(mod_name));
+    char *mod_path = sentry__string_from_wstr(mod_path_w);
+    char *symbol_name = sentry__string_from_wstr(symbol_info->Name);
 
-    sentry_frame_info_t frame_info;
-    memset(&frame_info, 0, sizeof(sentry_frame_info_t));
-    frame_info.load_addr = (void *)(size_t)sym->ModBase;
+    sentry_frame_info_t frame_info = { 0 };
+    frame_info.load_addr = (void *)(uintptr_t)symbol_info->ModBase;
     frame_info.instruction_addr = addr;
-    frame_info.symbol_addr = (void *)(size_t)sym->Address;
-    frame_info.symbol = sym->Name;
-    frame_info.object_name = mod_name;
+    frame_info.symbol_addr = (void *)(uintptr_t)symbol_info->Address;
+    frame_info.symbol = symbol_name;
+    frame_info.object_name = mod_path;
     func(&frame_info, data);
+
+    sentry_free(mod_path);
+    sentry_free(symbol_name);
+    sentry_free(mod_path_w);
 #endif // SENTRY_PLATFORM_XBOX
 
     return true;

tests/assertions.py

@@ -6,6 +6,7 @@
 import sys
 from dataclasses import dataclass
 from datetime import datetime, UTC
+from pathlib import Path
 
 import msgpack
 
@@ -167,7 +168,9 @@ def assert_event_meta(
         )
 
 
-def assert_stacktrace(envelope, inside_exception=False, check_size=True):
+def assert_stacktrace(
+    envelope, inside_exception=False, check_size=True, check_package=False
+):
     event = envelope.get_event()
 
     parent = event["exception"] if inside_exception else event["threads"]
@@ -182,6 +185,17 @@ def assert_stacktrace(envelope, inside_exception=False, check_size=True):
             for frame in frames
         )
 
+    if check_package:
+        for frame in frames:
+            frame_package = frame.get("package")
+            if frame_package is not None:
+                frame_package_path = Path(frame_package)
+                # only assert on absolute paths, since letting pathlib resolve relative paths is cheating
+                if frame_package_path.is_absolute():
+                    assert (
+                        frame_package_path.is_file()
+                    ), f"package is not a valid file path: '{frame_package}'"
+
 
 def assert_breadcrumb_inner(breadcrumbs, message="debug crumb"):
     expected = {

tests/test_integration_stdout.py

@@ -1,7 +1,9 @@
 import os
+import shutil
 import subprocess
 import sys
 import time
+from pathlib import Path
 
 import pytest
 
@@ -18,6 +20,7 @@
     assert_no_before_send,
     assert_crash_timestamp,
     assert_breakpad_crash,
+    assert_exception,
 )
 from .conditions import has_breakpad, has_files
 
@@ -46,6 +49,59 @@ def test_capture_stdout(cmake):
     assert_event(envelope)
 
 
+def copy_except(src: Path, dst: Path, exceptions: list[str] = None) -> None:
+    """
+    Recursively copy everything from src to dst, except for entries whose
+    names are in `exceptions`.
+    """
+    exceptions = set(exceptions or [])
+
+    dst.mkdir(parents=True, exist_ok=True)
+
+    for entry in src.iterdir():
+        if entry.name in exceptions:
+            continue
+
+        dest = dst / entry.name
+        if entry.is_dir():
+            shutil.copytree(entry, dest, symlinks=True)
+        else:
+            shutil.copy2(entry, dest)
+
+
+@pytest.mark.skipif(not has_files, reason="test needs a local filesystem")
+def test_capture_exception_from_utf8_path_stdout(cmake):
+    """
+    This test verifies that we can handle symbolication from an utf-8 path.
+    """
+    tmp_path = cmake(
+        ["sentry_example"],
+        {
+            "SENTRY_BACKEND": "none",
+            "SENTRY_TRANSPORT": "none",
+        },
+    )
+    # create a cyrillic subdirectory in tmp_path and copy tmp_path into it
+    cwd = tmp_path / "кириллица-тест"
+    cwd.mkdir()
+    copy_except(tmp_path, cwd, exceptions=["кириллица-тест"])
+
+    output = check_output(
+        cwd,
+        "sentry_example",
+        ["stdout", "capture-exception", "add-stacktrace"],
+    )
+    envelope = Envelope.deserialize(output)
+
+    assert_meta(envelope)
+    assert_breadcrumb(envelope)
+    assert_stacktrace(envelope, inside_exception=True, check_package=True)
+    assert_exception(envelope)
+
+    # delete the cyrillic directory, but only after we asserted on stack frame packages being files
+    shutil.rmtree(cwd)
+
+
 def test_dynamic_sdk_name_override(cmake):
     tmp_path = cmake(
         ["sentry_example"],