From 84f9da42b412832a8813494b2a42431392d55f03 Mon Sep 17 00:00:00 2001 From: Julio Buendia Date: Thu, 20 Aug 2026 15:27:12 -0700 Subject: [PATCH] fix(firestore,ios): guard the shared transactions map against concurrent access `transactions` is a file-scope static NSMutableDictionary shared by every in-flight Firestore transaction, but every @synchronized in this file locks the per-transaction `transactionState` object instead of the container. Two transactions beginning at once therefore hold two different locks while mutating the same dictionary from different threads, corrupting its internal hash table. The next lookup dereferences a null bucket pointer and the app dies with EXC_BAD_ACCESS in mdict_index_for_key. Add a dedicated `transactionsLock` covering every access to the container and nothing else: the insert in transactionBegin, the remove in the completion block, invalidate, and the three lookup methods. The lookups previously opened with `@synchronized(transactions[key])`, which is `@synchronized(nil)` -- a silent no-op -- whenever the key is absent, and which read the shared dictionary outside any lock in order to acquire that lock. They now resolve the state under the container lock, release it, and only then lock the state. Lock ordering is state -> container everywhere, and the lookups never hold the container lock while acquiring a state lock, so there is no reverse-order hold-and-wait. The container lock is never held across the semaphore wait or the event dispatch. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01GhxoXDyQvDTCxak1GSmVpr --- .../RNFBFirestoreTransactionModule.mm | 68 +++++++++++++------ 1 file changed, 46 insertions(+), 22 deletions(-) diff --git a/packages/firestore/ios/RNFBFirestore/RNFBFirestoreTransactionModule.mm b/packages/firestore/ios/RNFBFirestore/RNFBFirestoreTransactionModule.mm index 98fcd2423f..52d7130418 100644 --- a/packages/firestore/ios/RNFBFirestore/RNFBFirestoreTransactionModule.mm +++ b/packages/firestore/ios/RNFBFirestore/RNFBFirestoreTransactionModule.mm @@ -24,8 +24,24 @@ #import "RNFBFirestoreTurboModules.h" static __strong NSMutableDictionary *transactions; +// Guards access to the `transactions` CONTAINER itself (get/set/remove) -- nothing else. +// Every other @synchronized in this file locks a per-transaction `transactionState`, which +// serialises two callers touching the SAME transaction but does nothing for two DIFFERENT +// transactions mutating this shared dictionary concurrently. That race corrupts the +// dictionary's internal hash table and crashes with EXC_BAD_ACCESS in mdict_index_for_key. +// Never hold this lock across the semaphore wait or an event dispatch. +static __strong NSObject *transactionsLock; static NSString *const RNFB_FIRESTORE_TRANSACTION_EVENT = @"firestore_transaction_event"; +// Look a transaction's state up under the container lock and release that lock before the +// caller locks the state. This also replaces `@synchronized(transactions[key])`, which locks +// NOTHING when the key is absent -- @synchronized(nil) is a no-op. +static NSMutableDictionary *RNFBTransactionStateFor(NSString *key) { + @synchronized(transactionsLock) { + return transactions[key]; + } +} + @interface RNFBFirestoreTransactionModule () @end @@ -46,6 +62,7 @@ - (id)init { static dispatch_once_t onceToken; dispatch_once(&onceToken, ^{ transactions = [[NSMutableDictionary alloc] init]; + transactionsLock = [NSObject new]; }); return self; } @@ -59,8 +76,8 @@ - (void)dealloc { } - (void)invalidate { - for (NSString *key in [transactions allKeys]) { - [transactions removeObjectForKey:key]; + @synchronized(transactionsLock) { + [transactions removeAllObjects]; } } @@ -86,8 +103,10 @@ - (void)transactionBegin:(NSString *)appName transactionState[@"semaphore"] = semaphore; transactionState[@"transaction"] = transaction; - if (!transactions[[transactionIdNumber stringValue]]) { - transactions[[transactionIdNumber stringValue]] = transactionState; + @synchronized(transactionsLock) { + if (!transactions[[transactionIdNumber stringValue]]) { + transactions[[transactionIdNumber stringValue]] = transactionState; + } } dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ @@ -188,7 +207,9 @@ - (void)transactionBegin:(NSString *)appName }]; } - [transactions removeObjectForKey:[transactionIdNumber stringValue]]; + @synchronized(transactionsLock) { + [transactions removeObjectForKey:[transactionIdNumber stringValue]]; + } } }; @@ -210,14 +231,15 @@ - (void)transactionGetDocument:(NSString *)appName FIRApp *firebaseApp = [RCTConvert firAppFromString:appName]; NSNumber *transactionIdNumber = @(transactionId); - @synchronized(transactions[[transactionIdNumber stringValue]]) { - NSMutableDictionary *transactionState = transactions[[transactionIdNumber stringValue]]; + NSMutableDictionary *transactionState = + RNFBTransactionStateFor([transactionIdNumber stringValue]); - if (!transactionState) { - DLog(@"transactionGetDocument called for non-existent transactionId %@", transactionIdNumber); - return; - } + if (!transactionState) { + DLog(@"transactionGetDocument called for non-existent transactionId %@", transactionIdNumber); + return; + } + @synchronized(transactionState) { NSError *error = nil; FIRTransaction *transaction = [transactionState valueForKey:@"transaction"]; FIRFirestore *firestore = [RNFBFirestoreCommon getFirestoreForApp:firebaseApp @@ -249,13 +271,14 @@ - (void)transactionDispose:(NSString *)appName transactionId:(double)transactionId { NSNumber *transactionIdNumber = @(transactionId); - @synchronized(transactions[[transactionIdNumber stringValue]]) { - NSMutableDictionary *transactionState = transactions[[transactionIdNumber stringValue]]; + NSMutableDictionary *transactionState = + RNFBTransactionStateFor([transactionIdNumber stringValue]); - if (!transactionState) { - return; - } + if (!transactionState) { + return; + } + @synchronized(transactionState) { dispatch_semaphore_t semaphore = transactionState[@"semaphore"]; transactionState[@"aborted"] = @(true); dispatch_semaphore_signal(semaphore); @@ -268,14 +291,15 @@ - (void)transactionApplyBuffer:(NSString *)appName commandBuffer:(NSArray *)commandBuffer { NSNumber *transactionIdNumber = @(transactionId); - @synchronized(transactions[[transactionIdNumber stringValue]]) { - NSMutableDictionary *transactionState = transactions[[transactionIdNumber stringValue]]; + NSMutableDictionary *transactionState = + RNFBTransactionStateFor([transactionIdNumber stringValue]); - if (!transactionState) { - DLog(@"transactionApplyBuffer called for non-existent transactionId %@", transactionIdNumber); - return; - } + if (!transactionState) { + DLog(@"transactionApplyBuffer called for non-existent transactionId %@", transactionIdNumber); + return; + } + @synchronized(transactionState) { dispatch_semaphore_t semaphore = [transactionState valueForKey:@"semaphore"]; [transactionState setValue:commandBuffer forKey:@"commandBuffer"]; dispatch_semaphore_signal(semaphore);