diff --git a/src/sentry/utils/sdk_crashes/cocoa_sdk_crash_detector.py b/src/sentry/utils/sdk_crashes/cocoa_sdk_crash_detector.py index 97a750875022f6..d2f90a2f380904 100644 --- a/src/sentry/utils/sdk_crashes/cocoa_sdk_crash_detector.py +++ b/src/sentry/utils/sdk_crashes/cocoa_sdk_crash_detector.py @@ -12,6 +12,11 @@ def is_sdk_crash(self, frames: Sequence[Mapping[str, Any]]) -> bool: # The frames are ordered from caller to callee, or oldest to youngest. # The last frame is the one creating the exception. # Therefore, we must iterate in reverse order. + # In a first iteration of this algorithm, we checked for in_app frames, but + # customers can change the in_app configuration, so we can't rely on that. + # Furthermore, if they use static linking for including Sentry Cocoa, Cocoa SDK + # frames can be marked as in_app. Therefore, the algorithm only checks if frames + # are SDK frames or from system libraries. for frame in reversed(frames): # [SentrySDK crash] is a testing function causing a crash. # Therefore, we don't want to mark it a as a SDK crash. @@ -22,7 +27,7 @@ def is_sdk_crash(self, frames: Sequence[Mapping[str, Any]]) -> bool: if self.is_sdk_frame(frame): return True - if frame.get("in_app") is True: + if not self.is_system_library_frame(frame): return False return False @@ -31,7 +36,12 @@ def is_sdk_frame(self, frame: Mapping[str, Any]) -> bool: function = frame.get("function") if function: - function_matchers = ["*sentrycrash*", "**[[]Sentry*"] + function_matchers = [ + r"*sentrycrash*", + r"*\[Sentry*", + r"*(Sentry*)*", # Objective-C class extension categories + r"SentryMX*", # MetricKit Swift classes + ] for matcher in function_matchers: if glob_match(function, matcher, ignorecase=True): return True @@ -44,3 +54,14 @@ def is_sdk_frame(self, frame: Mapping[str, Any]) -> bool: return True return False + + def is_system_library_frame(self, frame: Mapping[str, Any]) -> bool: + system_library_paths = {"/System/Library/", "/usr/lib/"} + + for field in self.fields_containing_paths: + for system_library_path in system_library_paths: + field_with_path = frame.get(field) + if field_with_path and field_with_path.startswith(system_library_path): + return True + + return False diff --git a/src/sentry/utils/sdk_crashes/event_stripper.py b/src/sentry/utils/sdk_crashes/event_stripper.py index 733504c6d6044e..91b26cfa721402 100644 --- a/src/sentry/utils/sdk_crashes/event_stripper.py +++ b/src/sentry/utils/sdk_crashes/event_stripper.py @@ -153,24 +153,12 @@ def _strip_frames( We need to adapt this logic once we support other platforms. """ - fields_containing_paths = {"package", "module", "abs_path"} - - def is_system_library(frame: Mapping[str, Any]) -> bool: - system_library_paths = {"/System/Library/", "/usr/lib/system/"} - - for field in fields_containing_paths: - for path in system_library_paths: - if frame.get(field, "").startswith(path): - return True - - return False - def strip_frame(frame: MutableMapping[str, Any]) -> MutableMapping[str, Any]: if sdk_crash_detector.is_sdk_frame(frame): frame["in_app"] = True # The path field usually contains the name of the application, which we can't keep. - for field in fields_containing_paths: + for field in sdk_crash_detector.fields_containing_paths: if frame.get(field): frame[field] = "Sentry.framework" else: @@ -181,5 +169,6 @@ def strip_frame(frame: MutableMapping[str, Any]) -> MutableMapping[str, Any]: return [ strip_frame(frame) for frame in frames - if sdk_crash_detector.is_sdk_frame(frame) or is_system_library(frame) + if sdk_crash_detector.is_sdk_frame(frame) + or sdk_crash_detector.is_system_library_frame(frame) ] diff --git a/src/sentry/utils/sdk_crashes/sdk_crash_detector.py b/src/sentry/utils/sdk_crashes/sdk_crash_detector.py index 500c3d40b1ba88..e1ee76c8ebc7bd 100644 --- a/src/sentry/utils/sdk_crashes/sdk_crash_detector.py +++ b/src/sentry/utils/sdk_crashes/sdk_crash_detector.py @@ -1,8 +1,12 @@ from abc import ABC, abstractmethod -from typing import Any, Mapping, Sequence +from typing import Any, Mapping, Sequence, Set class SDKCrashDetector(ABC): + @property + def fields_containing_paths(self) -> Set[str]: + return {"package", "module", "abs_path"} + @abstractmethod def is_sdk_crash(self, frames: Sequence[Mapping[str, Any]]) -> bool: """ @@ -15,3 +19,7 @@ def is_sdk_crash(self, frames: Sequence[Mapping[str, Any]]) -> bool: @abstractmethod def is_sdk_frame(self, frame: Mapping[str, Any]) -> bool: raise NotImplementedError + + @abstractmethod + def is_system_library_frame(self, frame: Mapping[str, Any]) -> bool: + raise NotImplementedError diff --git a/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py b/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py index 7b231bf645d20a..499b31277e4319 100644 --- a/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py +++ b/tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py @@ -15,7 +15,7 @@ from sentry.testutils.performance_issues.store_transaction import PerfIssueTransactionTestMixin from sentry.testutils.silo import region_silo_test from sentry.utils.pytest.fixtures import django_db_all -from sentry.utils.safe import set_path +from sentry.utils.safe import get_path, set_path from sentry.utils.sdk_crashes.sdk_crash_detection import sdk_crash_detection @@ -101,6 +101,221 @@ def test_cocoa_sdk_crash_detection_without_context(self, mock_sdk_crash_reporter self.execute_test(event, True, mock_sdk_crash_reporter) + def test_metric_kit_crash_is_detected(self, mock_sdk_crash_reporter): + """ + The frames stem from a real world crash caused by our MetricKit integration. + All data was anonymized. + """ + frames = [ + { + "function": "_dispatch_workloop_worker_thread", + "package": "/usr/lib/system/libdispatch.dylib", + "in_app": False, + }, + { + "function": "_dispatch_lane_serial_drain$VARIANT$armv81", + "package": "/usr/lib/system/libdispatch.dylib", + "in_app": False, + }, + { + "function": "__44-[MXMetricManager deliverDiagnosticPayload:]_block_invoke", + "package": "/System/Library/Frameworks/MetricKit.framework/MetricKit", + "in_app": False, + }, + { + "function": "Sequence.forEach", + "raw_function": "specialized Sequence.forEach((A.Element))", + "package": "/private/var/containers/Bundle/Application/CA061D22-C965-4C50-B383-59D8F14A6DDF/Sentry.app/Sentry", + "filename": "", + "abs_path": "", + "in_app": True, + }, + { + "function": "SentryMXManager.didReceive", + "raw_function": "closure #1 (MXDiagnosticPayload) in SentryMXManager.didReceive([MXDiagnosticPayload])", + "package": "/private/var/containers/Bundle/Application/CA061D22-C965-4C50-B383-59D8F14A6DDF/Sentry.app/Sentry", + "filename": "SentryMXManager.swift", + "abs_path": "/Users/sentry/Library/Developer/Xcode/DerivedData/Consumer/SourcePackages/checkouts/sentry-cocoa/Sources/Swift/MetricKit/SentryMXManager.swift", + "in_app": True, + }, + { + "function": "Sequence.forEach", + "raw_function": "specialized Sequence.forEach((A.Element))", + "package": "/private/var/containers/Bundle/Application/CA061D22-C965-4C50-B383-59D8F14A6DDF/Sentry.app/Sentry", + "filename": "", + "abs_path": "", + "in_app": True, + }, + { + "function": "SentryMXManager.didReceive", + "raw_function": "closure #1 (SentryMXCallStackTree) in closure #3 (MXCPUExceptionDiagnostic) in closure #1 (MXDiagnosticPayload) in SentryMXManager.didReceive([MXDiagnosticPayload])", + "package": "/private/var/containers/Bundle/Application/CA061D22-C965-4C50-B383-59D8F14A6DDF/Sentry.app/Sentry", + "filename": "SentryMXManager.swift", + "abs_path": "/Users/sentry/Library/Developer/Xcode/DerivedData/Consumer/SourcePackages/checkouts/sentry-cocoa/Sources/Swift/MetricKit/SentryMXManager.swift", + "in_app": True, + }, + { + "function": "-[SentryMetricKitIntegration captureEventNotPerThread:params:]", + "package": "/private/var/containers/Bundle/Application/CA061D22-C965-4C50-B383-59D8F14A6DDF/Sentry.app/Sentry", + "filename": "SentryMetricKitIntegration.m", + "abs_path": "/Users/sentry/Library/Developer/Xcode/DerivedData/Consumer/SourcePackages/checkouts/sentry-cocoa/Sources/Sentry/SentryMetricKitIntegration.m", + "in_app": False, + }, + { + "function": "+[SentrySDK captureEvent:]", + "package": "/private/var/containers/Bundle/Application/CA061D22-C965-4C50-B383-59D8F14A6DDF/Sentry.app/Sentry", + "filename": "SentrySDK.m", + "abs_path": "/Users/sentry/Library/Developer/Xcode/DerivedData/Consumer/SourcePackages/checkouts/sentry-cocoa/Sources/Sentry/SentrySDK.m", + "in_app": False, + }, + { + "function": "-[SentryFileManager readAppStateFrom:]", + "package": "/private/var/containers/Bundle/Application/CA061D22-C965-4C50-B383-59D8F14A6DDF/Sentry.app/Sentry", + "filename": "SentryFileManager.m", + "abs_path": "/Users/sentry/Library/Developer/Xcode/DerivedData/Consumer/SourcePackages/checkouts/sentry-cocoa/Sources/Sentry/SentryFileManager.m", + "in_app": False, + }, + { + "function": "+[SentrySerialization appStateWithData:]", + "package": "/private/var/containers/Bundle/Application/CA061D22-C965-4C50-B383-59D8F14A6DDF/Sentry.app/Sentry", + "filename": "SentrySerialization.m", + "abs_path": "/Users/sentry/Library/Developer/Xcode/DerivedData/Consumer/SourcePackages/checkouts/sentry-cocoa/Sources/Sentry/SentrySerialization.m", + "in_app": False, + }, + { + "function": "-[SentryAppState initWithJSONObject:]", + "package": "/private/var/containers/Bundle/Application/CA061D22-C965-4C50-B383-59D8F14A6DDF/Sentry.app/Sentry", + "filename": "SentryAppState.m", + "abs_path": "/Users/sentry/Library/Developer/Xcode/DerivedData/Consumer/SourcePackages/checkouts/sentry-cocoa/Sources/Sentry/SentryAppState.m", + "in_app": False, + }, + { + "function": "+[NSDate(SentryExtras) sentry_fromIso8601String:]", + "package": "/private/var/containers/Bundle/Application/CA061D22-C965-4C50-B383-59D8F14A6DDF/Sentry.app/Sentry", + "filename": "NSDate+SentryExtras.m", + "abs_path": "/Users/sentry/Library/Developer/Xcode/DerivedData/Consumer/SourcePackages/checkouts/sentry-cocoa/Sources/Sentry/NSDate+SentryExtras.m", + "in_app": True, + }, + { + "function": "-[NSDateFormatter getObjectValue:forString:errorDescription:]", + "package": "/System/Library/Frameworks/Foundation.framework/Foundation", + "in_app": False, + }, + { + "function": "CFDateFormatterGetAbsoluteTimeFromString", + "package": "/System/Library/Frameworks/CoreFoundation.framework/CoreFoundation", + "in_app": False, + }, + { + "function": "__cficu_ucal_clear", + "package": "/System/Library/Frameworks/CoreFoundation.framework/CoreFoundation", + "in_app": False, + }, + { + "function": "icu::Calendar::clear", + "raw_function": "icu::Calendar::clear()", + "package": "/usr/lib/libicucore.A.dylib", + "in_app": False, + }, + ] + + event = get_crash_event_with_frames(frames) + + self.execute_test(event, True, mock_sdk_crash_reporter) + + reported_event_data = mock_sdk_crash_reporter.report.call_args.args[0] + actual_frames = get_path( + reported_event_data, "exception", "values", -1, "stacktrace", "frames" + ) + assert actual_frames == [ + { + "function": "_dispatch_workloop_worker_thread", + "package": "/usr/lib/system/libdispatch.dylib", + "in_app": False, + }, + { + "function": "_dispatch_lane_serial_drain$VARIANT$armv81", + "package": "/usr/lib/system/libdispatch.dylib", + "in_app": False, + }, + { + "function": "__44-[MXMetricManager deliverDiagnosticPayload:]_block_invoke", + "package": "/System/Library/Frameworks/MetricKit.framework/MetricKit", + "in_app": False, + }, + { + "function": "SentryMXManager.didReceive", + "raw_function": "closure #1 (MXDiagnosticPayload) in SentryMXManager.didReceive([MXDiagnosticPayload])", + "package": "Sentry.framework", + "abs_path": "Sentry.framework", + "in_app": True, + }, + { + "function": "SentryMXManager.didReceive", + "raw_function": "closure #1 (SentryMXCallStackTree) in closure #3 (MXCPUExceptionDiagnostic) in closure #1 (MXDiagnosticPayload) in SentryMXManager.didReceive([MXDiagnosticPayload])", + "package": "Sentry.framework", + "abs_path": "Sentry.framework", + "in_app": True, + }, + { + "function": "-[SentryMetricKitIntegration captureEventNotPerThread:params:]", + "package": "Sentry.framework", + "abs_path": "Sentry.framework", + "in_app": True, + }, + { + "function": "+[SentrySDK captureEvent:]", + "package": "Sentry.framework", + "abs_path": "Sentry.framework", + "in_app": True, + }, + { + "function": "-[SentryFileManager readAppStateFrom:]", + "package": "Sentry.framework", + "abs_path": "Sentry.framework", + "in_app": True, + }, + { + "function": "+[SentrySerialization appStateWithData:]", + "package": "Sentry.framework", + "abs_path": "Sentry.framework", + "in_app": True, + }, + { + "function": "-[SentryAppState initWithJSONObject:]", + "package": "Sentry.framework", + "abs_path": "Sentry.framework", + "in_app": True, + }, + { + "function": "+[NSDate(SentryExtras) sentry_fromIso8601String:]", + "package": "Sentry.framework", + "abs_path": "Sentry.framework", + "in_app": True, + }, + { + "function": "-[NSDateFormatter getObjectValue:forString:errorDescription:]", + "package": "/System/Library/Frameworks/Foundation.framework/Foundation", + "in_app": False, + }, + { + "function": "CFDateFormatterGetAbsoluteTimeFromString", + "package": "/System/Library/Frameworks/CoreFoundation.framework/CoreFoundation", + "in_app": False, + }, + { + "function": "__cficu_ucal_clear", + "package": "/System/Library/Frameworks/CoreFoundation.framework/CoreFoundation", + "in_app": False, + }, + { + "function": "icu::Calendar::clear", + "raw_function": "icu::Calendar::clear()", + "package": "/usr/lib/libicucore.A.dylib", + "in_app": False, + }, + ] + @patch("sentry.utils.sdk_crashes.sdk_crash_detection.sdk_crash_detection.sdk_crash_reporter") class CococaSDKFunctionTestMixin(BaseSDKCrashDetectionMixin): @@ -128,6 +343,34 @@ def test_sentryswizzle_reported(self, mock_sdk_crash_reporter): mock_sdk_crash_reporter, ) + def test_sentry_date_category_reported(self, mock_sdk_crash_reporter): + self.execute_test( + get_crash_event(function="+[NSDate(SentryExtras) sentry_fromIso8601String:]"), + True, + mock_sdk_crash_reporter, + ) + + def test_sentry_ns_data_category_reported(self, mock_sdk_crash_reporter): + self.execute_test( + get_crash_event(function="-[NSData(Sentry) sentry_nullTerminated:]"), + True, + mock_sdk_crash_reporter, + ) + + def test_sentry_swift_metric_kit_reported(self, mock_sdk_crash_reporter): + self.execute_test( + get_crash_event(function="SentryMXManager.didReceive"), + True, + mock_sdk_crash_reporter, + ) + + def test_sentry_swift_wrong_metric_kit_not_reported(self, mock_sdk_crash_reporter): + self.execute_test( + get_crash_event(function="SentryManager.didReceive"), + False, + mock_sdk_crash_reporter, + ) + def test_sentrycrash_crash_reported(self, mock_sdk_crash_reporter): self.execute_test( get_crash_event(function="-[SentryCrash crash]"), @@ -201,7 +444,7 @@ def _get_crash_event(self, filename) -> Dict[str, Collection[str]]: { "function": "_objc_terminate", "symbol": "_ZL15_objc_terminatev", - "package": "libobjc.A.dylib", + "package": "/usr/lib/system/libobjc.A.dylib", "in_app": False, "image_addr": "0x1a4e8f000", }, @@ -217,7 +460,7 @@ def _get_crash_event(self, filename) -> Dict[str, Collection[str]]: { "function": "std::__terminate", "symbol": "_ZSt11__terminatePFvvE", - "package": "libc++abi.dylib", + "package": "/usr/lib/system/libc++abi.dylib", "in_app": False, "image_addr": "0x1a4e8f000", }, @@ -270,7 +513,7 @@ def test_frames_only_non_in_app_after_sentry_frame_is_reported(self, mock_sdk_cr { "function": "std::__terminate", "symbol": "_ZSt11__terminatePFvvE", - "package": "libc++abi.dylib", + "package": "/usr/lib/system/libc++abi.dylib", "in_app": False, "image_addr": "0x1a4e8f000", }, @@ -287,7 +530,7 @@ def test_frames_only_in_app_after_sentry_frame_not_reported(self, mock_sdk_crash { "function": "std::__terminate", "symbol": "_ZSt11__terminatePFvvE", - "package": "libc++abi.dylib", + "package": "/usr/lib/system/libc++abi.dylib", "in_app": False, }, get_sentry_frame("sentrycrashdl_getBinaryImage"),