Add fix for Accessing connections dictioray serially to avoid crash - #85
Merged
Merged
Conversation
…Lite to Courier and Clickstream SDKs
AbhijeetMallick
approved these changes
Aug 21, 2026
…Lite to Courier and Clickstream SDKs
AbhijeetMallick
approved these changes
Aug 24, 2026
deepanshu42
approved these changes
Aug 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes an
EXC_BAD_ACCESS (KERN_INVALID_ADDRESS)crash in-[MQTTSession subscribeToTopics:subscribeHandler:], caused by concurrentmutation of
MQTTSession's handler dictionaries from two unsynchronised threads.Gated behind a new opt-in
serializeSessionAccessconfig flag, defaulting tofalse, so behaviour is unchanged unless a client explicitly enables it.Root cause
MQTTSessionis confined to the queue its CFStreams are scheduled on(
MQTTSession.h—queue, applied viaCFReadStreamSetDispatchQueueinMQTTCFSocketTransport). Every callback it raises and every mutation of itsinternal state happens there.
MQTTClientFrameworkSessionManager.subscribe(_:)and
unsubscribe(_:)called into the session on the caller's thread, with nohop onto that queue, so two paths raced:
synchronously through
handleEvent→updateState→ delegate → the client'sconnect-success handling → re-subscribe of all stored topics.
subscribe/unsubscribe, e.g. fromthe main thread or a network completion handler.
Both mutate
subscribeHandlers/unsubscribeHandlers, which are plainNSMutableDictionarywith no synchronisation. When an insert triggers a bucketreallocation (
mdict_rehashd) while the other thread is reading or writing thesame table, the second thread dereferences freed memory and the process dies in
CFStringHashCString/objc_msgSend.Note that
nextMsgIdis already@synchronized(self)— message-ID generation wasprotected, the handler-map write two lines later was not.
The
isConnectedguard inMQTTClientFrameworkConnection.subscribe(_:)makes thismore likely, not less: it opens the app-level path exactly when the connection has
just come up, which is when the internal re-subscribe burst is firing. Reconnects
are the peak-risk window.
Fix
All session access now goes through a single
withSessionhelper inMQTTClientFrameworkSessionManager:mutual exclusion with the session's event handling. It also re-validates
state == .connectedon the queue, since execution is deferred and the connectionmay have dropped since the caller checked; and it reads
sessionon the queue,which additionally removes a race on that property (it is
private(set) var, not@Atomic, and is reassigned on reconnect).workis invoked inline on the caller's thread, identical tothe previous behaviour.
queue.asyncis used deliberately, neverqueue.sync: the CONNACK path is alreadyexecuting on that serial queue, so
syncwould deadlock immediately. This is calledout in a code comment to stop it being "optimised" later.
subscribeandunsubscribeare both routed through the helper under the same flag.They must move together — the reconnect path issues them as an ordered pair, and
deferring only one would invert their relative order.
Configuration
New
serializeSessionAccess: BoolonMQTTClientConfig, defaulting tofalse,plumbed through
MQTTConfiguration→ConnectionConfig→MQTTClientFrameworkConnection→MQTTClientFrameworkSessionManager, following theexisting
fixCxxDestructCrashpattern.