Skip to content

Commit

Permalink
CBL-5418 : Ensure to disconnect from the remote before CBLWebSocket b…
Browse files Browse the repository at this point in the history
…eing dealloc (#3240)

* Ported the fix (159c4ee) from release/3.1 branch.

* It is possible that the dispose() function is called by the c4socket before the disconnect() function is called. One of the cases is when c4socket calls the dispose() as the close timeout is reached.

* The fix ensures that when the dispose() is called, the disconnect() function will be called if the connection hasn’t been not closed yet.

* The fix also make sure that the crash doesn’t happen after the c4socket is freed while there are pending tasks in the queue being run at the same time and might access it after it’s freed. This requires to use a lock to prevent this.

* Added message to the assertions.
  • Loading branch information
pasin authored Feb 28, 2024
1 parent 24f1562 commit 2285757
Showing 1 changed file with 48 additions and 14 deletions.
62 changes: 48 additions & 14 deletions Objective-C/Internal/Replicator/CBLWebSocket.mm
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ @implementation CBLWebSocket
dispatch_queue_t _queue;
NSString* _expectedAcceptHeader;
CBLHTTPLogic* _logic;
std::atomic<C4Socket*> _c4socket;
C4Socket* _c4socket;
CFHTTPMessageRef _httpResponse;
id _keepMeAlive;

Expand Down Expand Up @@ -223,21 +223,49 @@ - (instancetype) initWithURL: (NSURL*)url

- (void) dealloc {
CBLLogVerbose(WebSocket, @"%@: DEALLOC...", self);
Assert(!_in);
Assert(_sockfd < 0);
Assert(!_in, @"Network stream was not closed");
Assert(_sockfd < 0, @"Socket was not closed");
free(_readBuffer);
if (_httpResponse)
if (_httpResponse) {
CFRelease(_httpResponse);
}
}

- (void) dispose {
CBLLogVerbose(WebSocket, @"%@: C4Socket of is being disposed", self);
CBLLogVerbose(WebSocket, @"%@: CBLWebSocket is being disposed", self);

// This has to be done synchronously, because _c4socket will be freed when this method returns
auto socket = _c4socket.exchange(nullptr);
if (socket)
[self callC4Socket: ^(C4Socket *socket) {
// A lock is necessary as the socket could be accessed from another thread under the dispatch
// queue, otherwise crash will happen as the c4socket will be freed after this.
// The c4socket doesn't call dispose under a mutex so this is safe from being deadlock.
c4Socket_setNativeHandle(socket, nullptr);
// Remove the self-reference, so this object will be dealloced:
_keepMeAlive = nil;
self->_c4socket = nullptr;
}];

dispatch_async(_queue, ^{
// CBSE-16151:
//
// The CBLWebSocket may be called to dispose() by the c4socket before the
// disconnect() can happen. For example, if the CBLWebSocket cannot
// call c4socket_closed() callback before the timeout (5 seconds),
// the c4socket will call to dispose() the CBLWebSocket right away.
//
// Therefore, before CBLWebSocket is dealloc, we need to ensure that the
// disconnect() is called to close the network steams and sockets. This
// needs to be done under the same queue that the network streams and
// c4socket's handlers/callbacks are using to avoid threading issues.
//
// Note: the CBLWebSocket will be retained until this block is called
// even though the _keepMeAlive is set to nil at the end of this
// dispose method.
if ([self isConnected]) {
[self disconnect];
}
});

// Remove the self-reference, so this object will be dealloced.
self->_keepMeAlive = nil;
}

- (void) clearHTTPState {
Expand Down Expand Up @@ -291,9 +319,11 @@ - (void) setupAuth {
}

- (void) callC4Socket: (void (^)(C4Socket*))callback {
auto socket = _c4socket.load();
if (socket)
callback(socket);
@synchronized (self) {
if (_c4socket) {
callback(_c4socket);
}
}
}

#pragma mark - HANDSHAKE:
Expand Down Expand Up @@ -467,7 +497,7 @@ - (void) _socketConnect: (AddressInfo*)info {
return;
}

// Create a pair steam with the socket:
// Create a pair stream with the socket:
CFReadStreamRef readStream;
CFWriteStreamRef writeStream;
CFStreamCreatePairWithSocket(kCFAllocatorDefault, self->_sockfd, &readStream, &writeStream);
Expand Down Expand Up @@ -854,7 +884,7 @@ - (void) completedReceive: (size_t)byteCount {
- (void) closeSocket {
CBLLogInfo(WebSocket, @"%@: CBLWebSocket closeSocket requested", self);
dispatch_async(_queue, ^{
if (self->_in || self->_out || self->_sockfd >= 0) {
if ([self isConnected]) {
[self closeWithError: nil];
}
});
Expand Down Expand Up @@ -1093,6 +1123,10 @@ - (void) disconnect {
}
}

- (BOOL) isConnected {
return (_in || _out || _sockfd >= 0 || _dnsService);
}

#pragma mark - Helper

+ (NSArray*) parseCookies: (NSString*) cookieStr {
Expand Down

0 comments on commit 2285757

Please sign in to comment.