Fix a data race that could manifest as null pointer dereference in FutureBase::Release() (#747)

Author: dconeybe

app/src/include/firebase/future.h

@@ -20,6 +20,7 @@
 #include <stddef.h>
 #include <stdint.h>
 
+#include <mutex>
 #include <utility>
 
 #include "firebase/internal/common.h"
@@ -309,6 +310,7 @@ class FutureBase {
 
   /// Returns true if the two Futures reference the same result.
   bool operator==(const FutureBase& rhs) const {
+    std::lock_guard<std::mutex> lock(mutex_);
     return api_ == rhs.api_ && handle_ == rhs.handle_;
   }
 
@@ -317,12 +319,17 @@ class FutureBase {
 
 #if defined(INTERNAL_EXPERIMENTAL)
   /// Returns the API-specific handle. Should only be called by the API.
-  const FutureHandle& GetHandle() const { return handle_; }
+  FutureHandle GetHandle() const {
+    std::lock_guard<std::mutex> lock(mutex_);
+    return handle_;
+  }
 #endif  // defined(INTERNAL_EXPERIMENTAL)
 
  protected:
   /// @cond FIREBASE_APP_INTERNAL
 
+  mutable std::mutex mutex_;
+
   /// Backpointer to the issuing API class.
   /// Set to nullptr when Future is invalidated.
   detail::FutureApiInterface* api_;

app/src/include/firebase/internal/future_impl.h

@@ -207,41 +207,63 @@ inline FutureBase::FutureBase(const FutureBase& rhs)
     : api_(NULL)  // NOLINT
 {                 // NOLINT
   *this = rhs;
-  detail::RegisterForCleanup(api_, this);
 }
 
 inline FutureBase& FutureBase::operator=(const FutureBase& rhs) {
   Release();
-  api_ = rhs.api_;
-  handle_ = rhs.handle_;
-  if (api_ != NULL) {  // NOLINT
-    api_->ReferenceFuture(handle_);
+
+  detail::FutureApiInterface* new_api;
+  FutureHandle new_handle;
+  {
+    std::lock_guard<std::mutex> lock(rhs.mutex_);
+    new_api = rhs.api_;
+    new_handle = rhs.handle_;
   }
-  detail::RegisterForCleanup(api_, this);
+
+  {
+    std::lock_guard<std::mutex> lock(mutex_);
+    api_ = new_api;
+    handle_ = new_handle;
+
+    if (api_ != NULL) {  // NOLINT
+      api_->ReferenceFuture(handle_);
+    }
+    detail::RegisterForCleanup(api_, this);
+  }
+
   return *this;
 }
 
 #if defined(FIREBASE_USE_MOVE_OPERATORS)
 inline FutureBase::FutureBase(FutureBase&& rhs) noexcept
     : api_(NULL)  // NOLINT
 {
-  detail::UnregisterForCleanup(rhs.api_, &rhs);
   *this = std::move(rhs);
-  detail::RegisterForCleanup(api_, this);
 }
 
 inline FutureBase& FutureBase::operator=(FutureBase&& rhs) noexcept {
   Release();
-  detail::UnregisterForCleanup(rhs.api_, &rhs);
-  api_ = rhs.api_;
-  handle_ = rhs.handle_;
-  rhs.api_ = NULL;  // NOLINT
+
+  detail::FutureApiInterface* new_api;
+  FutureHandle new_handle;
+  {
+    std::lock_guard<std::mutex> lock(rhs.mutex_);
+    detail::UnregisterForCleanup(rhs.api_, &rhs);
+    new_api = rhs.api_;
+    new_handle = rhs.handle_;
+    rhs.api_ = NULL;  // NOLINT
+  }
+
+  std::lock_guard<std::mutex> lock(mutex_);
+  api_ = new_api;
+  handle_ = new_handle;
   detail::RegisterForCleanup(api_, this);
   return *this;
 }
 #endif  // defined(FIREBASE_USE_MOVE_OPERATORS)
 
 inline void FutureBase::Release() {
+  std::lock_guard<std::mutex> lock(mutex_);
   if (api_ != NULL) {  // NOLINT
     detail::UnregisterForCleanup(api_, this);
     api_->ReleaseFuture(handle_);
@@ -250,25 +272,30 @@ inline void FutureBase::Release() {
 }
 
 inline FutureStatus FutureBase::status() const {
+  std::lock_guard<std::mutex> lock(mutex_);
   return api_ == NULL ?  // NOLINT
              kFutureStatusInvalid
                       : api_->GetFutureStatus(handle_);
 }
 
 inline int FutureBase::error() const {
+  std::lock_guard<std::mutex> lock(mutex_);
   return api_ == NULL ? -1 : api_->GetFutureError(handle_);  // NOLINT
 }
 
 inline const char* FutureBase::error_message() const {
+  std::lock_guard<std::mutex> lock(mutex_);
   return api_ == NULL ? NULL : api_->GetFutureErrorMessage(handle_);  // NOLINT
 }
 
 inline const void* FutureBase::result_void() const {
+  std::lock_guard<std::mutex> lock(mutex_);
   return api_ == NULL ? NULL : api_->GetFutureResult(handle_);  // NOLINT
 }
 
 inline void FutureBase::OnCompletion(CompletionCallback callback,
                                      void* user_data) const {
+  std::lock_guard<std::mutex> lock(mutex_);
   if (api_ != NULL) {  // NOLINT
     api_->AddCompletionCallback(handle_, callback, user_data, nullptr,
                                 /*clear_existing_callbacks=*/true);
@@ -278,6 +305,7 @@ inline void FutureBase::OnCompletion(CompletionCallback callback,
 #if defined(INTERNAL_EXPERIMENTAL)
 inline FutureBase::CompletionCallbackHandle FutureBase::AddOnCompletion(
     CompletionCallback callback, void* user_data) const {
+  std::lock_guard<std::mutex> lock(mutex_);
   if (api_ != NULL) {  // NOLINT
     return api_->AddCompletionCallback(handle_, callback, user_data, nullptr,
                                        /*clear_existing_callbacks=*/false);
@@ -287,6 +315,7 @@ inline FutureBase::CompletionCallbackHandle FutureBase::AddOnCompletion(
 
 inline void FutureBase::RemoveOnCompletion(
     CompletionCallbackHandle completion_handle) const {
+  std::lock_guard<std::mutex> lock(mutex_);
   if (api_ != NULL) {  // NOLINT
     api_->RemoveCompletionCallback(handle_, completion_handle);
   }
@@ -296,6 +325,7 @@ inline void FutureBase::RemoveOnCompletion(
 #if defined(FIREBASE_USE_STD_FUNCTION)
 inline void FutureBase::OnCompletion(
     std::function<void(const FutureBase&)> callback) const {
+  std::lock_guard<std::mutex> lock(mutex_);
   if (api_ != NULL) {  // NOLINT
     api_->AddCompletionCallbackLambda(handle_, callback,
                                       /*clear_existing_callbacks=*/true);
@@ -305,6 +335,7 @@ inline void FutureBase::OnCompletion(
 #if defined(INTERNAL_EXPERIMENTAL)
 inline FutureBase::CompletionCallbackHandle FutureBase::AddOnCompletion(
     std::function<void(const FutureBase&)> callback) const {
+  std::lock_guard<std::mutex> lock(mutex_);
   if (api_ != NULL) {  // NOLINT
     return api_->AddCompletionCallbackLambda(
         handle_, callback,

release_build_files/readme.md

@@ -569,6 +569,9 @@ code.
 ## Release Notes
 ### Next Release
 -   Changes
+    -   General (Android): Fixed a data race that could manifest as null pointer
+        dereference in `FutureBase::Release()`.
+        ([#747](https://github.com/firebase/firebase-cpp-sdk/pull/747))
     -   Auth (Desktop): Fixed a crash in `error_code()` when a request
         is cancelled or times out.
         ([#737](https://github.com/firebase/firebase-cpp-sdk/issues/737))