From 59ef782120ac46c949184e0ef49aa346b2b2ba31 Mon Sep 17 00:00:00 2001 From: James Crosswell Date: Fri, 17 Jan 2025 22:45:48 +1300 Subject: [PATCH 1/8] Disable the NDK by default --- src/Sentry/Platforms/Android/NativeOptions.cs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/Sentry/Platforms/Android/NativeOptions.cs b/src/Sentry/Platforms/Android/NativeOptions.cs index fdef25e518..9e53ee794d 100644 --- a/src/Sentry/Platforms/Android/NativeOptions.cs +++ b/src/Sentry/Platforms/Android/NativeOptions.cs @@ -167,13 +167,19 @@ internal NativeOptions(SentryOptions options) public TimeSpan ConnectionTimeout { get; set; } = TimeSpan.FromSeconds(5); /// + /// /// Gets or sets a value that indicates if the NDK (Android Native Development Kit) is enabled. - /// The default value is true (enabled). + /// The default value is false (disabled). + /// + /// + /// NOTE: We do not currently recommend enabling this feature. + /// See: https://github.com/getsentry/sentry-dotnet/issues/3902 + /// /// /// /// See https://docs.sentry.io/platforms/android/using-ndk/#disable-ndk-integration /// - public bool EnableNdk { get; set; } = true; + public bool EnableNdk { get; set; } = false; /// /// Gets or sets a value that indicates if the hook that flushes when the main Java thread shuts down From 179d173c667ccff44c9a24caae69fa1dd52ddba5 Mon Sep 17 00:00:00 2001 From: James Crosswell Date: Fri, 17 Jan 2025 23:00:58 +1300 Subject: [PATCH 2/8] Update CHANGELOG.md --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index cf2cf291e5..a029467ee6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,9 @@ ## Unreleased +### Significant change in behavior +- The NDK is now disabled by default on Android, to prevent SIGSEGV errors resulting from managed NullReferenceExceptions ([#3903](https://github.com/getsentry/sentry-dotnet/pull/3903)) + ### Features - .NET on iOS: Add experimental EnableAppHangTrackingV2 configuration flag to the options binding SDK ([#3877](https://github.com/getsentry/sentry-dotnet/pull/3877)) From 34853214fb91015f185e447114116c7c0bdc30b8 Mon Sep 17 00:00:00 2001 From: James Crosswell Date: Mon, 3 Feb 2025 19:22:21 +1300 Subject: [PATCH 3/8] Revert EnableNdk to true by default --- src/Sentry/Platforms/Android/NativeOptions.cs | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/src/Sentry/Platforms/Android/NativeOptions.cs b/src/Sentry/Platforms/Android/NativeOptions.cs index 9e53ee794d..fdef25e518 100644 --- a/src/Sentry/Platforms/Android/NativeOptions.cs +++ b/src/Sentry/Platforms/Android/NativeOptions.cs @@ -167,19 +167,13 @@ internal NativeOptions(SentryOptions options) public TimeSpan ConnectionTimeout { get; set; } = TimeSpan.FromSeconds(5); /// - /// /// Gets or sets a value that indicates if the NDK (Android Native Development Kit) is enabled. - /// The default value is false (disabled). - /// - /// - /// NOTE: We do not currently recommend enabling this feature. - /// See: https://github.com/getsentry/sentry-dotnet/issues/3902 - /// + /// The default value is true (enabled). /// /// /// See https://docs.sentry.io/platforms/android/using-ndk/#disable-ndk-integration /// - public bool EnableNdk { get; set; } = false; + public bool EnableNdk { get; set; } = true; /// /// Gets or sets a value that indicates if the hook that flushes when the main Java thread shuts down From a05408770f6408705bf7172e7ee447f004c28aef Mon Sep 17 00:00:00 2001 From: James Crosswell Date: Mon, 3 Feb 2025 19:23:11 +1300 Subject: [PATCH 4/8] Suppress SIGSEGV exceptions in the Native BeforeSend callback --- src/Sentry/Platforms/Android/SentrySdk.cs | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/src/Sentry/Platforms/Android/SentrySdk.cs b/src/Sentry/Platforms/Android/SentrySdk.cs index 607f4dee59..3c4a0caad0 100644 --- a/src/Sentry/Platforms/Android/SentrySdk.cs +++ b/src/Sentry/Platforms/Android/SentrySdk.cs @@ -3,6 +3,7 @@ using Sentry.Android; using Sentry.Android.Callbacks; using Sentry.Android.Extensions; +using Sentry.Extensibility; using Sentry.JavaSdk.Android.Core; // Don't let the Sentry Android SDK auto-init, as we do that manually in SentrySdk.Init @@ -99,10 +100,19 @@ private static void InitSentryAndroidSdk(SentryOptions options) } } - if (options.Native.EnableBeforeSend && options.BeforeSendInternal is { } beforeSend) + SentryEvent? BeforeSendWrapper(SentryEvent evt, SentryHint hint) { - o.BeforeSend = new BeforeSendCallback(beforeSend, options, o); + if (evt.SentryExceptions?.SingleOrDefault() is { Type: "SIGSEGV", Value: "Segfault" } exception) + { + options.LogDebug("Suppressing SIGSEGV (this will be thrown as a managed exception instead)"); + return null; + } + // Call the user defined BeforeSend callback, if it's defined - otherwise return the event as-is + return (options.Native.EnableBeforeSend && options.BeforeSendInternal is { } beforeSend) + ? beforeSend(evt, hint) + : evt; } + o.BeforeSend = new BeforeSendCallback(BeforeSendWrapper, options, o); // These options are from SentryAndroidOptions o.AttachScreenshot = options.Native.AttachScreenshot; From c3c1278ac1932636dd59129595d464ef3ef1381a Mon Sep 17 00:00:00 2001 From: James Crosswell Date: Mon, 3 Feb 2025 19:34:06 +1300 Subject: [PATCH 5/8] Update CHANGELOG.md --- CHANGELOG.md | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 758cea685c..f08eca6ca2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,9 +1,14 @@ # Changelog +## Unreleased + +### Fixes + +- Native SIGSEGV errors resulting from managed NullReferenceExceptions are now suppressed on Android ([#3903](https://github.com/getsentry/sentry-dotnet/pull/3903)) + ## 5.1.0 ### Significant change in behavior -- The NDK is now disabled by default on Android, to prevent SIGSEGV errors resulting from managed NullReferenceExceptions ([#3903](https://github.com/getsentry/sentry-dotnet/pull/3903)) - The User.IpAddress is now only set to `{{auto}}` when `SendDefaultPii` is enabled. This change gives you control over IP address collection directly on the client ([#3893](https://github.com/getsentry/sentry-dotnet/pull/3893)) ### Features From 664b37f646d14128a3c8439441fe8b3fc2a92044 Mon Sep 17 00:00:00 2001 From: James Crosswell Date: Wed, 5 Feb 2025 12:36:24 +1300 Subject: [PATCH 6/8] Added device tests for the Android BeforeSendWrapper --- src/Sentry/Platforms/Android/SentrySdk.cs | 33 ++++--- .../Platforms/Android/SentrySdkTests.cs | 89 +++++++++++++++++++ 2 files changed, 109 insertions(+), 13 deletions(-) create mode 100644 test/Sentry.Tests/Platforms/Android/SentrySdkTests.cs diff --git a/src/Sentry/Platforms/Android/SentrySdk.cs b/src/Sentry/Platforms/Android/SentrySdk.cs index 3c4a0caad0..141ebcc856 100644 --- a/src/Sentry/Platforms/Android/SentrySdk.cs +++ b/src/Sentry/Platforms/Android/SentrySdk.cs @@ -100,19 +100,7 @@ private static void InitSentryAndroidSdk(SentryOptions options) } } - SentryEvent? BeforeSendWrapper(SentryEvent evt, SentryHint hint) - { - if (evt.SentryExceptions?.SingleOrDefault() is { Type: "SIGSEGV", Value: "Segfault" } exception) - { - options.LogDebug("Suppressing SIGSEGV (this will be thrown as a managed exception instead)"); - return null; - } - // Call the user defined BeforeSend callback, if it's defined - otherwise return the event as-is - return (options.Native.EnableBeforeSend && options.BeforeSendInternal is { } beforeSend) - ? beforeSend(evt, hint) - : evt; - } - o.BeforeSend = new BeforeSendCallback(BeforeSendWrapper, options, o); + o.BeforeSend = new BeforeSendCallback(BeforeSendWrapper(options), options, o); // These options are from SentryAndroidOptions o.AttachScreenshot = options.Native.AttachScreenshot; @@ -182,6 +170,25 @@ private static void InitSentryAndroidSdk(SentryOptions options) // TODO: Pause/Resume } + internal static Func BeforeSendWrapper(SentryOptions options) + { + return (evt, hint) => + { + // Suppress SIGSEGV errors. + // See: https://github.com/getsentry/sentry-dotnet/pull/3903 + if (evt.SentryExceptions?.SingleOrDefault() is { Type: "SIGSEGV", Value: "Segfault" } exception) + { + options.LogDebug("Suppressing SIGSEGV (this will be thrown as a managed exception instead)"); + return null; + } + + // Call the user defined BeforeSend callback, if it's defined - otherwise return the event as-is + return (options.Native.EnableBeforeSend && options.BeforeSendInternal is { } beforeSend) + ? beforeSend(evt, hint) + : evt; + }; + } + private static void AndroidEnvironment_UnhandledExceptionRaiser(object? _, RaiseThrowableEventArgs e) { var description = "This exception was caught by the Android global error handler."; diff --git a/test/Sentry.Tests/Platforms/Android/SentrySdkTests.cs b/test/Sentry.Tests/Platforms/Android/SentrySdkTests.cs new file mode 100644 index 0000000000..7e2215c64c --- /dev/null +++ b/test/Sentry.Tests/Platforms/Android/SentrySdkTests.cs @@ -0,0 +1,89 @@ +#if ANDROID +namespace Sentry.Tests.Platforms.Android; + +public class SentrySdkTests +{ + [Fact] + public void BeforeSendWrapper_SuppressSIGSEGV_ReturnsNull() + { + // Arrange + var options = new SentryOptions(); + var evt = new SentryEvent + { + SentryExceptions = new[] + { + new SentryException + { + Type = "SIGSEGV", + Value = "Segfault" + } + } + }; + var hint = new SentryHint(); + + // Act + var result = SentrySdk.BeforeSendWrapper(options).Invoke(evt, hint); + + // Assert + result.Should().BeNull(); + } + + [Fact] + public void BeforeSendWrapper_BeforeSendCallbackDefined_CallsBeforeSend() + { + // Arrange + var beforeSend = Substitute.For>(); + beforeSend.Invoke(Arg.Any(), Arg.Any()).Returns(callInfo => callInfo.Arg()); + + var options = new SentryOptions(); + options.Native.EnableBeforeSend = true; + options.SetBeforeSend(beforeSend); + var evt = new SentryEvent(); + var hint = new SentryHint(); + + // Act + var result = SentrySdk.BeforeSendWrapper(options).Invoke(evt, hint); + + // Assert + beforeSend.Received(1).Invoke(Arg.Any(), Arg.Any()); + result.Should().Be(evt); + } + + [Fact] + public void BeforeSendWrapper_NoBeforeSendCallback_ReturnsEvent() + { + // Arrange + var options = new SentryOptions(); + options.Native.EnableBeforeSend = true; + var evt = new SentryEvent(); + var hint = new SentryHint(); + + // Act + var result = SentrySdk.BeforeSendWrapper(options).Invoke(evt, hint); + + // Assert + result.Should().Be(evt); + } + + [Fact] + public void BeforeSendWrapper_NativeBeforeSendDisabled_ReturnsEvent() + { + // Arrange + var beforeSend = Substitute.For>(); + beforeSend.Invoke(Arg.Any(), Arg.Any()).Returns(_ => null); + + var options = new SentryOptions(); + options.SetBeforeSend(beforeSend); + options.Native.EnableBeforeSend = false; + var evt = new SentryEvent(); + var hint = new SentryHint(); + + // Act + var result = SentrySdk.BeforeSendWrapper(options).Invoke(evt, hint); + + // Assert + beforeSend.DidNotReceive().Invoke(Arg.Any(), Arg.Any()); + result.Should().Be(evt); + } +} +#endif From 370efa0e924c62f253a4e1befed8ef6bd949248c Mon Sep 17 00:00:00 2001 From: James Crosswell Date: Fri, 7 Feb 2025 10:03:45 +1300 Subject: [PATCH 7/8] Update SentrySdk.cs --- src/Sentry/Platforms/Android/SentrySdk.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Sentry/Platforms/Android/SentrySdk.cs b/src/Sentry/Platforms/Android/SentrySdk.cs index 141ebcc856..dfd467d904 100644 --- a/src/Sentry/Platforms/Android/SentrySdk.cs +++ b/src/Sentry/Platforms/Android/SentrySdk.cs @@ -176,7 +176,7 @@ private static void InitSentryAndroidSdk(SentryOptions options) { // Suppress SIGSEGV errors. // See: https://github.com/getsentry/sentry-dotnet/pull/3903 - if (evt.SentryExceptions?.SingleOrDefault() is { Type: "SIGSEGV", Value: "Segfault" } exception) + if (evt.SentryExceptions?.FirstOrDefault() is { Type: "SIGSEGV", Value: "Segfault" } exception) { options.LogDebug("Suppressing SIGSEGV (this will be thrown as a managed exception instead)"); return null; From e06c09cd33ed4c931ba78741bca1fe010b745747 Mon Sep 17 00:00:00 2001 From: James Crosswell Date: Fri, 7 Feb 2025 10:47:25 +1300 Subject: [PATCH 8/8] Added option to enable/disable Segfault suppression --- .../Platforms/Android/AndroidOptions.cs | 16 ++++++++++ .../Android/BindableAndroidSentryOptions.cs | 2 ++ src/Sentry/Platforms/Android/SentrySdk.cs | 3 +- .../Platforms/Android/SentrySdkTests.cs | 29 ++++++++++++++++++- 4 files changed, 48 insertions(+), 2 deletions(-) diff --git a/src/Sentry/Platforms/Android/AndroidOptions.cs b/src/Sentry/Platforms/Android/AndroidOptions.cs index f6dd210aaa..73e38ae20a 100644 --- a/src/Sentry/Platforms/Android/AndroidOptions.cs +++ b/src/Sentry/Platforms/Android/AndroidOptions.cs @@ -25,5 +25,21 @@ public class AndroidOptions /// /// public int LogCatMaxLines { get; set; } = 1000; + + /// + /// + /// Whether to suppress capturing SIGSEGV (Segfault) errors in the Native SDK. + /// + /// + /// When managed code results in a NullReferenceException, this also causes a SIGSEGV (Segfault). Duplicate + /// events (one managed and one native) can be prevented by suppressing native Segfaults, which may be + /// convenient. + /// + /// + /// Enabling this may prevent the capture of Segfault originating from native (not managed) code... so it may + /// prevent the capture of genuine native Segfault errors. + /// + /// + public bool SuppressSegfaults { get; set; } = false; } } diff --git a/src/Sentry/Platforms/Android/BindableAndroidSentryOptions.cs b/src/Sentry/Platforms/Android/BindableAndroidSentryOptions.cs index ea721bb7d4..0fa8264cb1 100644 --- a/src/Sentry/Platforms/Android/BindableAndroidSentryOptions.cs +++ b/src/Sentry/Platforms/Android/BindableAndroidSentryOptions.cs @@ -14,11 +14,13 @@ public class AndroidOptions { public LogCatIntegrationType? LogCatIntegration { get; set; } public int? LogCatMaxLines { get; set; } + public bool? SuppressSegfaults { get; set; } public void ApplyTo(SentryOptions.AndroidOptions options) { options.LogCatIntegration = LogCatIntegration ?? options.LogCatIntegration; options.LogCatMaxLines = LogCatMaxLines ?? options.LogCatMaxLines; + options.SuppressSegfaults = SuppressSegfaults ?? options.SuppressSegfaults; } } } diff --git a/src/Sentry/Platforms/Android/SentrySdk.cs b/src/Sentry/Platforms/Android/SentrySdk.cs index dfd467d904..3cba5dac2c 100644 --- a/src/Sentry/Platforms/Android/SentrySdk.cs +++ b/src/Sentry/Platforms/Android/SentrySdk.cs @@ -176,7 +176,8 @@ private static void InitSentryAndroidSdk(SentryOptions options) { // Suppress SIGSEGV errors. // See: https://github.com/getsentry/sentry-dotnet/pull/3903 - if (evt.SentryExceptions?.FirstOrDefault() is { Type: "SIGSEGV", Value: "Segfault" } exception) + if (options.Android.SuppressSegfaults + && evt.SentryExceptions?.FirstOrDefault() is { Type: "SIGSEGV", Value: "Segfault" }) { options.LogDebug("Suppressing SIGSEGV (this will be thrown as a managed exception instead)"); return null; diff --git a/test/Sentry.Tests/Platforms/Android/SentrySdkTests.cs b/test/Sentry.Tests/Platforms/Android/SentrySdkTests.cs index 7e2215c64c..96bde8e62b 100644 --- a/test/Sentry.Tests/Platforms/Android/SentrySdkTests.cs +++ b/test/Sentry.Tests/Platforms/Android/SentrySdkTests.cs @@ -4,10 +4,11 @@ namespace Sentry.Tests.Platforms.Android; public class SentrySdkTests { [Fact] - public void BeforeSendWrapper_SuppressSIGSEGV_ReturnsNull() + public void BeforeSendWrapper_Suppress_SIGSEGV_ReturnsNull() { // Arrange var options = new SentryOptions(); + options.Android.SuppressSegfaults = true; var evt = new SentryEvent { SentryExceptions = new[] @@ -28,6 +29,32 @@ public void BeforeSendWrapper_SuppressSIGSEGV_ReturnsNull() result.Should().BeNull(); } + [Fact] + public void BeforeSendWrapper_DontSupress_SIGSEGV_ReturnsEvent() + { + // Arrange + var options = new SentryOptions(); + options.Android.SuppressSegfaults = false; + var evt = new SentryEvent + { + SentryExceptions = new[] + { + new SentryException + { + Type = "SIGSEGV", + Value = "Segfault" + } + } + }; + var hint = new SentryHint(); + + // Act + var result = SentrySdk.BeforeSendWrapper(options).Invoke(evt, hint); + + // Assert + result.Should().Be(evt); + } + [Fact] public void BeforeSendWrapper_BeforeSendCallbackDefined_CallsBeforeSend() {