diff --git a/CHANGELOG.md b/CHANGELOG.md index a67ea45..f4f5fdb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,7 @@ +# 8.1.1 +- [fixed] Prevent race condition crashes in GULNetworkURLSession by replacing + NSMapTable with thread-safe NSMutableDictionary and locking. (#228) + # 8.1.0 - Add a utility to detect App Clips and make a semantic check for background URL sessions. (#220) diff --git a/GoogleUtilities/AppDelegateSwizzler/GULAppDelegateSwizzler.m b/GoogleUtilities/AppDelegateSwizzler/GULAppDelegateSwizzler.m index 3b7280e..1857001 100644 --- a/GoogleUtilities/AppDelegateSwizzler/GULAppDelegateSwizzler.m +++ b/GoogleUtilities/AppDelegateSwizzler/GULAppDelegateSwizzler.m @@ -399,8 +399,8 @@ + (nullable Class)createSubclassWithObject:(id)appDelega } // For application:handleEventsForBackgroundURLSession:completionHandler: - SEL handleEventsForBackgroundURLSessionSEL = @selector(application: - handleEventsForBackgroundURLSession:completionHandler:); + SEL handleEventsForBackgroundURLSessionSEL = + @selector(application:handleEventsForBackgroundURLSession:completionHandler:); [self proxyDestinationSelector:handleEventsForBackgroundURLSessionSEL implementationsFromSourceSelector:handleEventsForBackgroundURLSessionSEL fromClass:[GULAppDelegateSwizzler class] @@ -411,8 +411,8 @@ + (nullable Class)createSubclassWithObject:(id)appDelega #if TARGET_OS_IOS // For application:openURL:sourceApplication:annotation: - SEL openURLSourceApplicationAnnotationSEL = @selector(application: - openURL:sourceApplication:annotation:); + SEL openURLSourceApplicationAnnotationSEL = + @selector(application:openURL:sourceApplication:annotation:); [self proxyDestinationSelector:openURLSourceApplicationAnnotationSEL implementationsFromSourceSelector:openURLSourceApplicationAnnotationSEL @@ -477,8 +477,8 @@ + (void)proxyRemoteNotificationsMethodsWithAppDelegateSubClass:(Class)appDelegat // For application:didRegisterForRemoteNotificationsWithDeviceToken: SEL didRegisterForRemoteNotificationsSEL = NSSelectorFromString(kGULDidRegisterForRemoteNotificationsSEL); - SEL didRegisterForRemoteNotificationsDonorSEL = @selector(application: - donor_didRegisterForRemoteNotificationsWithDeviceToken:); + SEL didRegisterForRemoteNotificationsDonorSEL = + @selector(application:donor_didRegisterForRemoteNotificationsWithDeviceToken:); [self proxyDestinationSelector:didRegisterForRemoteNotificationsSEL implementationsFromSourceSelector:didRegisterForRemoteNotificationsDonorSEL @@ -490,8 +490,8 @@ + (void)proxyRemoteNotificationsMethodsWithAppDelegateSubClass:(Class)appDelegat // For application:didFailToRegisterForRemoteNotificationsWithError: SEL didFailToRegisterForRemoteNotificationsSEL = NSSelectorFromString(kGULDidFailToRegisterForRemoteNotificationsSEL); - SEL didFailToRegisterForRemoteNotificationsDonorSEL = @selector(application: - donor_didFailToRegisterForRemoteNotificationsWithError:); + SEL didFailToRegisterForRemoteNotificationsDonorSEL = + @selector(application:donor_didFailToRegisterForRemoteNotificationsWithError:); [self proxyDestinationSelector:didFailToRegisterForRemoteNotificationsSEL implementationsFromSourceSelector:didFailToRegisterForRemoteNotificationsDonorSEL @@ -747,8 +747,8 @@ - (void)application:(GULApplication *)application handleEventsForBackgroundURLSession:(NSString *)identifier completionHandler:(void (^)())completionHandler { #pragma clang diagnostic pop - SEL methodSelector = @selector(application: - handleEventsForBackgroundURLSession:completionHandler:); + SEL methodSelector = + @selector(application:handleEventsForBackgroundURLSession:completionHandler:); NSValue *handleBackgroundSessionPointer = [GULAppDelegateSwizzler originalImplementationForSelector:methodSelector object:self]; GULRealHandleEventsForBackgroundURLSessionIMP handleBackgroundSessionIMP = diff --git a/GoogleUtilities/Network/GULNetwork.m b/GoogleUtilities/Network/GULNetwork.m index 23b3bb0..1a7e4a1 100644 --- a/GoogleUtilities/Network/GULNetwork.m +++ b/GoogleUtilities/Network/GULNetwork.m @@ -273,12 +273,12 @@ - (void)setLoggerDelegate:(id)loggerDelegate { // Explicitly check whether the delegate responds to the methods because conformsToProtocol does // not work correctly even though the delegate does respond to the methods. if (!loggerDelegate || - ![loggerDelegate respondsToSelector:@selector(GULNetwork_logWithLevel: - messageCode:message:contexts:)] || - ![loggerDelegate respondsToSelector:@selector(GULNetwork_logWithLevel: - messageCode:message:context:)] || - ![loggerDelegate respondsToSelector:@selector(GULNetwork_logWithLevel: - messageCode:message:)]) { + ![loggerDelegate + respondsToSelector:@selector(GULNetwork_logWithLevel:messageCode:message:contexts:)] || + ![loggerDelegate + respondsToSelector:@selector(GULNetwork_logWithLevel:messageCode:message:context:)] || + ![loggerDelegate + respondsToSelector:@selector(GULNetwork_logWithLevel:messageCode:message:)]) { GULOSLogError( kGULLogSubsystem, kGULLoggerNetwork, NO, [NSString stringWithFormat:@"I-NET%06ld", (long)kGULNetworkMessageCodeNetwork002], diff --git a/GoogleUtilities/Network/GULNetworkURLSession.m b/GoogleUtilities/Network/GULNetworkURLSession.m index c29de0c..762b0f1 100644 --- a/GoogleUtilities/Network/GULNetworkURLSession.m +++ b/GoogleUtilities/Network/GULNetworkURLSession.m @@ -28,6 +28,13 @@ @interface GULNetworkURLSession () @end +@interface GULNetworkURLSessionWeakHolder : NSObject +@property(nonatomic, weak) GULNetworkURLSession *session; +@end + +@implementation GULNetworkURLSessionWeakHolder +@end + @implementation GULNetworkURLSession { /// The handler to be called when the request completes or error has occurs. GULNetworkURLSessionCompletionHandler _completionHandler; @@ -552,12 +559,12 @@ + (instancetype)fetcherWithSessionIdentifier:(NSString *)sessionIdentifier { /// When reading and writing from/to the session map, don't use this method directly. /// To avoid thread safety issues, use one of the helper methods at the bottom of the /// file: setSessionInFetcherMap:forSessionID:, sessionFromFetcherMapForSessionID: -+ (NSMapTable *)sessionIDToFetcherMap { - static NSMapTable *sessionIDToFetcherMap; ++ (NSMutableDictionary *)sessionIDToFetcherMap { + static NSMutableDictionary *sessionIDToFetcherMap; static dispatch_once_t sessionMapOnceToken; dispatch_once(&sessionMapOnceToken, ^{ - sessionIDToFetcherMap = [NSMapTable strongToWeakObjectsMapTable]; + sessionIDToFetcherMap = [[NSMutableDictionary alloc] init]; }); return sessionIDToFetcherMap; } @@ -672,9 +679,13 @@ - (void)URLSession:(NSURLSession *)session #pragma mark - Helper Methods + (void)setSessionInFetcherMap:(GULNetworkURLSession *)session forSessionID:(NSString *)sessionID { + if (!sessionID) { + return; + } [[self sessionIDToFetcherMapReadWriteLock] lock]; - GULNetworkURLSession *existingSession = + GULNetworkURLSessionWeakHolder *holder = [[[self class] sessionIDToFetcherMap] objectForKey:sessionID]; + GULNetworkURLSession *existingSession = holder.session; if (existingSession) { if (session) { NSString *message = [NSString stringWithFormat:@"Discarding session: %@", existingSession]; @@ -685,7 +696,20 @@ + (void)setSessionInFetcherMap:(GULNetworkURLSession *)session forSessionID:(NSS [existingSession->_URLSession finishTasksAndInvalidate]; } if (session) { - [[[self class] sessionIDToFetcherMap] setObject:session forKey:sessionID]; + // Cleanup nil entries. + NSMutableArray *keysToRemove = [[NSMutableArray alloc] init]; + [[[self class] sessionIDToFetcherMap] + enumerateKeysAndObjectsUsingBlock:^(NSString *key, GULNetworkURLSessionWeakHolder *holder, + BOOL *stop) { + if (holder.session == nil) { + [keysToRemove addObject:key]; + } + }]; + [[[self class] sessionIDToFetcherMap] removeObjectsForKeys:keysToRemove]; + + GULNetworkURLSessionWeakHolder *newHolder = [[GULNetworkURLSessionWeakHolder alloc] init]; + newHolder.session = session; + [[[self class] sessionIDToFetcherMap] setObject:newHolder forKey:sessionID]; } else { [[[self class] sessionIDToFetcherMap] removeObjectForKey:sessionID]; } @@ -693,8 +717,13 @@ + (void)setSessionInFetcherMap:(GULNetworkURLSession *)session forSessionID:(NSS } + (nullable GULNetworkURLSession *)sessionFromFetcherMapForSessionID:(NSString *)sessionID { + if (!sessionID) { + return nil; + } [[self sessionIDToFetcherMapReadWriteLock] lock]; - GULNetworkURLSession *session = [[[self class] sessionIDToFetcherMap] objectForKey:sessionID]; + GULNetworkURLSessionWeakHolder *holder = + [[[self class] sessionIDToFetcherMap] objectForKey:sessionID]; + GULNetworkURLSession *session = holder.session; [[self sessionIDToFetcherMapReadWriteLock] unlock]; return session; } diff --git a/GoogleUtilities/Tests/Unit/Network/GULMutableDictionaryTest.m b/GoogleUtilities/Tests/Unit/Network/GULMutableDictionaryTest.m index 50cc812..8c0824f 100644 --- a/GoogleUtilities/Tests/Unit/Network/GULMutableDictionaryTest.m +++ b/GoogleUtilities/Tests/Unit/Network/GULMutableDictionaryTest.m @@ -15,6 +15,12 @@ #import #import "GoogleUtilities/Network/Public/GoogleUtilities/GULMutableDictionary.h" +#import "GoogleUtilities/Network/Public/GoogleUtilities/GULNetworkURLSession.h" + +@interface GULNetworkURLSession (Test) ++ (void)setSessionInFetcherMap:(GULNetworkURLSession *)session forSessionID:(NSString *)sessionID; ++ (nullable GULNetworkURLSession *)sessionFromFetcherMapForSessionID:(NSString *)sessionID; +@end const static NSString *const kKey = @"testKey1"; const static NSString *const kValue = @"testValue1"; @@ -84,4 +90,46 @@ - (void)testUnderlyingDictionary { XCTAssertEqual(dict[kKey2], kValue2); } +- (void)testSessionMapStress { + int iterations = 1000; + int numSessionIDs = 10; + + XCTestExpectation *expectation = [self expectationWithDescription:@"Stress test finished"]; + + dispatch_queue_t queue = + dispatch_queue_create("com.google.utilities.stress", DISPATCH_QUEUE_CONCURRENT); + + dispatch_group_t group = dispatch_group_create(); + + // Reader threads + for (int t = 0; t < 5; t++) { + dispatch_group_async(group, queue, ^{ + for (int i = 0; i < iterations; i++) { + int id_idx = i % numSessionIDs; + NSString *sessionID = [NSString stringWithFormat:@"session_%d", id_idx]; + [GULNetworkURLSession sessionFromFetcherMapForSessionID:sessionID]; + } + }); + } + + // Writer threads + for (int t = 0; t < 5; t++) { + dispatch_group_async(group, queue, ^{ + for (int i = 0; i < iterations; i++) { + int id_idx = i % numSessionIDs; + NSString *sessionID = [NSString stringWithFormat:@"session_%d", id_idx]; + GULNetworkURLSession *session = + [[GULNetworkURLSession alloc] initWithNetworkLoggerDelegate:nil]; + [GULNetworkURLSession setSessionInFetcherMap:session forSessionID:sessionID]; + } + }); + } + + dispatch_group_notify(group, dispatch_get_main_queue(), ^{ + [expectation fulfill]; + }); + + [self waitForExpectationsWithTimeout:30 handler:nil]; +} + @end diff --git a/GoogleUtilities/Tests/Unit/Network/GULNetworkTest.m b/GoogleUtilities/Tests/Unit/Network/GULNetworkTest.m index 57df4d9..c6bb89f 100644 --- a/GoogleUtilities/Tests/Unit/Network/GULNetworkTest.m +++ b/GoogleUtilities/Tests/Unit/Network/GULNetworkTest.m @@ -37,6 +37,11 @@ - (void)maybeRemoveTempFilesAtURL:(NSURL *)tempFile expiringTime:(NSTimeInterval @end +@interface GULNetworkURLSession (Test) ++ (void)setSessionInFetcherMap:(GULNetworkURLSession *)session forSessionID:(NSString *)sessionID; ++ (nullable GULNetworkURLSession *)sessionFromFetcherMapForSessionID:(NSString *)sessionID; +@end + @interface GULNetworkTest : XCTestCase @end @@ -1091,6 +1096,48 @@ - (void)reachabilityDidChange { _currentNetworkStatus = _fakeNetworkIsReachable; } +- (void)testSessionMapStress { + int iterations = 1000; + int numSessionIDs = 10; + + XCTestExpectation *expectation = [self expectationWithDescription:@"Stress test finished"]; + + dispatch_queue_t queue = + dispatch_queue_create("com.google.utilities.stress", DISPATCH_QUEUE_CONCURRENT); + + dispatch_group_t group = dispatch_group_create(); + + // Reader threads + for (int t = 0; t < 5; t++) { + dispatch_group_async(group, queue, ^{ + for (int i = 0; i < iterations; i++) { + int id_idx = i % numSessionIDs; + NSString *sessionID = [NSString stringWithFormat:@"session_%d", id_idx]; + [GULNetworkURLSession sessionFromFetcherMapForSessionID:sessionID]; + } + }); + } + + // Writer threads + for (int t = 0; t < 5; t++) { + dispatch_group_async(group, queue, ^{ + for (int i = 0; i < iterations; i++) { + int id_idx = i % numSessionIDs; + NSString *sessionID = [NSString stringWithFormat:@"session_%d", id_idx]; + GULNetworkURLSession *session = + [[GULNetworkURLSession alloc] initWithNetworkLoggerDelegate:nil]; + [GULNetworkURLSession setSessionInFetcherMap:session forSessionID:sessionID]; + } + }); + } + + dispatch_group_notify(group, dispatch_get_main_queue(), ^{ + [expectation fulfill]; + }); + + [self waitForExpectationsWithTimeout:30 handler:nil]; +} + @end #endif // TARGET_OS_MACCATALYST diff --git a/GoogleUtilities/Tests/Unit/Swizzler/GULAppDelegateSwizzlerTest.m b/GoogleUtilities/Tests/Unit/Swizzler/GULAppDelegateSwizzlerTest.m index 7ed61b7..1933987 100644 --- a/GoogleUtilities/Tests/Unit/Swizzler/GULAppDelegateSwizzlerTest.m +++ b/GoogleUtilities/Tests/Unit/Swizzler/GULAppDelegateSwizzlerTest.m @@ -107,8 +107,8 @@ + (void)load { gRespondsToOpenURLHandler_iOS9 = [self instancesRespondToSelector:@selector(application:openURL:options:)]; gRespondsToHandleBackgroundSession = - [self instancesRespondToSelector:@selector(application: - handleEventsForBackgroundURLSession:completionHandler:)]; + [self instancesRespondToSelector: + @selector(application:handleEventsForBackgroundURLSession:completionHandler:)]; gRespondsToContinueUserActivity = [self instancesRespondToSelector:@selector(application:continueUserActivity:restorationHandler:)]; #pragma clang diagnostic pop @@ -292,22 +292,22 @@ - (void)testProxyAppDelegate { // Class size must stay the same. XCTAssertEqual(sizeBefore, sizeAfter); - XCTAssertTrue([realAppDelegate respondsToSelector:@selector(application: - continueUserActivity:restorationHandler:)]); + XCTAssertTrue([realAppDelegate + respondsToSelector:@selector(application:continueUserActivity:restorationHandler:)]); XCTAssertTrue([realAppDelegate respondsToSelector:@selector(application:didRegisterForRemoteNotificationsWithDeviceToken:)]); XCTAssertTrue([realAppDelegate respondsToSelector:@selector(application:didFailToRegisterForRemoteNotificationsWithError:)]); - XCTAssertTrue([realAppDelegate respondsToSelector:@selector(application: - didReceiveRemoteNotification:)]); + XCTAssertTrue( + [realAppDelegate respondsToSelector:@selector(application:didReceiveRemoteNotification:)]); #if TARGET_OS_IOS || TARGET_OS_TV XCTAssertTrue([realAppDelegate respondsToSelector:@selector(application:openURL:options:)]); XCTAssertTrue([realAppDelegate - respondsToSelector:@selector(application: - handleEventsForBackgroundURLSession:completionHandler:)]); + respondsToSelector:@selector( + application:handleEventsForBackgroundURLSession:completionHandler:)]); XCTAssertTrue([realAppDelegate - respondsToSelector:@selector(application: - didReceiveRemoteNotification:fetchCompletionHandler:)]); + respondsToSelector:@selector( + application:didReceiveRemoteNotification:fetchCompletionHandler:)]); #endif // TARGET_OS_IOS || TARGET_OS_TV // Make sure that the class has changed. @@ -343,25 +343,25 @@ - (void)testProxyEmptyAppDelegate { // Class size must stay the same. XCTAssertEqual(sizeBefore, sizeAfter); - XCTAssertTrue([realAppDelegate respondsToSelector:@selector(application: - continueUserActivity:restorationHandler:)]); + XCTAssertTrue([realAppDelegate + respondsToSelector:@selector(application:continueUserActivity:restorationHandler:)]); // Remote notifications methods should be added only by // -proxyOriginalDelegateIncludingAPNSMethods XCTAssertFalse([realAppDelegate respondsToSelector:@selector(application:didRegisterForRemoteNotificationsWithDeviceToken:)]); XCTAssertFalse([realAppDelegate respondsToSelector:@selector(application:didFailToRegisterForRemoteNotificationsWithError:)]); - XCTAssertFalse([realAppDelegate respondsToSelector:@selector(application: - didReceiveRemoteNotification:)]); + XCTAssertFalse( + [realAppDelegate respondsToSelector:@selector(application:didReceiveRemoteNotification:)]); #if TARGET_OS_IOS || TARGET_OS_TV // The implementation should not be added if there is no original implementation XCTAssertFalse([realAppDelegate respondsToSelector:@selector(application:openURL:options:)]); XCTAssertTrue([realAppDelegate - respondsToSelector:@selector(application: - handleEventsForBackgroundURLSession:completionHandler:)]); + respondsToSelector:@selector( + application:handleEventsForBackgroundURLSession:completionHandler:)]); XCTAssertFalse([realAppDelegate - respondsToSelector:@selector(application: - didReceiveRemoteNotification:fetchCompletionHandler:)]); + respondsToSelector:@selector( + application:didReceiveRemoteNotification:fetchCompletionHandler:)]); #endif // TARGET_OS_IOS || TARGET_OS_TV // Make sure that the class has changed. @@ -390,8 +390,8 @@ - (void)testProxyRemoteNotificationsMethodsEmptyAppDelegate { // Class size must stay the same. XCTAssertEqual(sizeBefore, sizeAfter); - XCTAssertTrue([realAppDelegate respondsToSelector:@selector(application: - continueUserActivity:restorationHandler:)]); + XCTAssertTrue([realAppDelegate + respondsToSelector:@selector(application:continueUserActivity:restorationHandler:)]); // Remote notifications methods should be added only by // -proxyOriginalDelegateIncludingAPNSMethods @@ -405,12 +405,12 @@ - (void)testProxyRemoteNotificationsMethodsEmptyAppDelegate { XCTAssertFalse([realAppDelegate respondsToSelector:@selector(application:openURL:options:)]); XCTAssertTrue([realAppDelegate - respondsToSelector:@selector(application: - handleEventsForBackgroundURLSession:completionHandler:)]); + respondsToSelector:@selector( + application:handleEventsForBackgroundURLSession:completionHandler:)]); XCTAssertTrue([realAppDelegate - respondsToSelector:@selector(application: - didReceiveRemoteNotification:fetchCompletionHandler:)]); + respondsToSelector:@selector( + application:didReceiveRemoteNotification:fetchCompletionHandler:)]); #endif // TARGET_OS_IOS || TARGET_OS_TV || TARGET_OS_VISION @@ -440,8 +440,8 @@ - (void)testProxyRemoteNotificationsMethodsEmptyAppDelegateAfterInitialProxy { // Class size must stay the same. XCTAssertEqual(sizeBefore, sizeAfter); - XCTAssertTrue([realAppDelegate respondsToSelector:@selector(application: - continueUserActivity:restorationHandler:)]); + XCTAssertTrue([realAppDelegate + respondsToSelector:@selector(application:continueUserActivity:restorationHandler:)]); // Proxy remote notifications methods [GULAppDelegateSwizzler proxyOriginalDelegateIncludingAPNSMethods]; @@ -454,12 +454,12 @@ - (void)testProxyRemoteNotificationsMethodsEmptyAppDelegateAfterInitialProxy { // The implementation should not be added if there is no original implementation XCTAssertFalse([realAppDelegate respondsToSelector:@selector(application:openURL:options:)]); XCTAssertTrue([realAppDelegate - respondsToSelector:@selector(application: - handleEventsForBackgroundURLSession:completionHandler:)]); + respondsToSelector:@selector( + application:handleEventsForBackgroundURLSession:completionHandler:)]); XCTAssertTrue([realAppDelegate - respondsToSelector:@selector(application: - didReceiveRemoteNotification:fetchCompletionHandler:)]); + respondsToSelector:@selector( + application:didReceiveRemoteNotification:fetchCompletionHandler:)]); #endif // TARGET_OS_IOS || TARGET_OS_TV || TARGET_OS_VISION // Make sure that the class has changed. @@ -1004,13 +1004,15 @@ - (void)testApplicationDidReceiveRemoteNotificationWithCompletionImplementationI GULTestInterceptorAppDelegate *delegate = [[GULTestInterceptorAppDelegate alloc] init]; OCMStub([self.mockSharedApplication delegate]).andReturn(delegate); - XCTAssertFalse([delegate respondsToSelector:@selector - (application:didReceiveRemoteNotification:fetchCompletionHandler:)]); + XCTAssertFalse([delegate + respondsToSelector:@selector( + application:didReceiveRemoteNotification:fetchCompletionHandler:)]); [GULAppDelegateSwizzler proxyOriginalDelegateIncludingAPNSMethods]; - XCTAssertTrue([delegate respondsToSelector:@selector - (application:didReceiveRemoteNotification:fetchCompletionHandler:)]); + XCTAssertTrue([delegate + respondsToSelector:@selector( + application:didReceiveRemoteNotification:fetchCompletionHandler:)]); } #endif // TARGET_OS_IOS || TARGET_OS_TV diff --git a/Mintfile b/Mintfile index 4e60f1d..3eb1574 100644 --- a/Mintfile +++ b/Mintfile @@ -1 +1 @@ -nicklockwood/SwiftFormat@0.49.2 +nicklockwood/SwiftFormat@0.55.5