Fixed some potential race conditions in DatabaseReference's various

constructors. Because the database keeps its own bookkeeping regarding the memory addresses of the QueryInternals and DatabaseReferenceInternals, if they are created and added to the Database's registry in two steps, a situation could arise where the database shuts down with either stale or incomplete internal bookkeeping. To solve this, any place that we update Database's bookkeeping we need do so atomically. PiperOrigin-RevId: 265136296
firebase · Sep 7, 2019 · 493e97b · 493e97b
1 parent 0793d9e
commit 493e97b
Show file tree

Hide file tree

Showing 8 changed files with 31 additions and 3 deletions.
diff --git a/database/src/android/database_android.cc b/database/src/android/database_android.cc
@@ -34,6 +34,8 @@ namespace firebase {
 namespace database {
 namespace internal {
 
+Mutex g_database_reference_constructor_mutex;  // NOLINT
+
 const char kApiIdentifier[] = "Database";
 
 // clang-format off

diff --git a/database/src/android/database_android.h b/database/src/android/database_android.h
@@ -36,6 +36,9 @@ class ReferenceCountedFutureImpl;
 namespace database {
 namespace internal {
 
+// For constructing, copying or moving DatabaseReferences atomically.
+extern Mutex g_database_reference_constructor_mutex;
+
 // Used for registering global callbacks. See
 // firebase::util::RegisterCallbackOnTask for context.
 extern const char kApiIdentifier[];

diff --git a/database/src/common/database.cc b/database/src/common/database.cc
@@ -133,8 +133,11 @@ void Database::DeleteInternal() {
     app_notifier->UnregisterObject(this);
   }
 
-  // Force cleanup to happen first.
-  internal_->cleanup().CleanupAll();
+  {
+    MutexLock db_ref_lock(internal::g_database_reference_constructor_mutex);
+    // Force cleanup to happen first.
+    internal_->cleanup().CleanupAll();
+  }
   delete internal_;
   internal_ = nullptr;
   // If a Database is explicitly deleted, remove it from our cache.

diff --git a/database/src/common/database_reference.cc b/database/src/common/database_reference.cc
@@ -77,11 +77,15 @@ void DatabaseReference::SwitchCleanupRegistrationBackToQuery() {
 
 DatabaseReference::DatabaseReference(DatabaseReferenceInternal* internal)
     : Query(internal), internal_(internal) {
+  MutexLock lock(internal::g_database_reference_constructor_mutex);
+
   SwitchCleanupRegistrationToDatabaseReference();
 }
 
 DatabaseReference::DatabaseReference(const DatabaseReference& reference)
     : Query(), internal_(nullptr) {
+  MutexLock lock(internal::g_database_reference_constructor_mutex);
+
   internal_ = reference.internal_
                   ? new DatabaseReferenceInternal(*reference.internal_)
                   : nullptr;
@@ -91,6 +95,8 @@ DatabaseReference::DatabaseReference(const DatabaseReference& reference)
 
 DatabaseReference& DatabaseReference::operator=(
     const DatabaseReference& reference) {
+  MutexLock lock(internal::g_database_reference_constructor_mutex);
+
   internal_ = reference.internal_
                   ? new DatabaseReferenceInternal(*reference.internal_)
                   : nullptr;
@@ -103,12 +109,16 @@ DatabaseReference& DatabaseReference::operator=(
 #if defined(FIREBASE_USE_MOVE_OPERATORS) || defined(DOXYGEN)
 DatabaseReference::DatabaseReference(DatabaseReference&& reference)
     : Query(), internal_(reference.internal_) {
-  Query::operator=(std::move(reference));
+  MutexLock lock(internal::g_database_reference_constructor_mutex);
+
   reference.internal_ = nullptr;
+  Query::operator=(std::move(reference));
   SwitchCleanupRegistrationToDatabaseReference();
 }
 
 DatabaseReference& DatabaseReference::operator=(DatabaseReference&& reference) {
+  MutexLock lock(internal::g_database_reference_constructor_mutex);
+
   internal_ = reference.internal_;
   reference.internal_ = nullptr;
   Query::operator=(std::move(reference));

diff --git a/database/src/desktop/database_desktop.cc b/database/src/desktop/database_desktop.cc
@@ -39,6 +39,8 @@ using callback::NewCallback;
 namespace database {
 namespace internal {
 
+Mutex g_database_reference_constructor_mutex;  // NOLINT
+
 SingleValueListener::SingleValueListener(DatabaseInternal* database,
                                          ReferenceCountedFutureImpl* future,
                                          SafeFutureHandle<DataSnapshot> handle)

diff --git a/database/src/desktop/database_desktop.h b/database/src/desktop/database_desktop.h
@@ -40,6 +40,9 @@ namespace firebase {
 namespace database {
 namespace internal {
 
+// For constructing, copying or moving DatabaseReferences atomically.
+extern Mutex g_database_reference_constructor_mutex;
+
 typedef int64_t WriteId;
 
 class SingleValueListener : public ValueListener {

diff --git a/database/src/ios/database_ios.h b/database/src/ios/database_ios.h
@@ -37,6 +37,9 @@ class ReferenceCountedFutureImpl;
 namespace database {
 namespace internal {
 
+// For constructing, copying or moving DatabaseReferences atomically.
+extern Mutex g_database_reference_constructor_mutex;
+
 // This defines the class FIRDatabasePointer, which is a C++-compatible wrapper
 // around the FIRDatabase Obj-C class.
 OBJ_C_PTR_WRAPPER(FIRDatabase);

diff --git a/database/src/ios/database_ios.mm b/database/src/ios/database_ios.mm
@@ -26,6 +26,8 @@
 namespace database {
 namespace internal {
 
+Mutex g_database_reference_constructor_mutex;  // NOLINT
+
 DatabaseInternal::DatabaseInternal(App* app)
     : app_(app), log_level_(kLogLevelInfo) {
   @try {