fix: Crash in Client when reading integrations (#2398)

Fix reading from the integrations list while it's being modified
on another thread on the client.

Fixes GH-2397

CHANGELOG.md

@@ -4,6 +4,7 @@
 
 ### Fixes
 
+- Crash in Client when reading integrations (#2398)
 - Don't update session for dropped events (#2374)
 
 ## 7.31.1

Sentry.xcodeproj/project.pbxproj

@@ -40,8 +40,8 @@
 		0A1B497328E597DD00D7BFA3 /* TestLogOutput.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0A1B497228E597DD00D7BFA3 /* TestLogOutput.swift */; };
 		0A1C3592287D7107007D01E3 /* SentryMetaTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0A1C3591287D7107007D01E3 /* SentryMetaTests.swift */; };
 		0A2690B72885C2E000E4432D /* TestSentryPermissionsObserver.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0AABE2EF2885C2120057ED69 /* TestSentryPermissionsObserver.swift */; };
-		0A2D7BBA29152CBF008727AF /* SentryOutOfMemoryScopeObserverTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0A2D7BB929152CBF008727AF /* SentryOutOfMemoryScopeObserverTests.swift */; };
 		0A283E79291A67E000EF4126 /* SentryUIDeviceWrapperTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0A283E78291A67E000EF4126 /* SentryUIDeviceWrapperTests.swift */; };
+		0A2D7BBA29152CBF008727AF /* SentryOutOfMemoryScopeObserverTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0A2D7BB929152CBF008727AF /* SentryOutOfMemoryScopeObserverTests.swift */; };
 		0A2D8D5B289815C0008720F6 /* SentryBaseIntegration.m in Sources */ = {isa = PBXBuildFile; fileRef = 0A2D8D5A289815C0008720F6 /* SentryBaseIntegration.m */; };
 		0A2D8D5D289815EB008720F6 /* SentryBaseIntegration.h in Headers */ = {isa = PBXBuildFile; fileRef = 0A2D8D5C289815EB008720F6 /* SentryBaseIntegration.h */; };
 		0A2D8D8728992260008720F6 /* SentryBaseIntegrationTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0A2D8D8628992260008720F6 /* SentryBaseIntegrationTests.swift */; };
@@ -466,6 +466,7 @@
 		7BB654FB253DC14A00887E87 /* SentryUserFeedback.h in Headers */ = {isa = PBXBuildFile; fileRef = 7BB654FA253DC14A00887E87 /* SentryUserFeedback.h */; settings = {ATTRIBUTES = (Public, ); }; };
 		7BB65501253DC1B500887E87 /* SentryUserFeedback.m in Sources */ = {isa = PBXBuildFile; fileRef = 7BB65500253DC1B500887E87 /* SentryUserFeedback.m */; };
 		7BB6550D253EEB3900887E87 /* SentryUserFeedbackTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 7BB6550C253EEB3900887E87 /* SentryUserFeedbackTests.swift */; };
+		7BB7E7C729267A28004BF96B /* EmptyIntegration.swift in Sources */ = {isa = PBXBuildFile; fileRef = 7BB7E7C629267A28004BF96B /* EmptyIntegration.swift */; };
 		7BBC826D25DFCFDE005F1ED8 /* SentryInAppLogic.h in Headers */ = {isa = PBXBuildFile; fileRef = 7BBC826C25DFCFDE005F1ED8 /* SentryInAppLogic.h */; };
 		7BBC827125DFD039005F1ED8 /* SentryInAppLogic.m in Sources */ = {isa = PBXBuildFile; fileRef = 7BBC827025DFD039005F1ED8 /* SentryInAppLogic.m */; };
 		7BBC827925DFD7D7005F1ED8 /* SentryInAppLogicTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 7BBC827825DFD7D7005F1ED8 /* SentryInAppLogicTests.swift */; };
@@ -773,8 +774,8 @@
 		03F9D37B2819A65C00602916 /* SentryProfilerTests.mm */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.objcpp; path = SentryProfilerTests.mm; sourceTree = "<group>"; };
 		0A1B497228E597DD00D7BFA3 /* TestLogOutput.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = TestLogOutput.swift; sourceTree = "<group>"; };
 		0A1C3591287D7107007D01E3 /* SentryMetaTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SentryMetaTests.swift; sourceTree = "<group>"; };
-		0A2D7BB929152CBF008727AF /* SentryOutOfMemoryScopeObserverTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SentryOutOfMemoryScopeObserverTests.swift; sourceTree = "<group>"; };
 		0A283E78291A67E000EF4126 /* SentryUIDeviceWrapperTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SentryUIDeviceWrapperTests.swift; sourceTree = "<group>"; };
+		0A2D7BB929152CBF008727AF /* SentryOutOfMemoryScopeObserverTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SentryOutOfMemoryScopeObserverTests.swift; sourceTree = "<group>"; };
 		0A2D8D5A289815C0008720F6 /* SentryBaseIntegration.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = SentryBaseIntegration.m; sourceTree = "<group>"; };
 		0A2D8D5C289815EB008720F6 /* SentryBaseIntegration.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = SentryBaseIntegration.h; path = include/SentryBaseIntegration.h; sourceTree = "<group>"; };
 		0A2D8D8628992260008720F6 /* SentryBaseIntegrationTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SentryBaseIntegrationTests.swift; sourceTree = "<group>"; };
@@ -1225,6 +1226,7 @@
 		7BB654FA253DC14A00887E87 /* SentryUserFeedback.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = SentryUserFeedback.h; path = Public/SentryUserFeedback.h; sourceTree = "<group>"; };
 		7BB65500253DC1B500887E87 /* SentryUserFeedback.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = SentryUserFeedback.m; sourceTree = "<group>"; };
 		7BB6550C253EEB3900887E87 /* SentryUserFeedbackTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SentryUserFeedbackTests.swift; sourceTree = "<group>"; };
+		7BB7E7C629267A28004BF96B /* EmptyIntegration.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = EmptyIntegration.swift; sourceTree = "<group>"; };
 		7BBC826C25DFCFDE005F1ED8 /* SentryInAppLogic.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = SentryInAppLogic.h; path = include/SentryInAppLogic.h; sourceTree = "<group>"; };
 		7BBC827025DFD039005F1ED8 /* SentryInAppLogic.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = SentryInAppLogic.m; sourceTree = "<group>"; };
 		7BBC827825DFD7D7005F1ED8 /* SentryInAppLogicTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SentryInAppLogicTests.swift; sourceTree = "<group>"; };
@@ -2736,6 +2738,7 @@
 				7BF1F6AC282A4FC6006BD6AB /* SentryTestObserver.h */,
 				7BF1F6AD282A4FE2006BD6AB /* SentryTestObserver.m */,
 				7B72D23928D074BC0014798A /* TestExtensions.swift */,
+				7BB7E7C629267A28004BF96B /* EmptyIntegration.swift */,
 			);
 			path = TestUtils;
 			sourceTree = "<group>";
@@ -3755,6 +3758,7 @@
 				8E70B10125CB8695002B3155 /* SentrySpanIdTests.swift in Sources */,
 				7BFE7A0A27A1B6B000D2B66E /* SentryOutOfMemoryIntegrationTests.swift in Sources */,
 				7BA61CAF247BBF3C00C130A8 /* SentryDebugImageProviderTests.swift in Sources */,
+				7BB7E7C729267A28004BF96B /* EmptyIntegration.swift in Sources */,
 				7B965728268321CD00C66E25 /* SentryCrashScopeObserverTests.swift in Sources */,
 				7BD86ECB264A6DB5005439DB /* TestSysctl.swift in Sources */,
 				035E73CA27D57398005EEB11 /* SentryThreadHandleTests.mm in Sources */,

Sources/Sentry/SentryClient.m

@@ -652,6 +652,7 @@ - (void)setSdk:(SentryEvent *)event
     id integrations = event.extra[@"__sentry_sdk_integrations"];
     if (!integrations) {
         integrations = [NSMutableArray new];
+
         for (NSString *integration in SentrySDK.currentHub.installedIntegrationNames) {
             // Every integration starts with "Sentry" and ends with "Integration". To keep the
             // payload of the event small we remove both.

Sources/Sentry/SentryHub.m

@@ -31,14 +31,14 @@
 @property (nonatomic, strong) SentryTracesSampler *tracesSampler;
 @property (nonatomic, strong) SentryProfilesSampler *profilesSampler;
 @property (nonatomic, strong) id<SentryCurrentDateProvider> currentDateProvider;
-@property (nonatomic, strong)
-    NSMutableArray<NSObject<SentryIntegrationProtocol> *> *installedIntegrations;
-@property (nonatomic, strong) NSMutableArray<NSString *> *installedIntegrationNames;
+@property (nonatomic, strong) NSMutableArray<id<SentryIntegrationProtocol>> *installedIntegrations;
+@property (nonatomic, strong) NSMutableSet<NSString *> *installedIntegrationNames;
 
 @end
 
 @implementation SentryHub {
     NSObject *_sessionLock;
+    NSObject *_integrationsLock;
 }
 
 - (instancetype)initWithClient:(nullable SentryClient *)client
@@ -48,8 +48,9 @@ - (instancetype)initWithClient:(nullable SentryClient *)client
         _client = client;
         _scope = scope;
         _sessionLock = [[NSObject alloc] init];
+        _integrationsLock = [[NSObject alloc] init];
         _installedIntegrations = [[NSMutableArray alloc] init];
-        _installedIntegrationNames = [[NSMutableArray alloc] init];
+        _installedIntegrationNames = [[NSMutableSet alloc] init];
         _crashWrapper = [SentryCrashWrapper sharedInstance];
         _tracesSampler = [[SentryTracesSampler alloc] initWithOptions:client.options];
 #if SENTRY_TARGET_PROFILING_SUPPORTED
@@ -539,17 +540,53 @@ - (void)configureScope:(void (^)(SentryScope *scope))callback
  */
 - (BOOL)isIntegrationInstalled:(Class)integrationClass
 {
-    for (id<SentryIntegrationProtocol> item in self.installedIntegrations) {
-        if ([item isKindOfClass:integrationClass]) {
-            return YES;
+    @synchronized(_integrationsLock) {
+        for (id<SentryIntegrationProtocol> item in _installedIntegrations) {
+            if ([item isKindOfClass:integrationClass]) {
+                return YES;
+            }
         }
+        return NO;
     }
-    return NO;
 }
 
 - (BOOL)hasIntegration:(NSString *)integrationName
 {
-    return [self.installedIntegrationNames containsObject:integrationName];
+    // installedIntegrations and installedIntegrationNames share the same lock.
+    // Instead of creating an extra lock object, we use _installedIntegrations.
+    @synchronized(_integrationsLock) {
+        return [_installedIntegrationNames containsObject:integrationName];
+    }
+}
+
+- (void)addInstalledIntegration:(id<SentryIntegrationProtocol>)integration name:(NSString *)name
+{
+    @synchronized(_integrationsLock) {
+        [_installedIntegrations addObject:integration];
+        [_installedIntegrationNames addObject:name];
+    }
+}
+
+- (void)removeAllIntegrations
+{
+    @synchronized(_integrationsLock) {
+        [_installedIntegrations removeAllObjects];
+        [_installedIntegrationNames removeAllObjects];
+    }
+}
+
+- (NSArray<id<SentryIntegrationProtocol>> *)installedIntegrations
+{
+    @synchronized(_integrationsLock) {
+        return _installedIntegrations.copy;
+    }
+}
+
+- (NSSet<NSString *> *)installedIntegrationNames
+{
+    @synchronized(_integrationsLock) {
+        return _installedIntegrationNames.copy;
+    }
 }
 
 - (void)setUser:(nullable SentryUser *)user

Sources/Sentry/SentrySDK.m

@@ -387,10 +387,10 @@ + (void)installIntegrations
         }
         id<SentryIntegrationProtocol> integrationInstance = [[integrationClass alloc] init];
         BOOL shouldInstall = [integrationInstance installWithOptions:options];
+
         if (shouldInstall) {
             SENTRY_LOG_DEBUG(@"Integration installed: %@", integrationName);
-            [SentrySDK.currentHub.installedIntegrations addObject:integrationInstance];
-            [SentrySDK.currentHub.installedIntegrationNames addObject:integrationName];
+            [SentrySDK.currentHub addInstalledIntegration:integrationInstance name:integrationName];
         }
     }
 }
@@ -407,21 +407,22 @@ + (void)close
 {
     // pop the hub and unset
     SentryHub *hub = SentrySDK.currentHub;
-    [SentrySDK setCurrentHub:nil];
 
     // uninstall all the integrations
     for (NSObject<SentryIntegrationProtocol> *integration in hub.installedIntegrations) {
         if ([integration respondsToSelector:@selector(uninstall)]) {
             [integration uninstall];
         }
     }
-    [hub.installedIntegrations removeAllObjects];
+    [hub removeAllIntegrations];
 
     // close the client
     SentryClient *client = [hub getClient];
     client.options.enabled = NO;
     [hub bindClient:nil];
 
+    [SentrySDK setCurrentHub:nil];
+
     [SentryDependencyContainer reset];
 
     SENTRY_LOG_DEBUG(@"SDK closed!");

Sources/Sentry/include/SentryHub+Private.h

@@ -8,9 +8,11 @@ NS_ASSUME_NONNULL_BEGIN
 @interface
 SentryHub (Private)
 
-@property (nonatomic, strong)
-    NSMutableArray<NSObject<SentryIntegrationProtocol> *> *installedIntegrations;
-@property (nonatomic, strong) NSMutableArray<NSString *> *installedIntegrationNames;
+@property (nonatomic, strong) NSArray<id<SentryIntegrationProtocol>> *installedIntegrations;
+@property (nonatomic, strong) NSSet<NSString *> *installedIntegrationNames;
+
+- (void)addInstalledIntegration:(id<SentryIntegrationProtocol>)integration name:(NSString *)name;
+- (void)removeAllIntegrations;
 
 - (SentryClient *_Nullable)client;
 

Tests/SentryTests/SentryClientTests.swift

@@ -32,6 +32,7 @@ class SentryClientTest: XCTestCase {
         let deviceWrapper = TestSentryUIDeviceWrapper()
         let locale = Locale(identifier: "en_US")
         let timezone = TimeZone(identifier: "Europe/Vienna")!
+        let queue = DispatchQueue(label: "SentryHubTests", qos: .utility, attributes: [.concurrent])
 
         init() {
             session = SentrySession(releaseName: "release")
@@ -1291,6 +1292,41 @@ class SentryClientTest: XCTestCase {
         XCTAssertEqual(item, fixture.transportAdapter.sendEventWithTraceStateInvocations.first?.additionalEnvelopeItems.first)
     }
 
+    @available(iOS 10.0, tvOS 10.0, OSX 10.12, *)
+    func testConcurrentlyAddingInstalledIntegrations_WhileSendingEvents() {
+        let sut = fixture.getSut()
+
+        let hub = SentryHub(client: sut, andScope: nil)
+        SentrySDK.setCurrentHub(hub)
+
+        func addIntegrations(amount: Int) {
+            let emptyIntegration = EmptyIntegration()
+            for i in 0..<amount {
+                hub.addInstalledIntegration(emptyIntegration, name: "Integration\(i)")
+            }
+        }
+
+        // So that the loop in Client.setSDK overlaps with addingIntegrations
+        addIntegrations(amount: 1_000)
+
+        let queue = fixture.queue
+        let group = DispatchGroup()
+
+        // Run this in a loop to ensure that add while iterating over the integrations
+        // Running it once doesn't guaranty failure
+        for _ in 0..<10 {
+            group.enter()
+            queue.async {
+                addIntegrations(amount: 1_000)
+                group.leave()
+            }
+
+            sut.capture(event: Event())
+            group.waitWithTimeout()
+            hub.removeAllIntegrations()
+        }
+    }
+
     private func givenEventWithDebugMeta() -> Event {
         let event = Event(level: SentryLevel.fatal)
         let debugMeta = DebugMeta()

Tests/SentryTests/SentryHubTests.swift

@@ -22,6 +22,7 @@ class SentryHubTests: XCTestCase {
         let transactionName = "Some Transaction"
         let transactionOperation = "Some Operation"
         let random = TestRandom(value: 0.5)
+        let queue = DispatchQueue(label: "SentryHubTests", qos: .utility, attributes: [.concurrent])
 
         init() {
             options = Options()
@@ -673,8 +674,7 @@ class SentryHubTests: XCTestCase {
         let sut = fixture.getSut()
         sut.startSession()
 
-        let queue = DispatchQueue(label: "SentryHubTests", qos: .utility, attributes: [.concurrent])
-
+        let queue = fixture.queue
         let group = DispatchGroup()
         for _ in 0..<count {
             group.enter()
@@ -687,6 +687,69 @@ class SentryHubTests: XCTestCase {
         group.waitWithTimeout()
     }
 
+    @available(iOS 10.0, tvOS 10.0, OSX 10.12, *)
+    func testModifyIntegrationsConcurrently() {
+
+        let sut = fixture.getSut()
+
+        let outerLoopAmount = 10
+        let innerLoopAmount = 100
+
+        let queue = fixture.queue
+        let group = DispatchGroup()
+
+        for i in 0..<outerLoopAmount {
+            group.enter()
+            queue.async {
+                for j in 0..<innerLoopAmount {
+                    let integrationName = "Integration\(i)\(j)"
+                    sut.addInstalledIntegration(EmptyIntegration(), name: integrationName)
+                    XCTAssertTrue(sut.hasIntegration(integrationName))
+                }
+                group.leave()
+            }
+        }
+
+        group.waitWithTimeout()
+
+        XCTAssertEqual(innerLoopAmount * outerLoopAmount, sut.installedIntegrations.count)
+        XCTAssertEqual(innerLoopAmount * outerLoopAmount, sut.installedIntegrationNames.count)
+
+    }
+
+    /**
+     * This test only ensures concurrent modifications don't crash.
+     */
+    @available(iOS 10.0, tvOS 10.0, OSX 10.12, *)
+    func testModifyIntegrationsConcurrently_NoCrash() {
+        let sut = fixture.getSut()
+
+        let queue = fixture.queue
+        let group = DispatchGroup()
+
+        for i in 0..<1_000 {
+            group.enter()
+            queue.async {
+                for j in 0..<10 {
+                    let integrationName = "Integration\(i)\(j)"
+                    sut.addInstalledIntegration(EmptyIntegration(), name: integrationName)
+                    sut.hasIntegration(integrationName)
+                    sut.isIntegrationInstalled(EmptyIntegration.self)
+                }
+                XCTAssertLessThanOrEqual(0, sut.installedIntegrations.count)
+                sut.installedIntegrations.forEach { XCTAssertNotNil($0) }
+
+                XCTAssertLessThanOrEqual(0, sut.installedIntegrationNames.count)
+                sut.installedIntegrationNames.forEach { XCTAssertNotNil($0) }
+                sut.removeAllIntegrations()
+
+                group.leave()
+            }
+        }
+
+        group.wait()
+    }
+
     private func captureEventEnvelope(level: SentryLevel) {
         let event = TestData.event
         event.level = level

Tests/SentryTests/SentrySDKTests.swift

@@ -353,7 +353,7 @@ class SentrySDKTests: XCTestCase {
         SentrySDK.start(options: options)
 
         assertIntegrationsInstalled(integrations: ["SentryTestIntegration"])
-        let integration = SentrySDK.currentHub().installedIntegrations.firstObject
+        let integration = SentrySDK.currentHub().installedIntegrations.first
         XCTAssertTrue(integration is SentryTestIntegration)
         if let testIntegration = integration as? SentryTestIntegration {
             XCTAssertEqual(options.dsn, testIntegration.options.dsn)
@@ -482,6 +482,17 @@ class SentrySDKTests: XCTestCase {
         XCTAssertNotEqual(first, second)
     }
 
+    func testClose_ClearsIntegrations() {
+        SentrySDK.start { options in
+            options.dsn = SentrySDKTests.dsnAsString
+        }
+
+        let hub = SentrySDK.currentHub()
+        SentrySDK.close()
+        XCTAssertEqual(0, hub.installedIntegrations.count)
+        assertIntegrationsInstalled(integrations: [])
+    }
+
     func testFlush_CallsFlushCorrectlyOnTransport() {
         SentrySDK.start { options in
             options.dsn = SentrySDKTests.dsnAsString
@@ -564,6 +575,7 @@ class SentrySDKTests: XCTestCase {
     }
 
     private func assertIntegrationsInstalled(integrations: [String]) {
+        XCTAssertEqual(integrations.count, SentrySDK.currentHub().installedIntegrations.count)
         integrations.forEach { integration in
             if let integrationClass = NSClassFromString(integration) {
                 XCTAssertTrue(SentrySDK.currentHub().isIntegrationInstalled(integrationClass), "\(integration) not installed")

Tests/SentryTests/TestUtils/EmptyIntegration.swift

@@ -0,0 +1,7 @@
+import Foundation
+
+class EmptyIntegration: NSObject, SentryIntegrationProtocol {
+    func install(with options: Options) -> Bool {
+        return true
+    }
+}