diff --git a/CHANGELOG.md b/CHANGELOG.md index ae4f443e8..d3bce55f1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,11 +5,13 @@ **Features**: - Provide version information for non-static Windows binaries. ([#1076](https://github.com/getsentry/sentry-native/pull/1076), [crashpad#110](https://github.com/getsentry/crashpad/pull/110)) +- Add an alternative handler strategy to `inproc` to support `.NET` on Linux and `Mono` on Android (specifically, [.NET MAUI](https://github.com/dotnet/android/issues/9055#issuecomment-2261347912)). ([#1027](https://github.com/getsentry/sentry-native/pull/1027)) - Add SOCKS5 proxy support for macOS and Linux. ([#1063](https://github.com/getsentry/sentry-native/pull/1063)) **Fixes**: - Correct the timeout specified for the upload-task awaiting `dispatch_semaphore_wait()` when using an HTTP-proxy on macOS. ([#1077](https://github.com/getsentry/sentry-native/pull/1077), [crashpad#111](https://github.com/getsentry/crashpad/pull/111)) +- Emit `transaction.data` inside `context.trace.data`. ([#1075](https://github.com/getsentry/sentry-native/pull/1075)) **Thank you**: diff --git a/include/sentry.h b/include/sentry.h index c02bd53fc..e07c4bc4e 100644 --- a/include/sentry.h +++ b/include/sentry.h @@ -768,6 +768,14 @@ typedef enum { SENTRY_USER_CONSENT_REVOKED = 0, } sentry_user_consent_t; +/** + * The crash handler strategy. + */ +typedef enum { + SENTRY_HANDLER_STRATEGY_DEFAULT = 0, + SENTRY_HANDLER_STRATEGY_CHAIN_AT_START = 1, +} sentry_handler_strategy_t; + /** * Creates a new options struct. * Can be freed with `sentry_options_free`. @@ -1511,6 +1519,36 @@ SENTRY_EXPERIMENTAL_API void sentry_options_set_traces_sample_rate( SENTRY_EXPERIMENTAL_API double sentry_options_get_traces_sample_rate( sentry_options_t *opts); +#ifdef SENTRY_PLATFORM_LINUX + +/** + * Returns the currently set strategy for the handler. + * + * This option does only work for the `inproc` backend on `Linux` and `Android`. + * + * The main use-case is when the Native SDK is used in the context of the + * CLR/Mono runtimes which convert some POSIX signals into managed-code + * exceptions and discontinue the signal chain. + * + * If this happens and we invoke the previous handler at the end (i.e., after + * our handler processed the signal, which is the default strategy) we will end + * up sending a native crash in addition to the managed code exception (which + * will either generate another crash-event if uncaught or could be handled in + * the managed code and neither terminate the application nor create a crash + * event). To correctly process the signals of CLR/Mono applications, we must + * invoke the runtime handler at the start of our handler. + */ +SENTRY_EXPERIMENTAL_API sentry_handler_strategy_t +sentry_options_get_handler_strategy(const sentry_options_t *opts); + +/** + * Sets the handler strategy. + */ +SENTRY_EXPERIMENTAL_API void sentry_options_set_handler_strategy( + sentry_options_t *opts, sentry_handler_strategy_t handler_strategy); + +#endif // SENTRY_PLATFORM_LINUX + /* -- Session APIs -- */ typedef enum { diff --git a/src/backends/sentry_backend_inproc.c b/src/backends/sentry_backend_inproc.c index 042729be1..7ad78ab5b 100644 --- a/src/backends/sentry_backend_inproc.c +++ b/src/backends/sentry_backend_inproc.c @@ -16,27 +16,6 @@ #include "transports/sentry_disk_transport.h" #include -/** - * Android's bionic libc seems to allocate alternate signal handler stacks for - * every thread and also references them from their internal maintenance - * structs. - * - * The way we currently set up our sigaltstack seems to interfere with this - * setup and causes crashes whenever an ART signal handler touches the thread - * that called `sentry_init()`. - * - * In addition to this problem, it also means there is no need for our own - * sigaltstack on Android since our signal handler will always be running on - * an alternate stack managed by bionic. - * - * Note: In bionic the sigaltstacks for 32-bit devices have a size of 16KiB and - * on 64-bit devices they have 32KiB. The size of our own was set to 64KiB - * independent of the device. If this is a problem, we need figure out - * together with Google if there is a way in which our configs can coexist. - * - * Both breakpad and crashpad are way more defensive in the setup of their - * signal stacks and take existing stacks into account (or reuse them). - */ #define SIGNAL_DEF(Sig, Desc) { Sig, #Sig, Desc } #define MAX_FRAMES 128 @@ -108,8 +87,8 @@ startup_inproc_backend( // set up an alternate signal stack if noone defined one before stack_t old_sig_stack; - if (sigaltstack(NULL, &old_sig_stack) == -1 || old_sig_stack.ss_sp == NULL - || old_sig_stack.ss_size == 0) { + int ret = sigaltstack(NULL, &old_sig_stack); + if (ret == 0 && old_sig_stack.ss_flags == SS_DISABLE) { SENTRY_TRACEF("installing signal stack (size: %d)", SIGNAL_STACK_SIZE); g_signal_stack.ss_sp = sentry_malloc(SIGNAL_STACK_SIZE); if (!g_signal_stack.ss_sp) { @@ -118,9 +97,11 @@ startup_inproc_backend( g_signal_stack.ss_size = SIGNAL_STACK_SIZE; g_signal_stack.ss_flags = 0; sigaltstack(&g_signal_stack, 0); - } else { - SENTRY_TRACEF( - "using existing signal stack (size: %d)", old_sig_stack.ss_size); + } else if (ret == 0) { + SENTRY_TRACEF("using existing signal stack (size: %d, flags: %d)", + old_sig_stack.ss_size, old_sig_stack.ss_flags); + } else if (ret == -1) { + SENTRY_WARNF("Failed to query signal stack size: %s", strerror(errno)); } // install our own signal handler @@ -554,21 +535,47 @@ handle_ucontext(const sentry_ucontext_t *uctx) } #ifdef SENTRY_PLATFORM_UNIX - // give us an allocator we can use safely in signals before we tear down. - sentry__page_allocator_enable(); - // inform the sentry_sync system that we're in a signal handler. This will // make mutexes spin on a spinlock instead as it's no longer safe to use a // pthread mutex. sentry__enter_signal_handler(); #endif - sentry_value_t event = make_signal_event(sig_slot, uctx); - SENTRY_WITH_OPTIONS (options) { - sentry__write_crash_marker(options); +#ifdef SENTRY_PLATFORM_LINUX + // On Linux (and thus Android) CLR/Mono converts signals provoked by + // AOT/JIT-generated native code into managed code exceptions. In these + // cases, we shouldn't react to the signal at all and let their handler + // discontinue the signal chain by invoking the runtime handler before + // we process the signal. + if (sentry_options_get_handler_strategy(options) + == SENTRY_HANDLER_STRATEGY_CHAIN_AT_START) { + SENTRY_TRACE("defer to runtime signal handler at start"); + // there is a good chance that we won't return from the previous + // handler and that would mean we couldn't enter this handler with + // the next signal coming in if we didn't "leave" here. + sentry__leave_signal_handler(); + + // invoke the previous handler (typically the CLR/Mono + // signal-to-managed-exception handler) + invoke_signal_handler( + uctx->signum, uctx->siginfo, (void *)uctx->user_context); + + // let's re-enter because it means this was an actual native crash + sentry__enter_signal_handler(); + SENTRY_TRACE( + "return from runtime signal handler, we handle the signal"); + } +#endif +#ifdef SENTRY_PLATFORM_UNIX + // use a signal-safe allocator before we tear down. + sentry__page_allocator_enable(); +#endif + + sentry_value_t event = make_signal_event(sig_slot, uctx); bool should_handle = true; + sentry__write_crash_marker(options); if (options->on_crash_func) { SENTRY_TRACE("invoking `on_crash` hook"); @@ -580,7 +587,7 @@ handle_ucontext(const sentry_ucontext_t *uctx) sentry_envelope_t *envelope = sentry__prepare_event( options, event, NULL, !options->on_crash_func); // TODO(tracing): Revisit when investigating transaction flushing - // during hard crashes. + // during hard crashes. sentry_session_t *session = sentry__end_current_session_with_status( SENTRY_SESSION_STATUS_CRASHED); diff --git a/src/sentry_core.c b/src/sentry_core.c index ebd8ec7a9..cedce7847 100644 --- a/src/sentry_core.c +++ b/src/sentry_core.c @@ -934,6 +934,9 @@ sentry_transaction_finish(sentry_transaction_t *opaque_tx) sentry_value_t trace_context = sentry__value_get_trace_context(opaque_tx->inner); sentry_value_t contexts = sentry_value_new_object(); + sentry_value_set_by_key( + trace_context, "data", sentry_value_get_by_key(tx, "data")); + sentry_value_incref(sentry_value_get_by_key(tx, "data")); sentry_value_set_by_key(contexts, "trace", trace_context); sentry_value_set_by_key(tx, "contexts", contexts); @@ -944,6 +947,7 @@ sentry_transaction_finish(sentry_transaction_t *opaque_tx) sentry_value_remove_by_key(tx, "op"); sentry_value_remove_by_key(tx, "description"); sentry_value_remove_by_key(tx, "status"); + sentry_value_remove_by_key(tx, "data"); sentry__transaction_decref(opaque_tx); diff --git a/src/sentry_options.c b/src/sentry_options.c index e1d807bd1..38b2346af 100644 --- a/src/sentry_options.c +++ b/src/sentry_options.c @@ -57,6 +57,7 @@ sentry_options_new(void) opts->shutdown_timeout = SENTRY_DEFAULT_SHUTDOWN_TIMEOUT; opts->traces_sample_rate = 0.0; opts->max_spans = 0; + opts->handler_strategy = SENTRY_HANDLER_STRATEGY_DEFAULT; return opts; } @@ -613,3 +614,20 @@ sentry_options_set_backend(sentry_options_t *opts, sentry_backend_t *backend) sentry__backend_free(opts->backend); opts->backend = backend; } + +#ifdef SENTRY_PLATFORM_LINUX + +sentry_handler_strategy_t +sentry_options_get_handler_strategy(const sentry_options_t *opts) +{ + return opts->handler_strategy; +} + +void +sentry_options_set_handler_strategy( + sentry_options_t *opts, sentry_handler_strategy_t handler_strategy) +{ + opts->handler_strategy = handler_strategy; +} + +#endif // SENTRY_PLATFORM_LINUX diff --git a/src/sentry_options.h b/src/sentry_options.h index b077fc0bc..07a2fdbe6 100644 --- a/src/sentry_options.h +++ b/src/sentry_options.h @@ -71,6 +71,7 @@ typedef struct sentry_options_s { long user_consent; long refcount; uint64_t shutdown_timeout; + sentry_handler_strategy_t handler_strategy; } sentry_options_t; /** diff --git a/src/sentry_scope.c b/src/sentry_scope.c index 1e7f15e06..78fc084df 100644 --- a/src/sentry_scope.c +++ b/src/sentry_scope.c @@ -318,12 +318,20 @@ sentry__scope_apply_to_event(const sentry_scope_t *scope, // prep contexts sourced from scope; data about transaction on scope needs // to be extracted and inserted - sentry_value_t scope_trace = sentry__value_get_trace_context( - sentry__get_span_or_transaction(scope)); + sentry_value_t scoped_txn_or_span = sentry__get_span_or_transaction(scope); + sentry_value_t scope_trace + = sentry__value_get_trace_context(scoped_txn_or_span); if (!sentry_value_is_null(scope_trace)) { if (sentry_value_is_null(contexts)) { contexts = sentry_value_new_object(); } + sentry_value_t scoped_txn_or_span_data + = sentry_value_get_by_key(scoped_txn_or_span, "data"); + if (!sentry_value_is_null(scoped_txn_or_span_data)) { + sentry_value_incref(scoped_txn_or_span_data); + sentry_value_set_by_key( + scope_trace, "data", scoped_txn_or_span_data); + } sentry_value_set_by_key(contexts, "trace", scope_trace); } diff --git a/src/sentry_tracing.c b/src/sentry_tracing.c index 2a63511ce..e42599f48 100644 --- a/src/sentry_tracing.c +++ b/src/sentry_tracing.c @@ -578,7 +578,7 @@ sentry_transaction_set_data( } } -static const char txn_data_key[] = "extra"; +static const char txn_data_key[] = "data"; static const size_t txn_data_key_len = sizeof(txn_data_key) - 1; void diff --git a/tests/fixtures/dotnet_signal/Program.cs b/tests/fixtures/dotnet_signal/Program.cs new file mode 100644 index 000000000..951adaaf7 --- /dev/null +++ b/tests/fixtures/dotnet_signal/Program.cs @@ -0,0 +1,64 @@ +namespace dotnet_signal; + +using System; +using System.Runtime.InteropServices; + +class Program +{ + [DllImport("crash", EntryPoint = "native_crash")] + static extern void native_crash(); + + [DllImport("crash", EntryPoint = "enable_sigaltstack")] + static extern void enable_sigaltstack(); + + [DllImport("sentry", EntryPoint = "sentry_options_new")] + static extern IntPtr sentry_options_new(); + + [DllImport("sentry", EntryPoint = "sentry_options_set_handler_strategy")] + static extern IntPtr sentry_options_set_handler_strategy(IntPtr options, int strategy); + + [DllImport("sentry", EntryPoint = "sentry_options_set_debug")] + static extern IntPtr sentry_options_set_debug(IntPtr options, int debug); + + [DllImport("sentry", EntryPoint = "sentry_init")] + static extern int sentry_init(IntPtr options); + + static void Main(string[] args) + { + var githubActions = Environment.GetEnvironmentVariable("GITHUB_ACTIONS") ?? string.Empty; + if (githubActions == "true") { + // Set up our own `sigaltstack` for this thread if we're running on GHA because of a failure to run any + // signal handler after the initial setup. This behavior is locally non-reproducible and likely runner-related. + // I ran this against .net7/8/9 on at least 10 different Linux setups, and it worked on all, but on GHA + // it only works if we __don't__ accept the already installed `sigaltstack`. + enable_sigaltstack(); + } + + // setup minimal sentry-native + var options = sentry_options_new(); + sentry_options_set_handler_strategy(options, 1); + sentry_options_set_debug(options, 1); + sentry_init(options); + + var doNativeCrash = args is ["native-crash"]; + if (doNativeCrash) + { + native_crash(); + } + else + { + try + { + Console.WriteLine("dereference a NULL object from managed code"); + var s = default(string); + var c = s.Length; + } + catch (NullReferenceException exception) + { + Console.WriteLine("dereference another NULL object from managed code"); + var s = default(string); + var c = s.Length; + } + } + } +} \ No newline at end of file diff --git a/tests/fixtures/dotnet_signal/crash.c b/tests/fixtures/dotnet_signal/crash.c new file mode 100644 index 000000000..c76fd1a4f --- /dev/null +++ b/tests/fixtures/dotnet_signal/crash.c @@ -0,0 +1,19 @@ +#include +#include +void native_crash(void) +{ + *(int *)10 = 100; +} + +void enable_sigaltstack(void) +{ + const size_t signal_stack_size = 16384; + stack_t signal_stack; + signal_stack.ss_sp = malloc(signal_stack_size); + if (!signal_stack.ss_sp) { + return; + } + signal_stack.ss_size = signal_stack_size; + signal_stack.ss_flags = 0; + sigaltstack(&signal_stack, 0); +} \ No newline at end of file diff --git a/tests/fixtures/dotnet_signal/test_dotnet.csproj b/tests/fixtures/dotnet_signal/test_dotnet.csproj new file mode 100644 index 000000000..64e34a8d4 --- /dev/null +++ b/tests/fixtures/dotnet_signal/test_dotnet.csproj @@ -0,0 +1,8 @@ + + + Exe + net8.0 + enable + enable + + diff --git a/tests/test_dotnet_signals.py b/tests/test_dotnet_signals.py new file mode 100644 index 000000000..2ea098013 --- /dev/null +++ b/tests/test_dotnet_signals.py @@ -0,0 +1,119 @@ +import os +import pathlib +import shutil +import subprocess +import sys + +import pytest + +project_fixture_path = pathlib.Path("tests/fixtures/dotnet_signal") + + +def assert_empty_run_dir(database_path): + run_dirs = [d for d in database_path.glob("*.run") if d.is_dir()] + assert ( + len(run_dirs) == 1 + ), f"Expected exactly one .run directory, found {len(run_dirs)}" + + run_dir = run_dirs[0] + assert not any(run_dir.iterdir()), f"The directory {run_dir} is not empty" + + +def assert_run_dir_with_envelope(database_path): + run_dirs = [d for d in database_path.glob("*.run") if d.is_dir()] + assert ( + len(run_dirs) == 1 + ), f"Expected exactly one .run directory, found {len(run_dirs)}" + + run_dir = run_dirs[0] + crash_envelopes = [f for f in run_dir.glob("*.envelope") if f.is_file()] + assert len(crash_envelopes) > 0, f"Crash envelope is missing" + assert ( + len(crash_envelopes) == 1 + ), f"There is more than one crash envelope ({len(crash_envelopes)}" + + +def run_dotnet(tmp_path, args): + env = os.environ.copy() + env["LD_LIBRARY_PATH"] = str(tmp_path) + ":" + env.get("LD_LIBRARY_PATH", "") + return subprocess.Popen( + args, + cwd=str(project_fixture_path), + env=env, + text=True, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + ) + + +def run_dotnet_managed_exception(tmp_path): + return run_dotnet(tmp_path, ["dotnet", "run"]) + + +def run_dotnet_native_crash(tmp_path): + return run_dotnet(tmp_path, ["dotnet", "run", "native-crash"]) + + +skip_condition = ( + sys.platform != "linux" + or bool(os.environ.get("TEST_X86")) + or "asan" in os.environ.get("RUN_ANALYZER", "") +) + + +@pytest.mark.skipif( + skip_condition, + reason="dotnet signal handling is currently only supported on 64-bit Linux", +) +def test_dotnet_signals_inproc(cmake): + try: + # build native client library with inproc and the example for crash dumping + tmp_path = cmake( + ["sentry"], + {"SENTRY_BACKEND": "inproc", "SENTRY_TRANSPORT": "none"}, + ) + + # build the crashing native library + subprocess.run( + [ + "gcc", + "-Wall", + "-Wextra", + "-fPIC", + "-shared", + str(project_fixture_path / "crash.c"), + "-o", + str(tmp_path / "libcrash.so"), + ], + check=True, + ) + + # this runs the dotnet program with the Native SDK and chain-at-start, when managed code raises a signal that CLR convert to an exception. + dotnet_run = run_dotnet_managed_exception(tmp_path) + dotnet_run_stdout, dotnet_run_stderr = dotnet_run.communicate() + + # the program will fail with a `NullReferenceException`, but the Native SDK won't register a crash. + assert dotnet_run.returncode != 0 + assert ( + "NullReferenceException" in dotnet_run_stderr + ), f"Managed exception run failed.\nstdout:\n{dotnet_run_stdout}\nstderr:\n{dotnet_run_stderr}" + database_path = project_fixture_path / ".sentry-native" + assert database_path.exists(), "No database-path exists" + assert not (database_path / "last_crash").exists(), "A crash was registered" + assert_empty_run_dir(database_path) + + # this runs the dotnet program with the Native SDK and chain-at-start, when an actual native crash raises a signal + dotnet_run = run_dotnet_native_crash(tmp_path) + dotnet_run_stdout, dotnet_run_stderr = dotnet_run.communicate() + + # the program will fail with a SIGSEGV, that has been processed by the Native SDK which produced a crash envelope + assert dotnet_run.returncode != 0 + assert ( + "crash has been captured" in dotnet_run_stderr + ), f"Native exception run failed.\nstdout:\n{dotnet_run_stdout}\nstderr:\n{dotnet_run_stderr}" + assert (database_path / "last_crash").exists() + assert_run_dir_with_envelope(database_path) + finally: + shutil.rmtree(project_fixture_path / ".sentry-native", ignore_errors=True) + shutil.rmtree(project_fixture_path / "bin", ignore_errors=True) + shutil.rmtree(project_fixture_path / "obj", ignore_errors=True) diff --git a/tests/test_integration_http.py b/tests/test_integration_http.py index 639c11bad..67aa160ca 100644 --- a/tests/test_integration_http.py +++ b/tests/test_integration_http.py @@ -546,7 +546,6 @@ def test_transaction_only(cmake, httpserver, build_args): assert_meta( envelope, transaction="little.teapot", - transaction_data={"url": "https://example.com"}, ) # Extract the one-and-only-item @@ -577,6 +576,8 @@ def test_transaction_only(cmake, httpserver, build_args): timestamp = time.strptime(payload["timestamp"], RFC3339_FORMAT) assert timestamp >= start_timestamp + assert trace_context["data"] == {"url": "https://example.com"} + def test_capture_minidump(cmake, httpserver): tmp_path = cmake(["sentry_example"], {"SENTRY_BACKEND": "none"}) diff --git a/tests/unit/test_tracing.c b/tests/unit/test_tracing.c index 5d86ee48a..6650410dd 100644 --- a/tests/unit/test_tracing.c +++ b/tests/unit/test_tracing.c @@ -1089,10 +1089,10 @@ SENTRY_TEST(txn_data) sentry_transaction_set_data( txn, "os.name", sentry_value_new_string("Linux")); - check_after_set(txn->inner, "extra", "os.name", "Linux"); + check_after_set(txn->inner, "data", "os.name", "Linux"); sentry_transaction_remove_data(txn, "os.name"); - check_after_remove(txn->inner, "extra", "os.name"); + check_after_remove(txn->inner, "data", "os.name"); sentry__transaction_decref(txn); } @@ -1139,10 +1139,10 @@ SENTRY_TEST(txn_data_n) sentry_value_t data_value = sentry_value_new_string_n(data_v, sizeof(data_v)); sentry_transaction_set_data_n(txn, data_k, sizeof(data_k), data_value); - check_after_set(txn->inner, "extra", "os.name", "Linux"); + check_after_set(txn->inner, "data", "os.name", "Linux"); sentry_transaction_remove_data_n(txn, data_k, sizeof(data_k)); - check_after_remove(txn->inner, "extra", "os.name"); + check_after_remove(txn->inner, "data", "os.name"); sentry__transaction_decref(txn); }