From 265f000be04e395d238803241c146b97a5b04138 Mon Sep 17 00:00:00 2001 From: Philipp Hofmann Date: Tue, 30 Apr 2024 17:24:56 +0200 Subject: [PATCH] ref: Ignore more thread sanitizer warnings in code (#3923) Ignore the thread sanitizer in the code where possible, which has the advantage of knowing a method is ignored when reading the code and not jumping to the ThreadSanitizer.sup file. Furthermore, use the ThreadSanitizer.sup file not only for tests but for the Run Xcode schemes. --- Sentry.xcodeproj/project.pbxproj | 2 +- .../xcshareddata/xcschemes/Sentry.xcscheme | 9 +++- .../TestSentryDispatchQueueWrapper.swift | 11 +++- Sources/Resources/ThreadSanitizer.sup | 11 ++++ Sources/Sentry/SentryDependencyContainer.m | 53 +++++++++++++------ Sources/Sentry/SentryFramesTracker.m | 22 +++----- Sources/Sentry/SentryTracer.m | 4 +- .../Sentry/include/SentryInternalCDefines.h | 13 +++++ .../Sentry/include/SentryInternalDefines.h | 13 ----- .../SentryCrashMonitor_MachException.c | 3 +- .../Recording/SentryCrashCachedData.c | 6 ++- Tests/SentryTests/SentryCrash/TestThread.m | 3 +- develop-docs/README.md | 2 +- 13 files changed, 99 insertions(+), 53 deletions(-) create mode 100644 Sources/Resources/ThreadSanitizer.sup diff --git a/Sentry.xcodeproj/project.pbxproj b/Sentry.xcodeproj/project.pbxproj index e89bbb63916..716ee9b912e 100644 --- a/Sentry.xcodeproj/project.pbxproj +++ b/Sentry.xcodeproj/project.pbxproj @@ -2410,7 +2410,6 @@ isa = PBXGroup; children = ( 844DA7F6282435CD00E6B62E /* README.md */, - 7B9660B12783500E0014A767 /* ThreadSanitizer.sup */, 630C01951EC341D600C52CEF /* Resources */, 63AA76AA1EB9D5CD00D153DE /* Configuration */, 63AA75931EB8AEDB00D153DE /* SentryTests */, @@ -3728,6 +3727,7 @@ D8B0542F2A7D35F10056BAF6 /* Resources */ = { isa = PBXGroup; children = ( + 7B9660B12783500E0014A767 /* ThreadSanitizer.sup */, D8B0542D2A7D2C720056BAF6 /* PrivacyInfo.xcprivacy */, D8511F722BAC8F750015E6FD /* Sentry.modulemap */, ); diff --git a/Sentry.xcodeproj/xcshareddata/xcschemes/Sentry.xcscheme b/Sentry.xcodeproj/xcshareddata/xcschemes/Sentry.xcscheme index ff9e5062993..502c51e6134 100644 --- a/Sentry.xcodeproj/xcshareddata/xcschemes/Sentry.xcscheme +++ b/Sentry.xcodeproj/xcshareddata/xcschemes/Sentry.xcscheme @@ -40,7 +40,7 @@ @@ -159,6 +159,13 @@ ReferencedContainer = "container:Sentry.xcodeproj"> + + + + Void>() public var dispatchAsyncExecutesBlock = true public override func dispatchAsync(_ block: @escaping () -> Void) { - dispatchAsyncCalled += 1 - dispatchAsyncInvocations.record(block) + + dispatchAsyncLock.synchronized { + dispatchAsyncCalled += 1 + dispatchAsyncInvocations.record(block) + } + if dispatchAsyncExecutesBlock { block() } diff --git a/Sources/Resources/ThreadSanitizer.sup b/Sources/Resources/ThreadSanitizer.sup new file mode 100644 index 00000000000..80279890112 --- /dev/null +++ b/Sources/Resources/ThreadSanitizer.sup @@ -0,0 +1,11 @@ +# ThreadSanitizer suppressions file +# For syntax details, see https://github.com/google/sanitizers/wiki/ThreadSanitizerSuppressions + +# Races to fix +race:returnResponse +race:enableNetworkTracking +race:enableNetworkBreadcrumbs +race:disable +race:URLSessionDataTaskMock +race:getOriginalImplementation +race:SentrySpanContext diff --git a/Sources/Sentry/SentryDependencyContainer.m b/Sources/Sentry/SentryDependencyContainer.m index dcb9611b793..c89bdb34b05 100644 --- a/Sources/Sentry/SentryDependencyContainer.m +++ b/Sources/Sentry/SentryDependencyContainer.m @@ -5,6 +5,7 @@ #import "SentryDisplayLinkWrapper.h" #import "SentryExtraContextProvider.h" #import "SentryFileManager.h" +#import "SentryInternalCDefines.h" #import "SentryLog.h" #import "SentryNSProcessInfoWrapper.h" #import "SentryNSTimerFactory.h" @@ -120,7 +121,8 @@ - (SentryAppStateManager *)appStateManager } } -- (SentryCrashWrapper *)crashWrapper +- (SentryCrashWrapper *)crashWrapper SENTRY_DISABLE_THREAD_SANITIZER( + "double-checked lock produce false alarms") { if (_crashWrapper == nil) { @synchronized(sentryDependencyContainerLock) { @@ -132,7 +134,8 @@ - (SentryCrashWrapper *)crashWrapper return _crashWrapper; } -- (SentryCrash *)crashReporter +- (SentryCrash *)crashReporter SENTRY_DISABLE_THREAD_SANITIZER( + "double-checked lock produce false alarms") { if (_crashReporter == nil) { @synchronized(sentryDependencyContainerLock) { @@ -145,7 +148,8 @@ - (SentryCrash *)crashReporter return _crashReporter; } -- (SentrySysctl *)sysctlWrapper +- (SentrySysctl *)sysctlWrapper SENTRY_DISABLE_THREAD_SANITIZER( + "double-checked lock produce false alarms") { if (_sysctlWrapper == nil) { @synchronized(sentryDependencyContainerLock) { @@ -157,7 +161,8 @@ - (SentrySysctl *)sysctlWrapper return _sysctlWrapper; } -- (SentryThreadInspector *)threadInspector +- (SentryThreadInspector *)threadInspector SENTRY_DISABLE_THREAD_SANITIZER( + "double-checked lock produce false alarms") { if (_threadInspector == nil) { @synchronized(sentryDependencyContainerLock) { @@ -170,7 +175,8 @@ - (SentryThreadInspector *)threadInspector return _threadInspector; } -- (SentryExtraContextProvider *)extraContextProvider +- (SentryExtraContextProvider *)extraContextProvider SENTRY_DISABLE_THREAD_SANITIZER( + "double-checked lock produce false alarms") { if (_extraContextProvider == nil) { @synchronized(sentryDependencyContainerLock) { @@ -193,7 +199,8 @@ - (SentryNSNotificationCenterWrapper *)notificationCenterWrapper } #if TARGET_OS_IOS -- (SentryUIDeviceWrapper *)uiDeviceWrapper +- (SentryUIDeviceWrapper *)uiDeviceWrapper SENTRY_DISABLE_THREAD_SANITIZER( + "double-checked lock produce false alarms") { # if SENTRY_HAS_UIKIT if (_uiDeviceWrapper == nil) { @@ -214,7 +221,8 @@ - (SentryUIDeviceWrapper *)uiDeviceWrapper #endif // TARGET_OS_IOS #if SENTRY_UIKIT_AVAILABLE -- (SentryScreenshot *)screenshot +- (SentryScreenshot *)screenshot SENTRY_DISABLE_THREAD_SANITIZER( + "double-checked lock produce false alarms") { # if SENTRY_HAS_UIKIT if (_screenshot == nil) { @@ -233,7 +241,8 @@ - (SentryScreenshot *)screenshot # endif // SENTRY_HAS_UIKIT } -- (SentryViewHierarchy *)viewHierarchy +- (SentryViewHierarchy *)viewHierarchy SENTRY_DISABLE_THREAD_SANITIZER( + "double-checked lock produce false alarms") { # if SENTRY_HAS_UIKIT if (_viewHierarchy == nil) { @@ -252,7 +261,8 @@ - (SentryViewHierarchy *)viewHierarchy # endif // SENTRY_HAS_UIKIT } -- (SentryUIApplication *)application +- (SentryUIApplication *)application SENTRY_DISABLE_THREAD_SANITIZER( + "double-checked lock produce false alarms") { # if SENTRY_HAS_UIKIT if (_application == nil) { @@ -271,7 +281,8 @@ - (SentryUIApplication *)application # endif // SENTRY_HAS_UIKIT } -- (SentryFramesTracker *)framesTracker +- (SentryFramesTracker *)framesTracker SENTRY_DISABLE_THREAD_SANITIZER( + "double-checked lock produce false alarms") { # if SENTRY_HAS_UIKIT if (_framesTracker == nil) { @@ -294,7 +305,8 @@ - (SentryFramesTracker *)framesTracker # endif // SENTRY_HAS_UIKIT } -- (SentrySwizzleWrapper *)swizzleWrapper +- (SentrySwizzleWrapper *)swizzleWrapper SENTRY_DISABLE_THREAD_SANITIZER( + "double-checked lock produce false alarms") { # if SENTRY_HAS_UIKIT if (_swizzleWrapper == nil) { @@ -315,6 +327,7 @@ - (SentrySwizzleWrapper *)swizzleWrapper #endif // SENTRY_UIKIT_AVAILABLE - (SentryANRTracker *)getANRTracker:(NSTimeInterval)timeout + SENTRY_DISABLE_THREAD_SANITIZER("double-checked lock produce false alarms") { if (_anrTracker == nil) { @synchronized(sentryDependencyContainerLock) { @@ -331,7 +344,8 @@ - (SentryANRTracker *)getANRTracker:(NSTimeInterval)timeout return _anrTracker; } -- (SentryNSProcessInfoWrapper *)processInfoWrapper +- (SentryNSProcessInfoWrapper *)processInfoWrapper SENTRY_DISABLE_THREAD_SANITIZER( + "double-checked lock produce false alarms") { if (_processInfoWrapper == nil) { @synchronized(sentryDependencyContainerLock) { @@ -343,7 +357,8 @@ - (SentryNSProcessInfoWrapper *)processInfoWrapper return _processInfoWrapper; } -- (SentrySystemWrapper *)systemWrapper +- (SentrySystemWrapper *)systemWrapper SENTRY_DISABLE_THREAD_SANITIZER( + "double-checked lock produce false alarms") { if (_systemWrapper == nil) { @synchronized(sentryDependencyContainerLock) { @@ -355,7 +370,8 @@ - (SentrySystemWrapper *)systemWrapper return _systemWrapper; } -- (SentryDispatchFactory *)dispatchFactory +- (SentryDispatchFactory *)dispatchFactory SENTRY_DISABLE_THREAD_SANITIZER( + "double-checked lock produce false alarms") { if (_dispatchFactory == nil) { @synchronized(sentryDependencyContainerLock) { @@ -367,7 +383,8 @@ - (SentryDispatchFactory *)dispatchFactory return _dispatchFactory; } -- (SentryNSTimerFactory *)timerFactory +- (SentryNSTimerFactory *)timerFactory SENTRY_DISABLE_THREAD_SANITIZER( + "double-checked lock produce false alarms") { if (_timerFactory == nil) { @synchronized(sentryDependencyContainerLock) { @@ -380,7 +397,8 @@ - (SentryNSTimerFactory *)timerFactory } #if SENTRY_HAS_METRIC_KIT -- (SentryMXManager *)metricKitManager +- (SentryMXManager *)metricKitManager SENTRY_DISABLE_THREAD_SANITIZER( + "double-checked lock produce false alarms") { if (_metricKitManager == nil) { @synchronized(sentryDependencyContainerLock) { @@ -399,7 +417,8 @@ - (SentryMXManager *)metricKitManager #endif // SENTRY_HAS_METRIC_KIT #if SENTRY_HAS_REACHABILITY -- (SentryReachability *)reachability +- (SentryReachability *)reachability SENTRY_DISABLE_THREAD_SANITIZER( + "double-checked lock produce false alarms") { if (_reachability == nil) { @synchronized(sentryDependencyContainerLock) { diff --git a/Sources/Sentry/SentryFramesTracker.m b/Sources/Sentry/SentryFramesTracker.m index 0e0ffc82d08..da8df5796b5 100644 --- a/Sources/Sentry/SentryFramesTracker.m +++ b/Sources/Sentry/SentryFramesTracker.m @@ -7,7 +7,7 @@ # import "SentryDelayedFramesTracker.h" # import "SentryDispatchQueueWrapper.h" # import "SentryDisplayLinkWrapper.h" -# import "SentryInternalDefines.h" +# import "SentryInternalCDefines.h" # import "SentryLog.h" # import "SentryProfiler+Private.h" # import "SentryProfilingConditionals.h" @@ -87,22 +87,16 @@ - (void)setDisplayLinkWrapper:(SentryDisplayLinkWrapper *)displayLinkWrapper { _displayLinkWrapper = displayLinkWrapper; } - -/** - * As you can only disable the thread sanitizer for methods, we must manually create the setter - * here. - */ - (void)setPreviousFrameSystemTimestamp:(uint64_t)previousFrameSystemTimestamp - SENTRY_DISABLE_THREAD_SANITIZER + SENTRY_DISABLE_THREAD_SANITIZER("As you can only disable the thread sanitizer for methods, we " + "must manually create the setter here.") { _previousFrameSystemTimestamp = previousFrameSystemTimestamp; } -/** - * As you can only disable the thread sanitizer for methods, we must manually create the getter - * here. - */ -- (uint64_t)getPreviousFrameSystemTimestamp SENTRY_DISABLE_THREAD_SANITIZER +- (uint64_t)getPreviousFrameSystemTimestamp SENTRY_DISABLE_THREAD_SANITIZER( + "As you can only disable the thread sanitizer for methods, we must manually create the getter " + "here.") { return _previousFrameSystemTimestamp; } @@ -251,7 +245,7 @@ - (void)recordTimestamp:(uint64_t)timestamp value:(NSNumber *)value array:(NSMut } # endif // SENTRY_TARGET_PROFILING_SUPPORTED -- (SentryScreenFrames *)currentFrames SENTRY_DISABLE_THREAD_SANITIZER +- (SentryScreenFrames *)currentFrames SENTRY_DISABLE_THREAD_SANITIZER() { # if SENTRY_TARGET_PROFILING_SUPPORTED return [[SentryScreenFrames alloc] initWithTotal:_totalFrames @@ -268,7 +262,7 @@ - (SentryScreenFrames *)currentFrames SENTRY_DISABLE_THREAD_SANITIZER } - (CFTimeInterval)getFramesDelay:(uint64_t)startSystemTimestamp - endSystemTimestamp:(uint64_t)endSystemTimestamp SENTRY_DISABLE_THREAD_SANITIZER + endSystemTimestamp:(uint64_t)endSystemTimestamp SENTRY_DISABLE_THREAD_SANITIZER() { return [self.delayedFramesTracker getFramesDelay:startSystemTimestamp endSystemTimestamp:endSystemTimestamp diff --git a/Sources/Sentry/SentryTracer.m b/Sources/Sentry/SentryTracer.m index bec0d59c788..5145392c2e7 100644 --- a/Sources/Sentry/SentryTracer.m +++ b/Sources/Sentry/SentryTracer.m @@ -5,6 +5,7 @@ #import "SentryEvent+Private.h" #import "SentryFileManager.h" #import "SentryHub+Private.h" +#import "SentryInternalCDefines.h" #import "SentryInternalDefines.h" #import "SentryLog.h" #import "SentryNSDictionarySanitize.h" @@ -745,7 +746,8 @@ - (SentryTransaction *)toTransaction #if SENTRY_HAS_UIKIT -- (nullable SentryAppStartMeasurement *)getAppStartMeasurement +- (nullable SentryAppStartMeasurement *)getAppStartMeasurement SENTRY_DISABLE_THREAD_SANITIZER( + "double-checked lock produce false alarms") { // Only send app start measurement for transactions generated by auto performance // instrumentation. diff --git a/Sources/Sentry/include/SentryInternalCDefines.h b/Sources/Sentry/include/SentryInternalCDefines.h index 47c21f81fb4..67fd7d76cba 100644 --- a/Sources/Sentry/include/SentryInternalCDefines.h +++ b/Sources/Sentry/include/SentryInternalCDefines.h @@ -1 +1,14 @@ typedef unsigned long long bytes; + +/** + * For disabling the thread sanitizer for a method + */ +#if defined(__has_feature) +# if __has_feature(thread_sanitizer) +# define SENTRY_DISABLE_THREAD_SANITIZER(message) __attribute__((no_sanitize("thread"))) +# else +# define SENTRY_DISABLE_THREAD_SANITIZER(message) +# endif +#else +# define SENTRY_DISABLE_THREAD_SANITIZER(message) +#endif diff --git a/Sources/Sentry/include/SentryInternalDefines.h b/Sources/Sentry/include/SentryInternalDefines.h index d8e4161afaf..7a528138ae0 100644 --- a/Sources/Sentry/include/SentryInternalDefines.h +++ b/Sources/Sentry/include/SentryInternalDefines.h @@ -66,16 +66,3 @@ static NSString *const SentryPlatformName = @"cocoa"; #define SPAN_DATA_BLOCKED_MAIN_THREAD @"blocked_main_thread" #define SPAN_DATA_THREAD_ID @"thread.id" #define SPAN_DATA_THREAD_NAME @"thread.name" - -/** - * For disabling the thread sanitizer for a method - */ -#if defined(__has_feature) -# if __has_feature(thread_sanitizer) -# define SENTRY_DISABLE_THREAD_SANITIZER __attribute__((no_sanitize("thread"))) -# else -# define SENTRY_DISABLE_THREAD_SANITIZER -# endif -#else -# define SENTRY_DISABLE_THREAD_SANITIZER -#endif diff --git a/Sources/SentryCrash/Recording/Monitors/SentryCrashMonitor_MachException.c b/Sources/SentryCrash/Recording/Monitors/SentryCrashMonitor_MachException.c index 5a3dced3258..f66f55c8619 100644 --- a/Sources/SentryCrash/Recording/Monitors/SentryCrashMonitor_MachException.c +++ b/Sources/SentryCrash/Recording/Monitors/SentryCrashMonitor_MachException.c @@ -32,6 +32,7 @@ #include "SentryCrashStackCursor_MachineContext.h" #include "SentryCrashSystemCapabilities.h" #include "SentryCrashThread.h" +#include "SentryInternalCDefines.h" // #define SentryCrashLogger_LocalLevel TRACE #include "SentryCrashLogger.h" @@ -409,7 +410,7 @@ handleExceptions(void *const userData) // ============================================================================ static void -uninstallExceptionHandler(void) +uninstallExceptionHandler(void) SENTRY_DISABLE_THREAD_SANITIZER("Known data race to fix") { SentryCrashLOG_DEBUG("Uninstalling mach exception handler."); diff --git a/Sources/SentryCrash/Recording/SentryCrashCachedData.c b/Sources/SentryCrash/Recording/SentryCrashCachedData.c index 5302eb41b51..e366495ce3c 100644 --- a/Sources/SentryCrash/Recording/SentryCrashCachedData.c +++ b/Sources/SentryCrash/Recording/SentryCrashCachedData.c @@ -24,6 +24,7 @@ // #include "SentryCrashCachedData.h" +#include "SentryInternalCDefines.h" // #define SentryCrashLogger_LocalLevel TRACE #include "SentryCrashLogger.h" @@ -54,7 +55,7 @@ static _Atomic(int) g_semaphoreCount; static bool g_hasThreadStarted = false; static void -updateThreadList(void) +updateThreadList(void) SENTRY_DISABLE_THREAD_SANITIZER("Known data race to fix") { const task_t thisTask = mach_task_self(); int oldThreadsCount = g_allThreadsCount; @@ -129,6 +130,7 @@ updateThreadList(void) static void * monitorCachedData(__unused void *const userData) + SENTRY_DISABLE_THREAD_SANITIZER("Known data race to fix") { static int quickPollCount = 4; usleep(1); @@ -149,6 +151,7 @@ monitorCachedData(__unused void *const userData) void sentrycrashccd_init(int pollingIntervalInSeconds) + SENTRY_DISABLE_THREAD_SANITIZER("Known data race to fix") { if (g_hasThreadStarted == true) { return; @@ -211,6 +214,7 @@ sentrycrashccd_getAllThreads(int *threadCount) const char * sentrycrashccd_getThreadName(SentryCrashThread thread) + SENTRY_DISABLE_THREAD_SANITIZER("Known data race to fix") { if (g_allThreadNames != NULL) { for (int i = 0; i < g_allThreadsCount; i++) { diff --git a/Tests/SentryTests/SentryCrash/TestThread.m b/Tests/SentryTests/SentryCrash/TestThread.m index f8dac86c3d4..7bfbc28e62e 100644 --- a/Tests/SentryTests/SentryCrash/TestThread.m +++ b/Tests/SentryTests/SentryCrash/TestThread.m @@ -27,12 +27,13 @@ #import "TestThread.h" #import "SentryCrashThread.h" +#import "SentryInternalCDefines.h" @implementation TestThread @synthesize thread = _thread; -- (void)main +- (void)main SENTRY_DISABLE_THREAD_SANITIZER("Known data race to fix") { self.thread = (thread_t)sentrycrashthread_self(); [NSNotificationCenter.defaultCenter postNotificationName:@"io.sentry.test.TestThread.main" diff --git a/develop-docs/README.md b/develop-docs/README.md index 3ae7e7bea17..4557afe12bc 100644 --- a/develop-docs/README.md +++ b/develop-docs/README.md @@ -39,7 +39,7 @@ Reach out to a [CODEOWNER](https://github.com/getsentry/sentry-cocoa/blob/main/. ## Unit Tests with Thread Sanitizer CI runs the unit tests for one job with thread sanitizer enabled to detect race conditions. -The Test scheme of Sentry uses `TSAN_OPTIONS` to specify the [suppression file](../Tests/ThreadSanitizer.sup) to ignore false positives or known issues. +To ignore false positives or known issues, use the `SENTRY_DISABLE_THREAD_SANITIZER` macro or the [suppression file](../Sources/Resources/ThreadSanitizer.sup). It's worth noting that you can use the `$(PROJECT_DIR)` to specify the path to the suppression file. To run the unit tests with the thread sanitizer enabled in Xcode click on edit scheme, go to tests, then open diagnostics, and enable Thread Sanitizer. The profiler doesn't work with TSAN attached, so tests that run the profiler will be skipped.