build: remove obsolete CRASHPAD_WER_ENABLED (#950)

* build: remove obsolete CRASHPAD_WER_ENABLED we initially introduced the build flag because of a potentially missing struct member if an older target version than 19041 was selected in a build: getsentry/crashpad#70 But upstream fixed that with a local compatibility struct a while ago: https://chromium.googlesource.com/crashpad/crashpad/+/ca928c8d6b651b7123f1a5cad36dba08ca2416bc That means we can get rid of the build-flag entirely, because this will build on all supported platforms and add a runtime version check for `build == 19041`, so that we don't even register the module (including not polluting the registry) on WER versions that don't support fast-fail crashes .
getsentry · Feb 14, 2024 · 5596cbb · 5596cbb
1 parent 9ea6090
commit 5596cbb
Show file tree

Hide file tree

Showing 7 changed files with 157 additions and 99 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,5 +1,11 @@
 # Changelog
 
+## Unreleased
+
+**Internal**:
+
+- Remove the `CRASHPAD_WER_ENABLED` build flag. The WER module is now built for all supported Windows targets, and registration is conditional on runtime Windows version checks. ([#950](https://github.com/getsentry/sentry-native/pull/950), [crashpad#96](https://github.com/getsentry/crashpad/pull/96))
+
 ## 0.7.0
 
 **Breaking changes**:

diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -423,7 +423,7 @@ if(SENTRY_BACKEND_CRASHPAD)
 	endif()
 	add_subdirectory(external/crashpad crashpad_build)
 
-	if(CRASHPAD_WER_ENABLED)
+	if(WIN32)
 		add_dependencies(sentry crashpad::wer)
 	endif()
 
@@ -438,9 +438,7 @@ if(SENTRY_BACKEND_CRASHPAD)
 		set_property(TARGET crashpad_snapshot PROPERTY MSVC_RUNTIME_LIBRARY "MultiThreaded$<$<CONFIG:Debug>:Debug>")
 		set_property(TARGET crashpad_tools PROPERTY MSVC_RUNTIME_LIBRARY "MultiThreaded$<$<CONFIG:Debug>:Debug>")
 		set_property(TARGET crashpad_util PROPERTY MSVC_RUNTIME_LIBRARY "MultiThreaded$<$<CONFIG:Debug>:Debug>")
-		if(CRASHPAD_WER_ENABLED)
-			set_property(TARGET crashpad_wer PROPERTY MSVC_RUNTIME_LIBRARY "MultiThreaded$<$<CONFIG:Debug>:Debug>")
-		endif()
+		set_property(TARGET crashpad_wer PROPERTY MSVC_RUNTIME_LIBRARY "MultiThreaded$<$<CONFIG:Debug>:Debug>")
 		set_property(TARGET crashpad_zlib PROPERTY MSVC_RUNTIME_LIBRARY "MultiThreaded$<$<CONFIG:Debug>:Debug>")
 		set_property(TARGET mini_chromium PROPERTY MSVC_RUNTIME_LIBRARY "MultiThreaded$<$<CONFIG:Debug>:Debug>")
 	endif()
@@ -457,9 +455,7 @@ if(SENTRY_BACKEND_CRASHPAD)
 		set_target_properties(crashpad_util PROPERTIES FOLDER ${SENTRY_FOLDER})
 		set_target_properties(crashpad_zlib PROPERTIES FOLDER ${SENTRY_FOLDER})
 		set_target_properties(mini_chromium PROPERTIES FOLDER ${SENTRY_FOLDER})
-		if(CRASHPAD_WER_ENABLED)
-			set_target_properties(crashpad_wer PROPERTIES FOLDER ${SENTRY_FOLDER})
-		endif()
+		set_target_properties(crashpad_wer PROPERTIES FOLDER ${SENTRY_FOLDER})
 	endif()
 
 	target_link_libraries(sentry PRIVATE
@@ -472,16 +468,10 @@ if(SENTRY_BACKEND_CRASHPAD)
 	if(WIN32 AND MSVC)
 		sentry_install(FILES $<TARGET_PDB_FILE:crashpad_handler>
 			DESTINATION "${CMAKE_INSTALL_BINDIR}" OPTIONAL)
-		if (CRASHPAD_WER_ENABLED)
-			sentry_install(FILES $<TARGET_PDB_FILE:crashpad_wer>
-				DESTINATION "${CMAKE_INSTALL_BINDIR}" OPTIONAL)
-		endif()
+		sentry_install(FILES $<TARGET_PDB_FILE:crashpad_wer>
+			DESTINATION "${CMAKE_INSTALL_BINDIR}" OPTIONAL)
 	endif()
 	add_dependencies(sentry crashpad::handler)
-
-	if(CRASHPAD_WER_ENABLED)
-		add_compile_definitions(CRASHPAD_WER_ENABLED)
-	endif()
 elseif(SENTRY_BACKEND_BREAKPAD)
 	option(SENTRY_BREAKPAD_SYSTEM "Use system breakpad" OFF)
 	if(SENTRY_BREAKPAD_SYSTEM)
@@ -575,10 +565,9 @@ if(SENTRY_BUILD_EXAMPLES)
 
 	if(MSVC)
 		target_compile_options(sentry_example PRIVATE $<BUILD_INTERFACE:/wd5105>)
-		if(CRASHPAD_WER_ENABLED)
-			# to test handling SEH by-passing exceptions we need to enable the control flow guard
-			target_compile_options(sentry_example PRIVATE $<BUILD_INTERFACE:/guard:cf>)
-		endif()
+
+		# to test handling SEH by-passing exceptions we need to enable the control flow guard
+		target_compile_options(sentry_example PRIVATE $<BUILD_INTERFACE:/guard:cf>)
 	endif()
 
 	# set static runtime if enabled

diff --git a/examples/example.c b/examples/example.c
@@ -93,7 +93,9 @@ has_arg(int argc, char **argv, const char *arg)
     return false;
 }
 
-#ifdef CRASHPAD_WER_ENABLED
+#if defined(SENTRY_PLATFORM_WINDOWS) && !defined(__MINGW32__)                  \
+    && !defined(__MINGW64__)
+
 int
 call_rffe_many_times()
 {
@@ -138,7 +140,7 @@ trigger_fastfail_crash()
     __fastfail(77);
 }
 
-#endif // CRASHPAD_WER_ENABLED
+#endif
 
 #ifdef SENTRY_PLATFORM_AIX
 // AIX has a null page mapped to the bottom of memory, which means null derefs
@@ -301,7 +303,8 @@ main(int argc, char **argv)
     if (has_arg(argc, argv, "crash")) {
         trigger_crash();
     }
-#ifdef CRASHPAD_WER_ENABLED
+#if defined(SENTRY_PLATFORM_WINDOWS) && !defined(__MINGW32__)                  \
+    && !defined(__MINGW64__)
     if (has_arg(argc, argv, "fastfail")) {
         trigger_fastfail_crash();
     }

diff --git a/external/crashpad b/external/crashpad
diff --git a/src/backends/sentry_backend_crashpad.cpp b/src/backends/sentry_backend_crashpad.cpp
@@ -7,10 +7,15 @@ extern "C" {
 #include "sentry_database.h"
 #include "sentry_envelope.h"
 #include "sentry_options.h"
+#ifdef SENTRY_PLATFORM_WINDOWS
+#    include "sentry_os.h"
+#endif
 #include "sentry_path.h"
 #include "sentry_sync.h"
 #include "sentry_transport.h"
-#include "sentry_unix_pageallocator.h"
+#ifdef SENTRY_PLATFORM_LINUX
+#    include "sentry_unix_pageallocator.h"
+#endif
 #include "sentry_utils.h"
 #include "transports/sentry_disk_transport.h"
 }
@@ -110,6 +115,51 @@ crashpad_backend_user_consent_changed(sentry_backend_t *backend)
     data->db->GetSettings()->SetUploadsEnabled(!sentry__should_skip_upload());
 }
 
+#ifdef SENTRY_PLATFORM_WINDOWS
+static void
+crashpad_register_wer_module(
+    const sentry_path_t *absolute_handler_path, const crashpad_state_t *data)
+{
+    windows_version_t win_ver;
+    if (!sentry__get_windows_version(&win_ver) || win_ver.build < 19041) {
+        SENTRY_WARN("Crashpad WER module not registered, because Windows "
+                    "doesn't meet version requirements (build >= 19041).");
+        return;
+    }
+    sentry_path_t *handler_dir = sentry__path_dir(absolute_handler_path);
+    sentry_path_t *wer_path = nullptr;
+    if (handler_dir) {
+        wer_path = sentry__path_join_str(handler_dir, "crashpad_wer.dll");
+        sentry__path_free(handler_dir);
+    }
+
+    if (wer_path && sentry__path_is_file(wer_path)) {
+        SENTRY_TRACEF("registering crashpad WER handler "
+                      "\"%" SENTRY_PATH_PRI "\"",
+            wer_path->path);
+
+        // The WER handler needs to be registered in the registry first.
+        constexpr DWORD dwOne = 1;
+        const LSTATUS reg_res = RegSetKeyValueW(HKEY_CURRENT_USER,
+            L"Software\\Microsoft\\Windows\\Windows Error Reporting\\"
+            L"RuntimeExceptionHelperModules",
+            wer_path->path, REG_DWORD, &dwOne, sizeof(DWORD));
+        if (reg_res != ERROR_SUCCESS) {
+            SENTRY_WARN("registering crashpad WER handler in registry failed");
+        } else {
+            const std::wstring wer_path_string(wer_path->path);
+            if (!data->client->RegisterWerModule(wer_path_string)) {
+                SENTRY_WARN("registering crashpad WER handler module failed");
+            }
+        }
+
+        sentry__path_free(wer_path);
+    } else {
+        SENTRY_WARN("crashpad WER handler module not found");
+    }
+}
+#endif
+
 static void
 crashpad_backend_flush_scope(
     sentry_backend_t *backend, const sentry_options_t *options)
@@ -363,39 +413,9 @@ crashpad_backend_startup(
         return 1;
     }
 
-#ifdef CRASHPAD_WER_ENABLED
-    sentry_path_t *handler_dir = sentry__path_dir(absolute_handler_path);
-    sentry_path_t *wer_path = nullptr;
-    if (handler_dir) {
-        wer_path = sentry__path_join_str(handler_dir, "crashpad_wer.dll");
-        sentry__path_free(handler_dir);
-    }
-
-    if (wer_path && sentry__path_is_file(wer_path)) {
-        SENTRY_TRACEF("registering crashpad WER handler "
-                      "\"%" SENTRY_PATH_PRI "\"",
-            wer_path->path);
-
-        // The WER handler needs to be registered in the registry first.
-        DWORD dwOne = 1;
-        LSTATUS reg_res = RegSetKeyValueW(HKEY_CURRENT_USER,
-            L"Software\\Microsoft\\Windows\\Windows Error Reporting\\"
-            L"RuntimeExceptionHelperModules",
-            wer_path->path, REG_DWORD, &dwOne, sizeof(DWORD));
-        if (reg_res != ERROR_SUCCESS) {
-            SENTRY_WARN("registering crashpad WER handler in registry failed");
-        } else {
-            std::wstring wer_path_string(wer_path->path);
-            if (!data->client->RegisterWerModule(wer_path_string)) {
-                SENTRY_WARN("registering crashpad WER handler module failed");
-            }
-        }
-
-        sentry__path_free(wer_path);
-    } else {
-        SENTRY_WARN("crashpad WER handler module not found");
-    }
-#endif // CRASHPAD_WER_ENABLED
+#ifdef SENTRY_PLATFORM_WINDOWS
+    crashpad_register_wer_module(absolute_handler_path, data);
+#endif
 
     sentry__path_free(absolute_handler_path);
 

diff --git a/src/sentry_os.c b/src/sentry_os.c
@@ -8,10 +8,9 @@
 #    define CURRENT_VERSION "SOFTWARE\\Microsoft\\Windows NT\\CurrentVersion"
 
 void *
-sentry__try_file_version(LPCWSTR filename)
+sentry__try_file_version(const LPCWSTR filename)
 {
-
-    DWORD size = GetFileVersionInfoSizeW(filename, NULL);
+    const DWORD size = GetFileVersionInfoSizeW(filename, NULL);
     if (!size) {
         return NULL;
     }
@@ -24,86 +23,115 @@ sentry__try_file_version(LPCWSTR filename)
     return ffibuf;
 }
 
-sentry_value_t
-sentry__get_os_context(void)
+int
+sentry__get_kernel_version(windows_version_t *win_ver)
 {
-    sentry_value_t os = sentry_value_new_object();
-    if (sentry_value_is_null(os)) {
-        return os;
-    }
-
-    sentry_value_set_by_key(os, "name", sentry_value_new_string("Windows"));
-
     void *ffibuf = sentry__try_file_version(L"ntoskrnl.exe");
     if (!ffibuf) {
         ffibuf = sentry__try_file_version(L"kernel32.dll");
     }
     if (!ffibuf) {
-        goto fail;
+        return 0;
     }
 
     VS_FIXEDFILEINFO *ffi;
     UINT ffi_size;
-    if (!VerQueryValueW(ffibuf, L"\\", &ffi, &ffi_size)) {
-        goto fail;
+    if (!VerQueryValueW(ffibuf, L"\\", (LPVOID *)&ffi, &ffi_size)) {
+        sentry_free(ffibuf);
+        return 0;
     }
     ffi->dwFileFlags &= ffi->dwFileFlagsMask;
 
-    uint32_t major_version = ffi->dwFileVersionMS >> 16;
-    uint32_t minor_version = ffi->dwFileVersionMS & 0xffff;
-    uint32_t build_version = ffi->dwFileVersionLS >> 16;
-    uint32_t ubr = ffi->dwFileVersionLS & 0xffff;
-
-    char buf[32];
-    snprintf(buf, sizeof(buf), "%u.%u.%u.%lu", major_version, minor_version,
-        build_version, ubr);
-    sentry_value_set_by_key(os, "kernel_version", sentry_value_new_string(buf));
+    win_ver->major = ffi->dwFileVersionMS >> 16;
+    win_ver->minor = ffi->dwFileVersionMS & 0xffff;
+    win_ver->build = ffi->dwFileVersionLS >> 16;
+    win_ver->ubr = ffi->dwFileVersionLS & 0xffff;
 
     sentry_free(ffibuf);
 
+    return 1;
+}
+
+int
+sentry__get_windows_version(windows_version_t *win_ver)
+{
     // The `CurrentMajorVersionNumber`, `CurrentMinorVersionNumber` and `UBR`
     // are DWORD, while `CurrentBuild` is a SZ (text).
-
     uint32_t reg_version = 0;
     DWORD buf_size = sizeof(uint32_t);
     if (RegGetValueA(HKEY_LOCAL_MACHINE, CURRENT_VERSION,
             "CurrentMajorVersionNumber", RRF_RT_REG_DWORD, NULL, &reg_version,
             &buf_size)
-        == ERROR_SUCCESS) {
-        major_version = reg_version;
+        != ERROR_SUCCESS) {
+        return 0;
     }
+    win_ver->major = reg_version;
+
     buf_size = sizeof(uint32_t);
     if (RegGetValueA(HKEY_LOCAL_MACHINE, CURRENT_VERSION,
             "CurrentMinorVersionNumber", RRF_RT_REG_DWORD, NULL, &reg_version,
             &buf_size)
-        == ERROR_SUCCESS) {
-        minor_version = reg_version;
+        != ERROR_SUCCESS) {
+        return 0;
     }
+    win_ver->minor = reg_version;
+
+    char buf[32];
     buf_size = sizeof(buf);
     if (RegGetValueA(HKEY_LOCAL_MACHINE, CURRENT_VERSION, "CurrentBuild",
             RRF_RT_REG_SZ, NULL, buf, &buf_size)
-        == ERROR_SUCCESS) {
-        build_version = (uint32_t)sentry__strtod_c(buf, NULL);
+        != ERROR_SUCCESS) {
+        return 0;
     }
+    win_ver->build = (uint32_t)sentry__strtod_c(buf, NULL);
+
     buf_size = sizeof(uint32_t);
     if (RegGetValueA(HKEY_LOCAL_MACHINE, CURRENT_VERSION, "UBR",
             RRF_RT_REG_DWORD, NULL, &reg_version, &buf_size)
-        == ERROR_SUCCESS) {
-        ubr = reg_version;
+        != ERROR_SUCCESS) {
+        return 0;
     }
+    win_ver->ubr = reg_version;
 
-    snprintf(buf, sizeof(buf), "%u.%u.%u", major_version, minor_version,
-        build_version);
-    sentry_value_set_by_key(os, "version", sentry_value_new_string(buf));
+    return 1;
+}
 
-    snprintf(buf, sizeof(buf), "%lu", ubr);
-    sentry_value_set_by_key(os, "build", sentry_value_new_string(buf));
+sentry_value_t
+sentry__get_os_context(void)
+{
+    const sentry_value_t os = sentry_value_new_object();
+    if (sentry_value_is_null(os)) {
+        return os;
+    }
+    sentry_value_set_by_key(os, "name", sentry_value_new_string("Windows"));
 
-    sentry_value_freeze(os);
-    return os;
+    bool at_least_one_key_successful = false;
+    char buf[32];
+    windows_version_t win_ver;
+    if (sentry__get_kernel_version(&win_ver)) {
+        at_least_one_key_successful = true;
 
-fail:
-    sentry_free(ffibuf);
+        snprintf(buf, sizeof(buf), "%u.%u.%u.%lu", win_ver.major, win_ver.minor,
+            win_ver.build, win_ver.ubr);
+        sentry_value_set_by_key(
+            os, "kernel_version", sentry_value_new_string(buf));
+    }
+
+    if (sentry__get_windows_version(&win_ver)) {
+        at_least_one_key_successful = true;
+
+        snprintf(buf, sizeof(buf), "%u.%u.%u", win_ver.major, win_ver.minor,
+            win_ver.build);
+        sentry_value_set_by_key(os, "version", sentry_value_new_string(buf));
+
+        snprintf(buf, sizeof(buf), "%lu", win_ver.ubr);
+        sentry_value_set_by_key(os, "build", sentry_value_new_string(buf));
+    }
+
+    if (at_least_one_key_successful) {
+        sentry_value_freeze(os);
+        return os;
+    }
 
     sentry_value_decref(os);
     return sentry_value_new_null();