From 1a441cc40cc78ace8f0bb0a2f5d83fe8665573eb Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Mon, 21 May 2018 17:02:59 -0400 Subject: [PATCH] Respond to CR. --- .../src/Network/WebSockets/TSSocketManager.h | 2 +- .../src/Network/WebSockets/TSSocketManager.m | 82 +++++++++++++++---- SignalServiceKit/src/Util/Cryptography.h | 5 +- SignalServiceKit/src/Util/Cryptography.m | 20 ++++- 4 files changed, 88 insertions(+), 21 deletions(-) diff --git a/SignalServiceKit/src/Network/WebSockets/TSSocketManager.h b/SignalServiceKit/src/Network/WebSockets/TSSocketManager.h index 00d26231d..8502b5a13 100644 --- a/SignalServiceKit/src/Network/WebSockets/TSSocketManager.h +++ b/SignalServiceKit/src/Network/WebSockets/TSSocketManager.h @@ -24,7 +24,7 @@ typedef void (^TSSocketMessageFailure)(NSInteger statusCode, NSError *error); @interface TSSocketManager : NSObject -@property (nonatomic, readonly) SocketManagerState state; +@property (atomic, readonly) SocketManagerState state; @property (atomic, readonly) BOOL canMakeRequests; + (instancetype)sharedManager; diff --git a/SignalServiceKit/src/Network/WebSockets/TSSocketManager.m b/SignalServiceKit/src/Network/WebSockets/TSSocketManager.m index 2c49e4f09..f86a41557 100644 --- a/SignalServiceKit/src/Network/WebSockets/TSSocketManager.m +++ b/SignalServiceKit/src/Network/WebSockets/TSSocketManager.m @@ -69,7 +69,10 @@ NSString *const kNSNotification_SocketManagerStateDidChange = @"kNSNotification_ OWSAssert(self.success); OWSAssert(self.failure); - self.success(responseObject); + TSSocketMessageSuccess success = self.success; + dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ + success(responseObject); + }); self.success = nil; self.failure = nil; @@ -98,7 +101,10 @@ NSString *const kNSNotification_SocketManagerStateDidChange = @"kNSNotification_ OWSAssert(self.success); OWSAssert(self.failure); - self.failure(statusCode, error); + TSSocketMessageFailure failure = self.failure; + dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ + failure(statusCode, error); + }); self.success = nil; self.failure = nil; @@ -133,8 +139,8 @@ NSString *const kNSNotification_SocketManagerStateDidChange = @"kNSNotification_ // websocket's actual state, so we're defensive and distrustful of // this property. // -// We only ever access this state on the main thread. -@property (nonatomic) SocketManagerState state; +// We only ever mutate this state on the main thread. +@property (atomic) SocketManagerState state; #pragma mark - @@ -164,14 +170,14 @@ NSString *const kNSNotification_SocketManagerStateDidChange = @"kNSNotification_ // This property should only be accessed while synchronized on the socket manager. @property (nonatomic, readonly) NSMutableDictionary *socketMessageMap; -@property (atomic) BOOL canMakeRequests; - @end #pragma mark - @implementation TSSocketManager +@synthesize state = _state; + - (instancetype)init { self = [super init]; @@ -369,19 +375,37 @@ NSString *const kNSNotification_SocketManagerStateDidChange = @"kNSNotification_ // [SRWebSocket open] could hypothetically call a delegate method (e.g. if // the socket failed immediately for some reason), so we update the state // _before_ calling it, not after. - _state = state; - _canMakeRequests = state == SocketManagerStateOpen; + @synchronized(self) + { + _state = state; + } [socket open]; + [self failAllPendingSocketMessagesIfNecessary]; return; } } - _state = state; - _canMakeRequests = state == SocketManagerStateOpen; - + @synchronized(self) + { + _state = state; + } + [self failAllPendingSocketMessagesIfNecessary]; [self notifyStatusChange]; } +- (SocketManagerState)state +{ + @synchronized(self) + { + return _state; + } +} + +- (BOOL)canMakeRequests +{ + return self.state == SocketManagerStateOpen; +} + - (void)notifyStatusChange { [[NSNotificationCenter defaultCenter] postNotificationNameAsync:kNSNotification_SocketManagerStateDidChange @@ -428,8 +452,8 @@ NSString *const kNSNotification_SocketManagerStateDidChange = @"kNSNotification_ @synchronized(self) { - // TODO: Should we use another random number generator? - socketMessage.requestId = arc4random(); + socketMessage.requestId = [Cryptography randomUInt64]; + self.socketMessageMap[@(socketMessage.requestId)] = socketMessage; } @@ -445,7 +469,6 @@ NSString *const kNSNotification_SocketManagerStateDidChange = @"kNSNotification_ error:&error]; if (!jsonData || error) { OWSProdLogAndFail(@"%@ could not serialize request JSON: %@", self.logTag, error); - // TODO: [socketMessage didFailBeforeSending]; return; } @@ -495,7 +518,7 @@ NSString *const kNSNotification_SocketManagerStateDidChange = @"kNSNotification_ requestPath, jsonData.length); - // Ensure requests "timeout" after 30 seconds. + // Ensure requests "timeout" after 10 seconds. __weak TSSocketMessage *weakSocketMessage = socketMessage; dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(30 * NSEC_PER_SEC)), dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), @@ -544,14 +567,15 @@ NSString *const kNSNotification_SocketManagerStateDidChange = @"kNSNotification_ } NSArray *_Nullable responseHeaders = message.headers; + BOOL hasValidResponse = YES; id responseObject = responseBody; if (responseBody) { NSError *error; id _Nullable responseJson = [NSJSONSerialization JSONObjectWithData:responseBody options:(NSJSONReadingOptions)0 error:&error]; if (!responseJson || error) { - DDLogError(@"%@ could not parse WebSocket response JSON: %@.", self.logTag, error); - // TODO: Should we require JSON parsing to succeed? + OWSProdLogAndFail(@"%@ could not parse WebSocket response JSON: %@.", self.logTag, error); + hasValidResponse = NO; } else { responseObject = responseJson; } @@ -567,7 +591,8 @@ NSString *const kNSNotification_SocketManagerStateDidChange = @"kNSNotification_ if (!socketMessage) { DDLogError(@"%@ received response to unknown request.", self.logTag); } else { - BOOL didSucceed = 200 <= responseStatus && responseStatus <= 299; + BOOL hasSuccessStatus = 200 <= responseStatus && responseStatus <= 299; + BOOL didSucceed = hasSuccessStatus && hasValidResponse; if (didSucceed) { [socketMessage didSucceedWithResponseObject:responseObject]; } else { @@ -589,6 +614,27 @@ NSString *const kNSNotification_SocketManagerStateDidChange = @"kNSNotification_ responseObject); } +- (void)failAllPendingSocketMessagesIfNecessary +{ + if (!self.canMakeRequests) { + [self failAllPendingSocketMessages]; + } +} + +- (void)failAllPendingSocketMessages +{ + NSArray *socketMessages; + @synchronized(self) + { + socketMessages = self.socketMessageMap.allValues; + [self.socketMessageMap removeAllObjects]; + } + + for (TSSocketMessage *socketMessage in socketMessages) { + [socketMessage didFailBeforeSending]; + } +} + #pragma mark - Delegate methods - (void)webSocketDidOpen:(SRWebSocket *)webSocket { diff --git a/SignalServiceKit/src/Util/Cryptography.h b/SignalServiceKit/src/Util/Cryptography.h index e6d00487d..b4a5deced 100755 --- a/SignalServiceKit/src/Util/Cryptography.h +++ b/SignalServiceKit/src/Util/Cryptography.h @@ -1,5 +1,5 @@ // -// Copyright (c) 2017 Open Whisper Systems. All rights reserved. +// Copyright (c) 2018 Open Whisper Systems. All rights reserved. // NS_ASSUME_NONNULL_BEGIN @@ -36,6 +36,9 @@ typedef NS_ENUM(NSInteger, TSMACType) { + (NSData *)generateRandomBytes:(NSUInteger)numberBytes; ++ (uint32_t)randomUInt32; ++ (uint64_t)randomUInt64; + #pragma mark SHA and HMAC methods // Full length SHA256 digest for `data` diff --git a/SignalServiceKit/src/Util/Cryptography.m b/SignalServiceKit/src/Util/Cryptography.m index 9dd8909c0..272536cd4 100755 --- a/SignalServiceKit/src/Util/Cryptography.m +++ b/SignalServiceKit/src/Util/Cryptography.m @@ -102,7 +102,25 @@ const NSUInteger kAES256_KeyByteLength = 32; + (NSData *)generateRandomBytes:(NSUInteger)numberBytes { - return [Randomness generateRandomBytes:numberBytes]; + return [Randomness generateRandomBytes:(int)numberBytes]; +} + ++ (uint32_t)randomUInt32 +{ + size_t size = sizeof(uint32_t); + NSData *data = [self generateRandomBytes:size]; + uint32_t result = 0; + [data getBytes:&result range:NSMakeRange(0, size)]; + return result; +} + ++ (uint64_t)randomUInt64 +{ + size_t size = sizeof(uint64_t); + NSData *data = [self generateRandomBytes:size]; + uint64_t result = 0; + [data getBytes:&result range:NSMakeRange(0, size)]; + return result; } #pragma mark SHA1