From 8da6dba905e4f702742bc2f7d0dea077ec6ed8c6 Mon Sep 17 00:00:00 2001 From: jack Date: Fri, 10 Jul 2026 20:20:45 +0200 Subject: [PATCH] Fix BLE identity state races --- .../Services/BLE/BLEAnnounceThrottle.swift | 27 ++++++--- .../BLE/BLELocalIdentityStateStore.swift | 55 ++++++++++++++++++ bitchat/Services/BLE/BLEService.swift | 57 ++++++++++--------- .../Services/BLEAnnounceThrottleTests.swift | 38 +++++++++++-- .../BLELocalIdentityStateStoreTests.swift | 57 +++++++++++++++++++ 5 files changed, 193 insertions(+), 41 deletions(-) create mode 100644 bitchat/Services/BLE/BLELocalIdentityStateStore.swift create mode 100644 bitchatTests/Services/BLELocalIdentityStateStoreTests.swift diff --git a/bitchat/Services/BLE/BLEAnnounceThrottle.swift b/bitchat/Services/BLE/BLEAnnounceThrottle.swift index 6bd214b3..d6bf5490 100644 --- a/bitchat/Services/BLE/BLEAnnounceThrottle.swift +++ b/bitchat/Services/BLE/BLEAnnounceThrottle.swift @@ -1,6 +1,13 @@ import Foundation -struct BLEAnnounceThrottle { +/// Thread-safe announce admission state. +/// +/// Announce requests originate from the Bluetooth delegate queue, the +/// concurrent message queue, and the maintenance timer. Keeping the timestamp +/// behind a lock makes admission and maintenance snapshots atomic when those +/// request sources race. +final class BLEAnnounceThrottle: @unchecked Sendable { + private let lock = NSLock() private var lastSent: Date private let normalMinimumInterval: TimeInterval private let forcedMinimumInterval: TimeInterval @@ -16,16 +23,18 @@ struct BLEAnnounceThrottle { } func elapsed(since now: Date) -> TimeInterval { - now.timeIntervalSince(lastSent) + lock.withLock { now.timeIntervalSince(lastSent) } } - mutating func shouldSend(force: Bool, now: Date) -> Bool { - let minimumInterval = force ? forcedMinimumInterval : normalMinimumInterval - guard elapsed(since: now) >= minimumInterval else { - return false - } + func shouldSend(force: Bool, now: Date) -> Bool { + lock.withLock { + let minimumInterval = force ? forcedMinimumInterval : normalMinimumInterval + guard now.timeIntervalSince(lastSent) >= minimumInterval else { + return false + } - lastSent = now - return true + lastSent = now + return true + } } } diff --git a/bitchat/Services/BLE/BLELocalIdentityStateStore.swift b/bitchat/Services/BLE/BLELocalIdentityStateStore.swift new file mode 100644 index 00000000..b379b0cd --- /dev/null +++ b/bitchat/Services/BLE/BLELocalIdentityStateStore.swift @@ -0,0 +1,55 @@ +import BitFoundation +import Foundation + +struct BLELocalIdentitySnapshot: Equatable, Sendable { + let peerID: PeerID + let peerIDData: Data + let nickname: String +} + +/// Lock-backed local identity state shared by the transport's message, +/// Bluetooth, maintenance, and main-actor entry points. +/// +/// `peerID` and its binary wire representation must change as one unit during +/// panic rotation. A snapshot also gives announce construction one consistent +/// view of the nickname and identity instead of reading three independently +/// mutable properties across queues. +final class BLELocalIdentityStateStore: @unchecked Sendable { + private let lock = NSLock() + private var state: BLELocalIdentitySnapshot + + init( + peerID: PeerID = PeerID(str: ""), + nickname: String = "anon" + ) { + state = BLELocalIdentitySnapshot( + peerID: peerID, + peerIDData: Data(hexString: peerID.id) ?? Data(), + nickname: nickname + ) + } + + func snapshot() -> BLELocalIdentitySnapshot { + lock.withLock { state } + } + + func setNickname(_ nickname: String) { + lock.withLock { + state = BLELocalIdentitySnapshot( + peerID: state.peerID, + peerIDData: state.peerIDData, + nickname: nickname + ) + } + } + + func replacePeerIdentity(with peerID: PeerID) { + lock.withLock { + state = BLELocalIdentitySnapshot( + peerID: peerID, + peerIDData: Data(hexString: peerID.id) ?? Data(), + nickname: state.nickname + ) + } + } +} diff --git a/bitchat/Services/BLE/BLEService.swift b/bitchat/Services/BLE/BLEService.swift index 1f919a4a..51246deb 100644 --- a/bitchat/Services/BLE/BLEService.swift +++ b/bitchat/Services/BLE/BLEService.swift @@ -134,7 +134,7 @@ final class BLEService: NSObject { private let incomingFileStore = BLEIncomingFileStore() // Simple announce throttling - private var announceThrottle = BLEAnnounceThrottle() + private let announceThrottle = BLEAnnounceThrottle() // Application state tracking (thread-safe) #if os(iOS) @@ -162,9 +162,7 @@ final class BLEService: NSObject { private let identityManager: SecureIdentityStateManagerProtocol private let keychain: KeychainManagerProtocol private let idBridge: NostrIdentityBridge - /// Binary form of `myPeerID`; same contract — mutated only inside a - /// `messageQueue` barrier via `refreshPeerIdentity()`. - private var myPeerIDData: Data = Data() + private let localIdentityState = BLELocalIdentityStateStore() // MARK: - Advertising Privacy // No Local Name by default for maximum privacy. No rotating alias. @@ -417,7 +415,7 @@ final class BLEService: NSObject { } func resetIdentityForPanic(currentNickname: String) { - messageQueue.sync(flags: .barrier) { + collectionsQueue.sync(flags: .barrier) { pendingNoiseSessionQueues.removeAll() } @@ -537,20 +535,17 @@ final class BLEService: NSObject { // MARK: Identity - /// Derived from the Noise identity fingerprint; rotated only via - /// `refreshPeerIdentity()` (e.g. panic reset), which performs the swap - /// inside a `messageQueue` barrier so concurrent queue work never sees a - /// half-updated identity. Externally read-only — no out-of-band mutation - /// may bypass that derivation. - private(set) var myPeerID = PeerID(str: "") - /// Externally read-only; mutate via `setNickname(_:)`, which also - /// broadcasts the change to peers. - private(set) var myNickname: String = "anon" + /// Derived from the Noise identity fingerprint. Reads can originate from + /// the main actor, message queue, Bluetooth queue, and maintenance timer, + /// so all three local identity fields live in one lock-backed snapshot. + var myPeerID: PeerID { localIdentityState.snapshot().peerID } + var myNickname: String { localIdentityState.snapshot().nickname } + private var myPeerIDData: Data { localIdentityState.snapshot().peerIDData } /// Sole mutator for `myNickname`: updates the stored value and force-sends /// an announce so peers learn the new name. func setNickname(_ nickname: String) { - self.myNickname = nickname + localIdentityState.setNickname(nickname) // Send announce to notify peers of nickname change (force send) sendAnnounce(forceSend: true) } @@ -596,10 +591,11 @@ final class BLEService: NSObject { } func stopServices() { + let localIdentity = localIdentityState.snapshot() // Send leave message synchronously to ensure delivery var leavePacket = BitchatPacket( type: MessageType.leave.rawValue, - senderID: myPeerIDData, + senderID: localIdentity.peerIDData, recipientID: nil, timestamp: UInt64(Date().timeIntervalSince1970 * 1000), payload: Data(), @@ -1637,6 +1633,16 @@ final class BLEService: NSObject { } } private func sendAnnounce(forceSend: Bool = false) { + // Announce construction reads the replaceable Noise service and several + // related state snapshots. Serialize the whole operation with identity + // rotation instead of letting CoreBluetooth and maintenance callbacks + // execute it directly on their own queues. + messageQueue.async(flags: .barrier) { [weak self] in + self?.sendAnnounceNow(forceSend: forceSend) + } + } + + private func sendAnnounceNow(forceSend: Bool) { // Throttle announces to prevent flooding if !announceThrottle.shouldSend(force: forceSend, now: Date()) { return @@ -1656,8 +1662,9 @@ final class BLEService: NSObject { ) } + let localIdentity = localIdentityState.snapshot() let announcement = AnnouncementPacket( - nickname: myNickname, + nickname: localIdentity.nickname, noisePublicKey: noisePub, signingPublicKey: signingPub, directNeighbors: connectedPeerIDs, @@ -1673,7 +1680,7 @@ final class BLEService: NSObject { // Create packet with signature using the noise private key let packet = BitchatPacket( type: MessageType.announce.rawValue, - senderID: myPeerIDData, + senderID: localIdentity.peerIDData, recipientID: nil, timestamp: UInt64(Date().timeIntervalSince1970 * 1000), payload: payload, @@ -1687,14 +1694,7 @@ final class BLEService: NSObject { return } - // Call directly if on messageQueue, otherwise dispatch - if DispatchQueue.getSpecific(key: messageQueueKey) != nil { - broadcastPacket(signedPacket) - } else { - messageQueue.async { [weak self] in - self?.broadcastPacket(signedPacket) - } - } + broadcastPacket(signedPacket) // Ensure our own announce is included in sync state gossipSyncManager?.onPublicPacketSeen(signedPacket) @@ -3358,8 +3358,9 @@ extension BLEService { private func refreshPeerIdentity() { let swap = { let fingerprint = self.noiseService.getIdentityFingerprint() - self.myPeerID = PeerID(str: fingerprint.prefix(16)) - self.myPeerIDData = Data(hexString: self.myPeerID.id) ?? Data() + self.localIdentityState.replacePeerIdentity( + with: PeerID(str: fingerprint.prefix(16)) + ) self.meshTopology.reset() } if DispatchQueue.getSpecific(key: messageQueueKey) != nil { diff --git a/bitchatTests/Services/BLEAnnounceThrottleTests.swift b/bitchatTests/Services/BLEAnnounceThrottleTests.swift index 96dde95c..0ce23814 100644 --- a/bitchatTests/Services/BLEAnnounceThrottleTests.swift +++ b/bitchatTests/Services/BLEAnnounceThrottleTests.swift @@ -5,7 +5,7 @@ import Testing struct BLEAnnounceThrottleTests { @Test func firstAnnounceIsAllowed() { - var throttle = BLEAnnounceThrottle(normalMinimumInterval: 10, forcedMinimumInterval: 2) + let throttle = BLEAnnounceThrottle(normalMinimumInterval: 10, forcedMinimumInterval: 2) let shouldSend = throttle.shouldSend(force: false, now: Date(timeIntervalSince1970: 100)) @@ -15,7 +15,7 @@ struct BLEAnnounceThrottleTests { @Test func regularAnnounceUsesNormalMinimumInterval() { let now = Date(timeIntervalSince1970: 100) - var throttle = BLEAnnounceThrottle(normalMinimumInterval: 10, forcedMinimumInterval: 2) + let throttle = BLEAnnounceThrottle(normalMinimumInterval: 10, forcedMinimumInterval: 2) let first = throttle.shouldSend(force: false, now: now) let suppressed = throttle.shouldSend(force: false, now: now.addingTimeInterval(9.9)) @@ -29,7 +29,7 @@ struct BLEAnnounceThrottleTests { @Test func forcedAnnounceUsesShorterMinimumInterval() { let now = Date(timeIntervalSince1970: 100) - var throttle = BLEAnnounceThrottle(normalMinimumInterval: 10, forcedMinimumInterval: 2) + let throttle = BLEAnnounceThrottle(normalMinimumInterval: 10, forcedMinimumInterval: 2) let first = throttle.shouldSend(force: false, now: now) let suppressed = throttle.shouldSend(force: true, now: now.addingTimeInterval(1.9)) @@ -43,10 +43,40 @@ struct BLEAnnounceThrottleTests { @Test func elapsedReportsTimeSinceAcceptedSend() { let now = Date(timeIntervalSince1970: 100) - var throttle = BLEAnnounceThrottle(normalMinimumInterval: 10, forcedMinimumInterval: 2) + let throttle = BLEAnnounceThrottle(normalMinimumInterval: 10, forcedMinimumInterval: 2) _ = throttle.shouldSend(force: false, now: now) #expect(throttle.elapsed(since: now.addingTimeInterval(3)) == 3) } + + @Test + func concurrentRequestsAdmitOnlyOneAnnounce() { + let now = Date(timeIntervalSince1970: 100) + let throttle = BLEAnnounceThrottle( + normalMinimumInterval: 10, + forcedMinimumInterval: 2 + ) + let accepted = LockedCounter() + + DispatchQueue.concurrentPerform(iterations: 1_000) { _ in + if throttle.shouldSend(force: false, now: now) { + accepted.increment() + } + } + + #expect(accepted.value == 1) + #expect(throttle.elapsed(since: now.addingTimeInterval(3)) == 3) + } +} + +private final class LockedCounter: @unchecked Sendable { + private let lock = NSLock() + private var count = 0 + + var value: Int { lock.withLock { count } } + + func increment() { + lock.withLock { count += 1 } + } } diff --git a/bitchatTests/Services/BLELocalIdentityStateStoreTests.swift b/bitchatTests/Services/BLELocalIdentityStateStoreTests.swift new file mode 100644 index 00000000..ed10ab2f --- /dev/null +++ b/bitchatTests/Services/BLELocalIdentityStateStoreTests.swift @@ -0,0 +1,57 @@ +import BitFoundation +import Foundation +import Testing +@testable import bitchat + +struct BLELocalIdentityStateStoreTests { + @Test + func identityReplacementUpdatesWireBytesAtomically() throws { + let initial = PeerID(str: "0011223344556677") + let replacement = PeerID(str: "8899aabbccddeeff") + let store = BLELocalIdentityStateStore(peerID: initial, nickname: "alice") + + store.replacePeerIdentity(with: replacement) + + let snapshot = store.snapshot() + #expect(snapshot.peerID == replacement) + #expect(snapshot.peerIDData == Data(hexString: replacement.id)) + #expect(snapshot.nickname == "alice") + } + + @Test + func concurrentReadsNeverObserveSplitIdentityState() { + let peerIDs = [ + PeerID(str: "0011223344556677"), + PeerID(str: "8899aabbccddeeff") + ] + let store = BLELocalIdentityStateStore(peerID: peerIDs[0], nickname: "alice") + let failures = LockedFailureRecorder() + + DispatchQueue.concurrentPerform(iterations: 2_000) { index in + if index.isMultiple(of: 2) { + store.replacePeerIdentity(with: peerIDs[index % peerIDs.count]) + } else { + store.setNickname(index.isMultiple(of: 3) ? "alice" : "bob") + } + + let snapshot = store.snapshot() + let expectedWireID = Data(hexString: snapshot.peerID.id) ?? Data() + if snapshot.peerIDData != expectedWireID { + failures.record() + } + } + + #expect(!failures.hasFailure) + } +} + +private final class LockedFailureRecorder: @unchecked Sendable { + private let lock = NSLock() + private var failed = false + + var hasFailure: Bool { lock.withLock { failed } } + + func record() { + lock.withLock { failed = true } + } +}