Skip to content

Commit e81cb6d

Browse files
josephysicist1josephysicist1
authored
fix(cloud_firestore): guard shared transactions map against concurrent access (#18421)
On iOS, the plugin's _transactions NSMutableDictionary is mutated from Firestore's transaction worker queue (started:/ended: listeners) and read from a global queue (transactionGetApp), with no synchronization. Running many transactions in parallel corrupts the dictionary and aborts the process with SIGABRT (malloc heap-corruption check). Wrap all remaining accesses in @synchronized(self->_transactions), matching the existing synchronization in cleanupEventListeners. The macOS sources symlink to the iOS files, so this covers both platforms. Android has the identical race (plain HashMap written from transaction worker threads, read from the plugin's cached thread pool); switch it to ConcurrentHashMap. Adds an e2e regression test running 30 transactions concurrently. Fixes #18417
1 parent 1df97bb commit e81cb6d

3 files changed

Lines changed: 49 additions & 4 deletions

File tree

‎packages/cloud_firestore/cloud_firestore/android/src/main/java/io/flutter/plugins/firebase/firestore/FlutterFirebaseFirestorePlugin.java‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,7 @@
6262
import java.util.Map;
6363
import java.util.Objects;
6464
import java.util.UUID;
65+
import java.util.concurrent.ConcurrentHashMap;
6566
import java.util.concurrent.atomic.AtomicReference;
6667

6768
public class FlutterFirebaseFirestorePlugin
@@ -83,7 +84,9 @@ public class FlutterFirebaseFirestorePlugin
8384

8485
private final AtomicReference<Activity> activity = new AtomicReference<>(null);
8586

86-
private final Map<String, Transaction> transactions = new HashMap<>();
87+
// Written from Firestore's transaction worker threads and read from the plugin's
88+
// cached thread pool, so this must be a thread-safe map (see #18417).
89+
private final Map<String, Transaction> transactions = new ConcurrentHashMap<>();
8790
private final Map<String, EventChannel> eventChannels = new HashMap<>();
8891
private final Map<String, StreamHandler> streamHandlers = new HashMap<>();
8992
private final Map<String, OnTransactionResultListener> transactionHandlers = new HashMap<>();

‎packages/cloud_firestore/cloud_firestore/example/integration_test/transaction_e2e.dart‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -481,6 +481,39 @@ void runTransactionTests() {
481481
expect(snapshot2.exists, isFalse);
482482
});
483483

484+
test(
485+
'runs many transactions concurrently without corrupting native state',
486+
() async {
487+
// Regression test for
488+
// https://github.com/firebase/flutterfire/issues/18417: concurrent
489+
// transactions used to mutate the plugin's shared transaction map
490+
// from multiple threads without synchronization, which could crash
491+
// iOS with a heap-corruption SIGABRT.
492+
const int count = 30;
493+
494+
final refs = [
495+
for (var i = 0; i < count; i++)
496+
firestore.doc('flutter-tests/transaction-concurrent-$i'),
497+
];
498+
499+
await Future.wait([
500+
for (final ref in refs)
501+
firestore.runTransaction((Transaction transaction) async {
502+
final snapshot = await transaction.get(ref);
503+
transaction.set(ref, {
504+
'value': ((snapshot.data()?['value'] as int?) ?? 0) + 1,
505+
});
506+
}),
507+
]);
508+
509+
final snapshots = await Future.wait(refs.map((ref) => ref.get()));
510+
for (final snapshot in snapshots) {
511+
expect(snapshot.exists, isTrue);
512+
expect(snapshot.data()!['value'], isA<int>());
513+
}
514+
},
515+
);
516+
484517
// TODO(Lyokone): adding auth make some tests fails in macOS
485518
// test(
486519
// 'should not fail to complete transaction if user is authenticated',

‎packages/cloud_firestore/cloud_firestore/ios/cloud_firestore/Sources/cloud_firestore/FLTFirebaseFirestorePlugin.m‎

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -662,7 +662,10 @@ - (void)transactionGetApp:(nonnull FirestorePigeonFirebaseApp *)app
662662
FIRFirestore *firestore = [self getFIRFirestoreFromAppNameFromPigeon:app];
663663
FIRDocumentReference *document = [firestore documentWithPath:path];
664664

665-
FIRTransaction *transaction = self->_transactions[transactionId];
665+
FIRTransaction *transaction;
666+
@synchronized(self->_transactions) {
667+
transaction = self->_transactions[transactionId];
668+
}
666669

667670
if (transaction == nil) {
668671
completion(
@@ -782,10 +785,16 @@ - (void)transactionCreateApp:(nonnull FirestorePigeonFirebaseApp *)app
782785
timeout:timeout
783786
maxAttempts:maxAttempts
784787
started:^(FIRTransaction *_Nonnull transaction) {
785-
self->_transactions[transactionId] = transaction;
788+
// Called from Firestore's transaction worker queue; multiple
789+
// in-flight transactions may hit this concurrently.
790+
@synchronized(self->_transactions) {
791+
self->_transactions[transactionId] = transaction;
792+
}
786793
}
787794
ended:^{
788-
self->_transactions[transactionId] = nil;
795+
@synchronized(self->_transactions) {
796+
[self->_transactions removeObjectForKey:transactionId];
797+
}
789798
}];
790799

791800
_transactionHandlers[transactionId] = handler;

0 commit comments

Comments
 (0)