diff --git a/bitchat/Identity/SecureIdentityStateManager.swift b/bitchat/Identity/SecureIdentityStateManager.swift index 1531d3d4..f8ee80dd 100644 --- a/bitchat/Identity/SecureIdentityStateManager.swift +++ b/bitchat/Identity/SecureIdentityStateManager.swift @@ -243,20 +243,26 @@ final class SecureIdentityStateManager: SecureIdentityStateManagerProtocol { } /// Persists the cache. Always invoked on `queue` under a barrier (its callers - /// run inside `queue.async(.barrier)`), so it simply marks the cache dirty - /// and persists it on the same serialized context — no timer, nothing left - /// scheduled to keep the process alive. + /// run inside `queue.async(.barrier)`), so `cache` is read while serialized. + /// The encode + keychain write are done here (already on the exclusive + /// barrier context), so no separate hop is scheduled and nothing is left to + /// keep the process alive. private func saveIdentityCache() { pendingSave = true - performSave() + // On the barrier context already: snapshot is trivially consistent. + persist(snapshot: cache) + pendingSave = false } - /// Writes the cache to the keychain. Must run on `queue` with exclusive - /// (barrier) access. - private func performSave() { - guard pendingSave else { return } - pendingSave = false - + /// Encodes, seals, and writes a *snapshot* of the cache to the keychain. + /// + /// Takes the cache by value so callers can capture a consistent snapshot + /// under `queue` and then encode without holding it. Reading `cache` + /// concurrently with a barrier writer would be a data race on the + /// dictionary storage, which — because `JSONEncoder` walks that storage — + /// can spin forever (observed as a CI test-suite hang), so the snapshot + /// must be taken on `queue`, never off it. + private func persist(snapshot: IdentityCache) { // Never persist under an ephemeral key — it would overwrite the real // cache with data the next launch cannot decrypt. guard !encryptionKeyIsEphemeral else { @@ -265,7 +271,7 @@ final class SecureIdentityStateManager: SecureIdentityStateManagerProtocol { } do { - let data = try JSONEncoder().encode(cache) + let data = try JSONEncoder().encode(snapshot) let sealedBox = try AES.GCM.seal(data, using: encryptionKey) let saved = keychain.saveIdentityKey(sealedBox.combined!, forKey: cacheKey) if saved { @@ -276,14 +282,26 @@ final class SecureIdentityStateManager: SecureIdentityStateManagerProtocol { } } - // Force immediate save (for app termination / lifecycle events). Mutations - // already persist synchronously via saveIdentityCache, so this is normally a - // no-op (performSave early-returns when nothing is pending). Runs directly on - // the caller's thread — deliberately NOT a `queue.sync(barrier)`, which is - // reachable from `deinit` and from async tests on the swift-concurrency - // cooperative pool where a blocking barrier-sync can starve/deadlock it. + // Force a flush (for app termination / lifecycle events). Every mutating + // API already persists inline inside its own barrier via + // `saveIdentityCache`, so by the time this is called the keychain is + // already up to date and this is normally a no-op; it exists as a + // belt-and-suspenders flush of any `pendingSave` left set. + // + // The snapshot + persist run inside a `queue.async(flags: .barrier)`: + // reading `cache` on the barrier context is race-free (a plain off-queue + // read races in-flight barrier writers — JSONEncoder walking a + // concurrently-mutated dictionary can spin forever, which surfaced as a CI + // hang). It is deliberately `async`, NOT `sync`: `forceSave` is reachable + // from `deinit`, and the final release can itself run *on* `queue` (the + // fire-and-forget saves capture `self`), so a `queue.sync` here would be a + // re-entrant same-queue wait and deadlock (SIGTRAP). func forceSave() { - performSave() + queue.async(flags: .barrier) { [weak self] in + guard let self, self.pendingSave else { return } + self.pendingSave = false + self.persist(snapshot: self.cache) + } } // MARK: - Social Identity Management diff --git a/bitchatTests/Services/SecureIdentityStateManagerTests.swift b/bitchatTests/Services/SecureIdentityStateManagerTests.swift index 9d575d4e..33e36dc7 100644 --- a/bitchatTests/Services/SecureIdentityStateManagerTests.swift +++ b/bitchatTests/Services/SecureIdentityStateManagerTests.swift @@ -173,6 +173,55 @@ final class SecureIdentityStateManagerTests: XCTestCase { XCTAssertEqual(reloaded.getSocialIdentity(for: fingerprint)?.claimedNickname, "victim") } + func test_concurrentUpsertsAndForceSaveDoNotRaceOrHang() { + // Regression guard for a data race between a barrier writer mutating + // `cache` and `forceSave` encoding it off-queue: JSONEncoder walking a + // dictionary being concurrently mutated can spin forever (observed as + // a CI suite hang, killed at the watchdog timeout). forceSave must + // snapshot `cache` on `queue` before encoding. + // + // forceSave is funnelled through a single serial actor: the guard is + // for the manager's own `cache` race, and MockKeychain is not itself + // thread-safe (production forceSave is not called concurrently). The + // interleaving with the concurrent barrier writers is what matters. + let manager = SecureIdentityStateManager(LockedKeychain()) + let noiseKeys = (0..<32).map { Data(repeating: UInt8($0), count: 32) } + let forceSaveQueue = DispatchQueue(label: "test.forceSave.serial") + + let group = DispatchGroup() + for (index, noiseKey) in noiseKeys.enumerated() { + group.enter() + DispatchQueue.global().async { + let fingerprint = noiseKey.sha256Fingerprint() + for iteration in 0..<20 { + manager.upsertCryptographicIdentity( + fingerprint: fingerprint, + noisePublicKey: noiseKey, + signingPublicKey: Data(repeating: UInt8(index), count: 32), + claimedNickname: "peer-\(index)-\(iteration)" + ) + // Interleave a serialized off-queue save with the in-flight + // barrier writes from all the other threads. + forceSaveQueue.async { manager.forceSave() } + } + group.leave() + } + } + + let completed = group.wait(timeout: .now() + 20) + XCTAssertEqual(completed, .success, "concurrent upsert/forceSave workload hung") + forceSaveQueue.sync {} // drain outstanding saves + + // The pins landed and are readable (also fences pending barrier writes). + for (index, noiseKey) in noiseKeys.enumerated() { + let peerID = PeerID(publicKey: noiseKey) + XCTAssertEqual( + manager.getCryptoIdentitiesByPeerIDPrefix(peerID).first?.signingPublicKey, + Data(repeating: UInt8(index), count: 32) + ) + } + } + func test_setBlocked_clearsFavoriteState() async { let manager = SecureIdentityStateManager(MockKeychain()) let fingerprint = String(repeating: "ab", count: 32) @@ -523,6 +572,70 @@ final class SecureIdentityStateManagerTests: XCTestCase { } } +/// Thread-safe in-memory keychain for the concurrent stress test. MockKeychain +/// itself is not synchronized (production keychain access is serialized), so a +/// dedicated lock-guarded double is used so the test exercises the manager's +/// `cache` race rather than crashing on the double's own unsynchronized dict. +private final class LockedKeychain: KeychainManagerProtocol { + private let lock = NSLock() + private var storage: [String: Data] = [:] + private var serviceStorage: [String: [String: Data]] = [:] + + private func sync(_ body: () -> T) -> T { + lock.lock(); defer { lock.unlock() } + return body() + } + + func saveIdentityKey(_ keyData: Data, forKey key: String) -> Bool { + sync { storage[key] = keyData; return true } + } + + func getIdentityKey(forKey key: String) -> Data? { + sync { storage[key] } + } + + func deleteIdentityKey(forKey key: String) -> Bool { + sync { storage.removeValue(forKey: key); return true } + } + + func deleteAllKeychainData() -> Bool { + sync { storage.removeAll(); serviceStorage.removeAll(); return true } + } + + func secureClear(_ data: inout Data) { data = Data() } + func secureClear(_ string: inout String) { string = "" } + + func verifyIdentityKeyExists() -> Bool { + sync { storage["identity_noiseStaticKey"] != nil } + } + + func getIdentityKeyWithResult(forKey key: String) -> KeychainReadResult { + sync { + if let data = storage[key] { return .success(data) } + return .itemNotFound + } + } + + func saveIdentityKeyWithResult(_ keyData: Data, forKey key: String) -> KeychainSaveResult { + sync { storage[key] = keyData; return .success } + } + + func save(key: String, data: Data, service: String, accessible: CFString?) { + sync { + if serviceStorage[service] == nil { serviceStorage[service] = [:] } + serviceStorage[service]?[key] = data + } + } + + func load(key: String, service: String) -> Data? { + sync { serviceStorage[service]?[key] } + } + + func delete(key: String, service: String) { + sync { serviceStorage[service]?.removeValue(forKey: key) } + } +} + private final class FailingCacheSaveKeychain: KeychainManagerProtocol { private var storage: [String: Data] = [:] private var serviceStorage: [String: [String: Data]] = [:]