Skip to content

Add fix for Accessing connections dictioray serially to avoid crash - #85

Merged
rishabhabbu-ctrl merged 5 commits into
mainfrom
fix/accessConnectionSrially
Aug 24, 2026
Merged

rishabhabbu-ctrl merged 5 commits into
mainfrom
fix/accessConnectionSrially

Conversation

@rishabhabbu-ctrl

Copy link
Copy Markdown
Collaborator

Summary

Fixes an EXC_BAD_ACCESS (KERN_INVALID_ADDRESS) crash in
-[MQTTSession subscribeToTopics:subscribeHandler:], caused by concurrent
mutation of MQTTSession's handler dictionaries from two unsynchronised threads.

Gated behind a new opt-in serializeSessionAccess config flag, defaulting to
false, so behaviour is unchanged unless a client explicitly enables it.

Root cause

MQTTSession is confined to the queue its CFStreams are scheduled on
(MQTTSession.hqueue, applied via CFReadStreamSetDispatchQueue in
MQTTCFSocketTransport). Every callback it raises and every mutation of its
internal state happens there. MQTTClientFrameworkSessionManager.subscribe(_:)
and unsubscribe(_:) called into the session on the caller's thread, with no
hop onto that queue, so two paths raced:

  1. On the session queue — a socket read delivers CONNACK, which flows
    synchronously through handleEventupdateState → delegate → the client's
    connect-success handling → re-subscribe of all stored topics.
  2. On an arbitrary thread — an app-level subscribe/unsubscribe, e.g. from
    the main thread or a network completion handler.

Both mutate subscribeHandlers / unsubscribeHandlers, which are plain
NSMutableDictionary with no synchronisation. When an insert triggers a bucket
reallocation (mdict_rehashd) while the other thread is reading or writing the
same table, the second thread dereferences freed memory and the process dies in
CFStringHashCString / objc_msgSend.

Note that nextMsgId is already @synchronized(self) — message-ID generation was
protected, the handler-map write two lines later was not.

The isConnected guard in MQTTClientFrameworkConnection.subscribe(_:) makes this
more 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 withSession helper in
MQTTClientFrameworkSessionManager:

  • Flag enabled — work is dispatched onto the session's own serial queue, giving
    mutual exclusion with the session's event handling. It also re-validates
    state == .connected on the queue, since execution is deferred and the connection
    may have dropped since the caller checked; and it reads session on the queue,
    which additionally removes a race on that property (it is private(set) var, not
    @Atomic, and is reassigned on reconnect).
  • Flag disabledwork is invoked inline on the caller's thread, identical to
    the previous behaviour.

queue.async is used deliberately, never queue.sync: the CONNACK path is already
executing on that serial queue, so sync would deadlock immediately. This is called
out in a code comment to stop it being "optimised" later.

subscribe and unsubscribe are 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: Bool on MQTTClientConfig, defaulting to false,
plumbed through MQTTConfigurationConnectionConfig
MQTTClientFrameworkConnectionMQTTClientFrameworkSessionManager, following the
existing fixCxxDestructCrash pattern.

MQTTClientConfig(
    authService: authService,
    serializeSessionAccess: true
)

The value is captured for the lifetime of the client and is intentionally not
re-read per call. A per-call read would be actively harmful here: if one thread saw
true and hopped onto the queue while another saw false and called through
directly, the two would still be unserialised and the race would remain — a
half-enabled flag gives no protection. Consumers should read their remote value once
at construction time; changing it takes effect on the next client creation.

The default is false only to keep this release behaviour-compatible; enabling it is
recommended, and the legacy path is intended for removal once rollout completes.

Testing

- Full suite green: 180 tests, 0 failures (176 existing + 4 new).
- New tests in MQTTClientFrameworkSessionManagerTests:
  - testSubscribeRunsInlineWhenSerializeSessionAccessDisabled — legacy path stays
    synchronous.
  - testSubscribeIsSerialisedOntoSessionQueueWhenEnabled — proves the hop: the
    queue is suspended, subscribe is issued, the session is asserted not to have
    been touched inline, then the queue is drained and the call is asserted to have
    landed. Suspending makes this deterministic rather than racy.
  - testUnsubscribeIsSerialisedOntoSessionQueueWhenEnabled — same for unsubscribe.
  - testSubscribeIsDroppedWhenNotConnectedAndSerializeSessionAccessEnabled — covers
    the deferred-path re-validation.
- Existing tests were unchanged apart from passing the new parameter. Worth noting:
  they assert synchronously after subscribe, so an unconditional queue hop would
  have broken them — another reason the legacy path is preserved as the default.

Risk

- Disabled (default): no behavioural change; withSession calls straight through.
- Enabled: subscribe/unsubscribe become asynchronous. Both return Void at
  every layer, so no caller awaits a result. Subscription bookkeeping in the client
  remains synchronous and is the replay source on reconnect, so a deferred call that
  is dropped while disconnected is restored on the next successful connect.
- Expect a small p99 increase on subscribe latency from the queue hop.

Not included

Hardening MQTTSession's handler dictionaries with a lock (defense-in-depth for
callers outside this SDK). That has a different failure mode — a re-entrant handler
taking the lock would deadlock rather than crash — so it belongs behind its own flag
in a separate change.

Checklist

- [x] Builds for iOS Simulator
- [x] Unit tests added and passing
- [x] Default behaviour unchanged
- [ ] CHANGELOG / version bump (handled by CI)

@rishabhabbu-ctrl
rishabhabbu-ctrl merged commit dec3a02 into main Aug 24, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants