fix: more defensive auto thread stack guarantee (#1196)

* fix: provide less aggressive auto thread stack guarantee

* verbose-log is off

* update changelog

* ensure no unused `success` variable

* update inline docs to reflect that sentry_init() is no longer a requirement

* clarify error logs when SetThreadStackGuarantee failed

* redo dynamic loading of thread-stack-guarantee functions from DllMain since all come directly from kernel32 which guaranteed to be loaded.

* remove superflous 0

Co-authored-by: JoshuaMoelans <[email protected]>

* clarify 32KiB as a lower bound

* replace copy-pasted GetCurrentThreadStackLimits error message

* adding unit-test

* fix syntax error in calling convention of unit-test utilities

* separate cmake patterns in editorconfig

* clion ignores editorconfig for cmake -> fix inconsistent space-based indentation

* further whitespace fixes

* parameterize stack-overflow integration tests

expose handler stack size as first step where applicable.

---------

Co-authored-by: JoshuaMoelans <[email protected]>

Author: supervacuus

.editorconfig

@@ -9,5 +9,11 @@ charset = utf-8
 [Makefile]
 indent_style = tab
 
-[{CMakeLists.txt, *.cmake*}]
+[CMakeLists.txt]
+indent_style = tab
+
+[*.cmake]
+indent_style = tab
+
+[*.cmake.in]
 indent_style = tab

CHANGELOG.md

@@ -6,6 +6,10 @@
 
 - Provide an option for downstream SDKs to attach a view hierarchy file. ([#1191](https://github.com/getsentry/sentry-native/pull/1191))
 
+**Fixes**:
+
+- Provide a more defensive automatic thread stack guarantee. ([#1196](https://github.com/getsentry/sentry-native/pull/1196))
+
 ## 0.8.3
 
 **Features**:

CMakeLists.txt

@@ -1,11 +1,11 @@
 if(WIN32)
-	cmake_minimum_required (VERSION 3.18)
+	cmake_minimum_required(VERSION 3.18)
 
 	# enables support for CMAKE_MSVC_RUNTIME_LIBRARY
 	cmake_policy(SET CMP0091 NEW)
 else()
 	# The Android tools ship with this ancient version, which we need to support.
-	cmake_minimum_required (VERSION 3.10)
+	cmake_minimum_required(VERSION 3.10)
 	cmake_policy(SET CMP0077 NEW)
 endif()
 set(SENTRY_TOOLCHAINS_DIR "${CMAKE_CURRENT_LIST_DIR}/toolchains")
@@ -178,11 +178,23 @@ if(SENTRY_BACKEND_CRASHPAD AND ANDROID)
 endif()
 
 set(SENTRY_SDK_NAME "" CACHE STRING "The SDK name to report when sending events.")
+set(SENTRY_HANDLER_STACK_SIZE 64 CACHE STRING "The stack size (in KiB) that should be reserved for the crash handler.")
+if (WIN32)
+	set(SENTRY_THREAD_STACK_GUARANTEE_FACTOR 10 CACHE STRING "The factor by which a threads stack should be larger than the stack guarantee for its handler.")
+	option(SENTRY_THREAD_STACK_GUARANTEE_AUTO_INIT "Automatically sets the thread stack guarantee for each thread via `DllMain` or for the `sentry_init()` when building statically" ON)
+	option(SENTRY_THREAD_STACK_GUARANTEE_VERBOSE_LOG "Enables logging of successfully set thread stack guarantees" OFF)
+endif()
 
 message(STATUS "SENTRY_TRANSPORT=${SENTRY_TRANSPORT}")
 message(STATUS "SENTRY_BACKEND=${SENTRY_BACKEND}")
 message(STATUS "SENTRY_LIBRARY_TYPE=${SENTRY_LIBRARY_TYPE}")
 message(STATUS "SENTRY_SDK_NAME=${SENTRY_SDK_NAME}")
+message(STATUS "SENTRY_HANDLER_STACK_SIZE=${SENTRY_HANDLER_STACK_SIZE}")
+if (WIN32)
+	message(STATUS "SENTRY_THREAD_STACK_GUARANTEE_FACTOR=${SENTRY_THREAD_STACK_GUARANTEE_FACTOR}")
+	message(STATUS "SENTRY_THREAD_STACK_GUARANTEE_AUTO_INIT=${SENTRY_THREAD_STACK_GUARANTEE_AUTO_INIT}")
+	message(STATUS "SENTRY_THREAD_STACK_GUARANTEE_VERBOSE_LOG=${SENTRY_THREAD_STACK_GUARANTEE_VERBOSE_LOG}")
+endif()
 
 if(ANDROID)
 	set(SENTRY_WITH_LIBUNWINDSTACK TRUE)
@@ -253,6 +265,17 @@ target_sources(sentry PRIVATE "${PROJECT_SOURCE_DIR}/include/sentry.h")
 add_library(sentry::sentry ALIAS sentry)
 add_subdirectory(src)
 
+target_compile_definitions(sentry PRIVATE SENTRY_HANDLER_STACK_SIZE=${SENTRY_HANDLER_STACK_SIZE})
+if(WIN32)
+	target_compile_definitions(sentry PRIVATE SENTRY_THREAD_STACK_GUARANTEE_FACTOR=${SENTRY_THREAD_STACK_GUARANTEE_FACTOR})
+	if (SENTRY_THREAD_STACK_GUARANTEE_AUTO_INIT)
+		target_compile_definitions(sentry PRIVATE SENTRY_THREAD_STACK_GUARANTEE_AUTO_INIT)
+	endif()
+	if (SENTRY_THREAD_STACK_GUARANTEE_VERBOSE_LOG)
+		target_compile_definitions(sentry PRIVATE SENTRY_THREAD_STACK_GUARANTEE_VERBOSE_LOG)
+	endif()
+endif()
+
 if (NOT SENTRY_SDK_NAME STREQUAL "")
 	target_compile_definitions(sentry PRIVATE SENTRY_SDK_NAME="${SENTRY_SDK_NAME}")
 endif()
@@ -636,7 +659,10 @@ if(SENTRY_BUILD_EXAMPLES)
 	if(MSVC)
 		target_compile_options(sentry_example PRIVATE $<BUILD_INTERFACE:/wd5105 /wd4717>)
 		if(CMAKE_CXX_COMPILER_ID STREQUAL "Clang") # clang-cl
+			# ignore the warning for the intentionally infinite recursion
 			target_compile_options(sentry_example PRIVATE $<BUILD_INTERFACE:-Wno-infinite-recursion>)
+			# ensure that clang-cl builds an unoptimized `sentry_example` in all build types
+			target_compile_options(sentry_example PRIVATE $<BUILD_INTERFACE:/Od>)
 		endif()
 
 		# to test handling SEH by-passing exceptions we need to enable the control flow guard
@@ -660,7 +686,7 @@ if(SENTRY_BUILD_EXAMPLES)
 		COMMAND ${CMAKE_COMMAND} -E copy_if_different "${CMAKE_CURRENT_SOURCE_DIR}/tests/fixtures/minidump.dmp" "$<TARGET_FILE_DIR:sentry_example>/minidump.dmp")
 
 	add_custom_command(TARGET sentry_example POST_BUILD
-			COMMAND ${CMAKE_COMMAND} -E copy_if_different "${CMAKE_CURRENT_SOURCE_DIR}/tests/fixtures/view-hierarchy.json" "$<TARGET_FILE_DIR:sentry_example>/view-hierarchy.json")
+		COMMAND ${CMAKE_COMMAND} -E copy_if_different "${CMAKE_CURRENT_SOURCE_DIR}/tests/fixtures/view-hierarchy.json" "$<TARGET_FILE_DIR:sentry_example>/view-hierarchy.json")
 
 	add_test(NAME sentry_example COMMAND sentry_example)
 endif()

README.md

@@ -187,26 +187,23 @@ $ export SDKROOT=$(xcrun --sdk macosx --show-sdk-path)
 The following options can be set when running the cmake generator, for example
 using `cmake -D BUILD_SHARED_LIBS=OFF ..`.
 
-- `SENTRY_BUILD_SHARED_LIBS` (Default: ON):
+- `SENTRY_BUILD_SHARED_LIBS` (Default: `ON`):
   By default, `sentry` is built as a shared library. Setting this option to
   `OFF` will build `sentry` as a static library instead.
   If sentry is used as a subdirectory of another project, the value `BUILD_SHARED_LIBS` will be inherited by default.
 
   When using `sentry` as a static library, make sure to `#define SENTRY_BUILD_STATIC 1` before including the sentry header.
 
-- `SENTRY_PIC` (Default: ON):
+- `SENTRY_PIC` (Default: `ON`):
   By default, `sentry` is built as a position-independent library.
 
-- `SENTRY_EXPORT_SYMBOLS` (Default: ON):
-  By default, `sentry` exposes all symbols in the dynamic symbol table. You might want to disable it if the program intends to `dlopen` third-party shared libraries and avoid symbol collisions.
-
-- `SENTRY_BUILD_RUNTIMESTATIC` (Default: OFF):
+- `SENTRY_BUILD_RUNTIMESTATIC` (Default: `OFF`):
   Enables linking with the static MSVC runtime. Has no effect if the compiler is not MSVC.
 
-- `SENTRY_LINK_PTHREAD` (Default: ON):
+- `SENTRY_LINK_PTHREAD` (Default: `ON`):
   Links platform threads library like `pthread` on UNIX targets.
 
-- `SENTRY_BUILD_FORCE32` (Default: OFF):
+- `SENTRY_BUILD_FORCE32` (Default: `OFF`):
   Forces cross-compilation from 64-bit host to 32-bit target. Only affects Linux.
 
 - `CMAKE_SYSTEM_VERSION` (Default: depending on Windows SDK version):
@@ -246,32 +243,58 @@ using `cmake -D BUILD_SHARED_LIBS=OFF ..`.
   - **none**: This builds `sentry-native` without a backend, so it does not handle
     crashes. It is primarily used for tests.
 
-- `SENTRY_INTEGRATION_QT` (Default: OFF):
+- `SENTRY_INTEGRATION_QT` (Default: `OFF`):
   Builds the Qt integration, which turns Qt log messages into breadcrumbs.
 
-- `SENTRY_BREAKPAD_SYSTEM` (Default: OFF):
+- `SENTRY_BREAKPAD_SYSTEM` (Default: `OFF`):
   This instructs the build system to use system-installed breakpad libraries instead of the in-tree version.
 
-- `SENTRY_TRANSPORT_COMPRESSION` (Default: OFF):
+- `SENTRY_TRANSPORT_COMPRESSION` (Default: `OFF`):
   Adds Gzip transport compression. Requires `zlib`.
 
 - `SENTRY_FOLDER` (Default: not defined):
-  Sets the sentry-native projects folder name for generators that support project hierarchy (like Microsoft Visual Studio).
-  To use this feature, you need to enable hierarchy via [`USE_FOLDERS` property](https://cmake.org/cmake/help/latest/prop_gbl/USE_FOLDERS.html)
+  Sets the sentry-native projects folder name for generators that support project hierarchy (like Microsoft Visual
+  Studio). To use this feature, you need to enable hierarchy via [`USE_FOLDERS` property](https://cmake.org/cmake/help/latest/prop_gbl/USE_FOLDERS.html)
 
-- `CRASHPAD_ENABLE_STACKTRACE` (Default: OFF):
-  This enables client-side stackwalking when using the crashpad backend. Stack unwinding will happen on the client's machine
-  and the result will be submitted to Sentry attached to the generated minidump.
-  Note that this feature is still experimental.
+- `CRASHPAD_ENABLE_STACKTRACE` (Default: `OFF`):
+  This enables client-side stackwalking when using the crashpad backend. Stack unwinding will happen on the client's
+  machine and the result will be submitted to Sentry attached to the generated minidump. **Note that this feature is
+  still experimental**.
 
-- `SENTRY_SDK_NAME` (Default: sentry.native or sentry.native.android):
+- `SENTRY_SDK_NAME` (Default: `sentry.native` or `sentry.native.android`):
   Sets the SDK name that should be included in the reported events. If you're overriding this, also define
   the same value using `target_compile_definitions()` on your own targets that include `sentry.h`.
 
+- `SENTRY_HANDLER_STACK_SIZE` (Default: `64`):
+  This specifies the size in KiB of the stack reserved for the crash handler (including hooks like `on_crash` and
+  `before_send`) on Windows, Linux and the `inproc` backend in macOS. Reserving the stack is necessary in case of a
+  stack-overflow, where the handler could otherwise no longer execute. This parameter allows users to specify their
+  target stack size in KiB, because some applications might require a different value from our default. This value can
+  be as small as 16KiB (on `crashpad` and `breakpad`) for handlers to work, but we recommend 32KiB as the lower bound
+  on 64-bit systems. **The value should be a multiple of the page size**.
+
+- `SENTRY_THREAD_STACK_GUARANTEE_FACTOR` (Default: `10`, only for Windows):
+  Defines the factor by which the thread's stack reserve must be bigger than the specified guarantee for the handler.
+  _Example_: if the `SENTRY_HANDLER_STACK_SIZE` is defined as 64KiB then the thread's stack reserve must at least have
+  a size of 640KiB.
+- `SENTRY_THREAD_STACK_GUARANTEE_AUTO_INIT` (Default: `ON`, only for Windows):
+  Ensures that all threads created after the SDK's initialization will be configured to use the
+  `SENTRY_HANDLER_STACK_SIZE` as its handler stack guarantee.
+
+  _Note_: assigning this to all threads only works when building the SDK as a shared library. If you build it as a
+  static library and this option is enabled, only the `sentry_init()` thread will have a stack guarantee for the handler
+  (other threads must be manually initialized via `sentry_set_thread_stack_guarantee()`).
+
+- `SENTRY_THREAD_STACK_GUARANTEE_VERBOSE_LOG` (Default: `OFF`, only for Windows):
+  Adds info level logs for every successfully set thread stack guarantee. This is `OFF` by default, because depending
+  on the number of threads used (by all dependencies) this could flood the logs. But it will be helpful to anyone
+  tuning the thread stack guarantee parameters. Warnings and errors in the process of setting thread stack guarantees
+  will always be logged.
+
 ### Support Matrix
 
 | Feature    | Windows | macOS | Linux | Android | iOS   |
-| ---------- | ------- | ----- | ----- | ------- | ----- |
+|------------|---------|-------|-------|---------|-------|
 | Transports |         |       |       |         |       |
 | - curl     |         | ☑     | ☑     | (✓)***  |       |
 | - winhttp  | ☑       |       |       |         |       |

cmake/utils.cmake

@@ -1,21 +1,21 @@
 # Generates a version resource file from the `sentry.rc.in` template for the `TGT` argument and adds it as a source.
 function(sentry_add_version_resource TGT FILE_DESCRIPTION)
-    # generate a resource output-path from the target name
-    set(RESOURCE_PATH "${CMAKE_CURRENT_BINARY_DIR}/${TGT}.rc")
-    set(RESOURCE_PATH_TMP "${RESOURCE_PATH}.in")
+	# generate a resource output-path from the target name
+	set(RESOURCE_PATH "${CMAKE_CURRENT_BINARY_DIR}/${TGT}.rc")
+	set(RESOURCE_PATH_TMP "${RESOURCE_PATH}.in")
 
-    # Extract major, minor and patch version from SENTRY_VERSION
-    string(REPLACE "." ";" _SENTRY_VERSION_LIST "${SENTRY_VERSION}")
-    list(GET _SENTRY_VERSION_LIST 0 SENTRY_VERSION_MAJOR)
-    list(GET _SENTRY_VERSION_LIST 1 SENTRY_VERSION_MINOR)
-    list(GET _SENTRY_VERSION_LIST 2 SENTRY_VERSION_PATCH)
+	# Extract major, minor and patch version from SENTRY_VERSION
+	string(REPLACE "." ";" _SENTRY_VERSION_LIST "${SENTRY_VERSION}")
+	list(GET _SENTRY_VERSION_LIST 0 SENTRY_VERSION_MAJOR)
+	list(GET _SENTRY_VERSION_LIST 1 SENTRY_VERSION_MINOR)
+	list(GET _SENTRY_VERSION_LIST 2 SENTRY_VERSION_PATCH)
 
-    # Produce the resource file with configure-time replacements
-    configure_file("${SENTRY_SOURCE_DIR}/sentry.rc.in" "${RESOURCE_PATH_TMP}" @ONLY)
+	# Produce the resource file with configure-time replacements
+	configure_file("${SENTRY_SOURCE_DIR}/sentry.rc.in" "${RESOURCE_PATH_TMP}" @ONLY)
 
-    # Replace the `ORIGINAL_FILENAME` at generate-time using the generator expression `TARGET_FILE_NAME`
-    file(GENERATE OUTPUT ${RESOURCE_PATH} INPUT ${RESOURCE_PATH_TMP})
+	# Replace the `ORIGINAL_FILENAME` at generate-time using the generator expression `TARGET_FILE_NAME`
+	file(GENERATE OUTPUT ${RESOURCE_PATH} INPUT ${RESOURCE_PATH_TMP})
 
-    # Finally add the generated resource file to the target sources
-    target_sources("${TGT}" PRIVATE "${RESOURCE_PATH}")
-endfunction()
+	# Finally add the generated resource file to the target sources
+	target_sources("${TGT}" PRIVATE "${RESOURCE_PATH}")
+endfunction()