From 7788946b2d37c90dfa70a7bd5278234ff3aa0e93 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 11 Nov 2021 15:34:28 -0500 Subject: [PATCH 1/7] Fix race condition in Future/FutureBase --- app/src/include/firebase/future.h | 9 ++- .../include/firebase/internal/future_impl.h | 59 +++++++++++++++---- 2 files changed, 56 insertions(+), 12 deletions(-) diff --git a/app/src/include/firebase/future.h b/app/src/include/firebase/future.h index 15c42d5352..ae41d72f17 100644 --- a/app/src/include/firebase/future.h +++ b/app/src/include/firebase/future.h @@ -22,6 +22,7 @@ #include +#include "app/src/mutex.h" #include "firebase/internal/common.h" #ifdef FIREBASE_USE_STD_FUNCTION @@ -309,6 +310,7 @@ class FutureBase { /// Returns true if the two Futures reference the same result. bool operator==(const FutureBase& rhs) const { + MutexLock 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 { + MutexLock lock(mutex_); + return handle_; + } #endif // defined(INTERNAL_EXPERIMENTAL) protected: /// @cond FIREBASE_APP_INTERNAL + mutable Mutex mutex_; + /// Backpointer to the issuing API class. /// Set to nullptr when Future is invalidated. detail::FutureApiInterface* api_; diff --git a/app/src/include/firebase/internal/future_impl.h b/app/src/include/firebase/internal/future_impl.h index 5e4d0a74a0..0be0dc223a 100644 --- a/app/src/include/firebase/internal/future_impl.h +++ b/app/src/include/firebase/internal/future_impl.h @@ -212,12 +212,26 @@ inline FutureBase::FutureBase(const FutureBase& rhs) 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; + { + MutexLock lock(rhs.mutex_); + new_api = rhs.api_; + new_handle = rhs.handle_; } - detail::RegisterForCleanup(api_, this); + + { + MutexLock lock(mutex_); + api_ = new_api; + handle_ = new_handle; + + if (api_ != NULL) { // NOLINT + api_->ReferenceFuture(handle_); + } + detail::RegisterForCleanup(api_, this); + } + return *this; } @@ -225,23 +239,37 @@ inline FutureBase& FutureBase::operator=(const FutureBase& rhs) { inline FutureBase::FutureBase(FutureBase&& rhs) noexcept : api_(NULL) // NOLINT { - detail::UnregisterForCleanup(rhs.api_, &rhs); - *this = std::move(rhs); + { + MutexLock lock(rhs.mutex_); + 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; + { + MutexLock lock(rhs.mutex_); + detail::UnregisterForCleanup(rhs.api_, &rhs); + rhs.api_ = NULL; // NOLINT + new_api = rhs.api_; + new_handle = rhs.handle_; + } + + MutexLock lock(mutex_); + api_ = new_api; + handle_ = new_handle; detail::RegisterForCleanup(api_, this); return *this; } #endif // defined(FIREBASE_USE_MOVE_OPERATORS) inline void FutureBase::Release() { + MutexLock lock(mutex_); if (api_ != NULL) { // NOLINT detail::UnregisterForCleanup(api_, this); api_->ReleaseFuture(handle_); @@ -250,25 +278,30 @@ inline void FutureBase::Release() { } inline FutureStatus FutureBase::status() const { + MutexLock lock(mutex_); return api_ == NULL ? // NOLINT kFutureStatusInvalid : api_->GetFutureStatus(handle_); } inline int FutureBase::error() const { + MutexLock lock(mutex_); return api_ == NULL ? -1 : api_->GetFutureError(handle_); // NOLINT } inline const char* FutureBase::error_message() const { + MutexLock lock(mutex_); return api_ == NULL ? NULL : api_->GetFutureErrorMessage(handle_); // NOLINT } inline const void* FutureBase::result_void() const { + MutexLock lock(mutex_); return api_ == NULL ? NULL : api_->GetFutureResult(handle_); // NOLINT } inline void FutureBase::OnCompletion(CompletionCallback callback, void* user_data) const { + MutexLock lock(mutex_); if (api_ != NULL) { // NOLINT api_->AddCompletionCallback(handle_, callback, user_data, nullptr, /*clear_existing_callbacks=*/true); @@ -278,6 +311,7 @@ inline void FutureBase::OnCompletion(CompletionCallback callback, #if defined(INTERNAL_EXPERIMENTAL) inline FutureBase::CompletionCallbackHandle FutureBase::AddOnCompletion( CompletionCallback callback, void* user_data) const { + MutexLock lock(mutex_); if (api_ != NULL) { // NOLINT return api_->AddCompletionCallback(handle_, callback, user_data, nullptr, /*clear_existing_callbacks=*/false); @@ -287,6 +321,7 @@ inline FutureBase::CompletionCallbackHandle FutureBase::AddOnCompletion( inline void FutureBase::RemoveOnCompletion( CompletionCallbackHandle completion_handle) const { + MutexLock lock(mutex_); if (api_ != NULL) { // NOLINT api_->RemoveCompletionCallback(handle_, completion_handle); } @@ -296,6 +331,7 @@ inline void FutureBase::RemoveOnCompletion( #if defined(FIREBASE_USE_STD_FUNCTION) inline void FutureBase::OnCompletion( std::function callback) const { + MutexLock lock(mutex_); if (api_ != NULL) { // NOLINT api_->AddCompletionCallbackLambda(handle_, callback, /*clear_existing_callbacks=*/true); @@ -305,6 +341,7 @@ inline void FutureBase::OnCompletion( #if defined(INTERNAL_EXPERIMENTAL) inline FutureBase::CompletionCallbackHandle FutureBase::AddOnCompletion( std::function callback) const { + MutexLock lock(mutex_); if (api_ != NULL) { // NOLINT return api_->AddCompletionCallbackLambda( handle_, callback, From 52c6da13bdbe5349d72047c364abdd118cc3cfd1 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 12 Nov 2021 01:02:47 -0500 Subject: [PATCH 2/7] Add release notes entry skeleton --- release_build_files/readme.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/release_build_files/readme.md b/release_build_files/readme.md index 739dc91794..5dd474008a 100644 --- a/release_build_files/readme.md +++ b/release_build_files/readme.md @@ -569,6 +569,9 @@ code. ## Release Notes ### Next Release - Changes + - All Products (Android): Fixed a data race that could manifest as null + pointer dereference in FutureBase::Release() + ([#NNN](https://github.com/firebase/firebase-cpp-sdk/issues/NNN)) - 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)) From 18e0cc481ec77077361eb74fd15a45ccaf914fcf Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 12 Nov 2021 01:10:03 -0500 Subject: [PATCH 3/7] Add a link to the PR in the release notes --- release_build_files/readme.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/release_build_files/readme.md b/release_build_files/readme.md index 5dd474008a..09fe8a50d9 100644 --- a/release_build_files/readme.md +++ b/release_build_files/readme.md @@ -570,8 +570,8 @@ code. ### Next Release - Changes - All Products (Android): Fixed a data race that could manifest as null - pointer dereference in FutureBase::Release() - ([#NNN](https://github.com/firebase/firebase-cpp-sdk/issues/NNN)) + 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)) From 17d6beba4f0e637af3ba509e4dc6484c4ba050d5 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Mon, 15 Nov 2021 11:31:01 -0500 Subject: [PATCH 4/7] future_impl.h: Fix a bug in FutureBase::operator=(FutureBase&&) where api_ was always assigned to null --- app/src/include/firebase/internal/future_impl.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/src/include/firebase/internal/future_impl.h b/app/src/include/firebase/internal/future_impl.h index 0be0dc223a..873426a51b 100644 --- a/app/src/include/firebase/internal/future_impl.h +++ b/app/src/include/firebase/internal/future_impl.h @@ -255,9 +255,9 @@ inline FutureBase& FutureBase::operator=(FutureBase&& rhs) noexcept { { MutexLock lock(rhs.mutex_); detail::UnregisterForCleanup(rhs.api_, &rhs); - rhs.api_ = NULL; // NOLINT new_api = rhs.api_; new_handle = rhs.handle_; + rhs.api_ = NULL; // NOLINT } MutexLock lock(mutex_); From ca4a566328791d1362d50f1495ffe70539f29373 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Mon, 15 Nov 2021 11:31:34 -0500 Subject: [PATCH 5/7] future_impl.h: Remove code from the copy/move constructors that is duplicated in the call to the copy/move operators --- app/src/include/firebase/internal/future_impl.h | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/app/src/include/firebase/internal/future_impl.h b/app/src/include/firebase/internal/future_impl.h index 873426a51b..9e465f2724 100644 --- a/app/src/include/firebase/internal/future_impl.h +++ b/app/src/include/firebase/internal/future_impl.h @@ -207,7 +207,6 @@ inline FutureBase::FutureBase(const FutureBase& rhs) : api_(NULL) // NOLINT { // NOLINT *this = rhs; - detail::RegisterForCleanup(api_, this); } inline FutureBase& FutureBase::operator=(const FutureBase& rhs) { @@ -239,12 +238,7 @@ inline FutureBase& FutureBase::operator=(const FutureBase& rhs) { inline FutureBase::FutureBase(FutureBase&& rhs) noexcept : api_(NULL) // NOLINT { - { - MutexLock lock(rhs.mutex_); - detail::UnregisterForCleanup(rhs.api_, &rhs); - *this = std::move(rhs); - } - detail::RegisterForCleanup(api_, this); + *this = std::move(rhs); } inline FutureBase& FutureBase::operator=(FutureBase&& rhs) noexcept { From 4be3dd774cb590e8d561128775fa9976181ddc06 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Mon, 15 Nov 2021 13:13:48 -0500 Subject: [PATCH 6/7] Swap out firebase::Mutex with std::mutex since future.h cannot include private headers --- app/src/include/firebase/future.h | 8 +++--- .../include/firebase/internal/future_impl.h | 28 +++++++++---------- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/app/src/include/firebase/future.h b/app/src/include/firebase/future.h index ae41d72f17..3f63b3e079 100644 --- a/app/src/include/firebase/future.h +++ b/app/src/include/firebase/future.h @@ -20,9 +20,9 @@ #include #include +#include #include -#include "app/src/mutex.h" #include "firebase/internal/common.h" #ifdef FIREBASE_USE_STD_FUNCTION @@ -310,7 +310,7 @@ class FutureBase { /// Returns true if the two Futures reference the same result. bool operator==(const FutureBase& rhs) const { - MutexLock lock(mutex_); + std::lock_guard lock(mutex_); return api_ == rhs.api_ && handle_ == rhs.handle_; } @@ -320,7 +320,7 @@ class FutureBase { #if defined(INTERNAL_EXPERIMENTAL) /// Returns the API-specific handle. Should only be called by the API. FutureHandle GetHandle() const { - MutexLock lock(mutex_); + std::lock_guard lock(mutex_); return handle_; } #endif // defined(INTERNAL_EXPERIMENTAL) @@ -328,7 +328,7 @@ class FutureBase { protected: /// @cond FIREBASE_APP_INTERNAL - mutable Mutex mutex_; + mutable std::mutex mutex_; /// Backpointer to the issuing API class. /// Set to nullptr when Future is invalidated. diff --git a/app/src/include/firebase/internal/future_impl.h b/app/src/include/firebase/internal/future_impl.h index 9e465f2724..dd95defc03 100644 --- a/app/src/include/firebase/internal/future_impl.h +++ b/app/src/include/firebase/internal/future_impl.h @@ -215,13 +215,13 @@ inline FutureBase& FutureBase::operator=(const FutureBase& rhs) { detail::FutureApiInterface* new_api; FutureHandle new_handle; { - MutexLock lock(rhs.mutex_); + std::lock_guard lock(rhs.mutex_); new_api = rhs.api_; new_handle = rhs.handle_; } { - MutexLock lock(mutex_); + std::lock_guard lock(mutex_); api_ = new_api; handle_ = new_handle; @@ -247,14 +247,14 @@ inline FutureBase& FutureBase::operator=(FutureBase&& rhs) noexcept { detail::FutureApiInterface* new_api; FutureHandle new_handle; { - MutexLock lock(rhs.mutex_); + std::lock_guard lock(rhs.mutex_); detail::UnregisterForCleanup(rhs.api_, &rhs); new_api = rhs.api_; new_handle = rhs.handle_; rhs.api_ = NULL; // NOLINT } - MutexLock lock(mutex_); + std::lock_guard lock(mutex_); api_ = new_api; handle_ = new_handle; detail::RegisterForCleanup(api_, this); @@ -263,7 +263,7 @@ inline FutureBase& FutureBase::operator=(FutureBase&& rhs) noexcept { #endif // defined(FIREBASE_USE_MOVE_OPERATORS) inline void FutureBase::Release() { - MutexLock lock(mutex_); + std::lock_guard lock(mutex_); if (api_ != NULL) { // NOLINT detail::UnregisterForCleanup(api_, this); api_->ReleaseFuture(handle_); @@ -272,30 +272,30 @@ inline void FutureBase::Release() { } inline FutureStatus FutureBase::status() const { - MutexLock lock(mutex_); + std::lock_guard lock(mutex_); return api_ == NULL ? // NOLINT kFutureStatusInvalid : api_->GetFutureStatus(handle_); } inline int FutureBase::error() const { - MutexLock lock(mutex_); + std::lock_guard lock(mutex_); return api_ == NULL ? -1 : api_->GetFutureError(handle_); // NOLINT } inline const char* FutureBase::error_message() const { - MutexLock lock(mutex_); + std::lock_guard lock(mutex_); return api_ == NULL ? NULL : api_->GetFutureErrorMessage(handle_); // NOLINT } inline const void* FutureBase::result_void() const { - MutexLock lock(mutex_); + std::lock_guard lock(mutex_); return api_ == NULL ? NULL : api_->GetFutureResult(handle_); // NOLINT } inline void FutureBase::OnCompletion(CompletionCallback callback, void* user_data) const { - MutexLock lock(mutex_); + std::lock_guard lock(mutex_); if (api_ != NULL) { // NOLINT api_->AddCompletionCallback(handle_, callback, user_data, nullptr, /*clear_existing_callbacks=*/true); @@ -305,7 +305,7 @@ inline void FutureBase::OnCompletion(CompletionCallback callback, #if defined(INTERNAL_EXPERIMENTAL) inline FutureBase::CompletionCallbackHandle FutureBase::AddOnCompletion( CompletionCallback callback, void* user_data) const { - MutexLock lock(mutex_); + std::lock_guard lock(mutex_); if (api_ != NULL) { // NOLINT return api_->AddCompletionCallback(handle_, callback, user_data, nullptr, /*clear_existing_callbacks=*/false); @@ -315,7 +315,7 @@ inline FutureBase::CompletionCallbackHandle FutureBase::AddOnCompletion( inline void FutureBase::RemoveOnCompletion( CompletionCallbackHandle completion_handle) const { - MutexLock lock(mutex_); + std::lock_guard lock(mutex_); if (api_ != NULL) { // NOLINT api_->RemoveCompletionCallback(handle_, completion_handle); } @@ -325,7 +325,7 @@ inline void FutureBase::RemoveOnCompletion( #if defined(FIREBASE_USE_STD_FUNCTION) inline void FutureBase::OnCompletion( std::function callback) const { - MutexLock lock(mutex_); + std::lock_guard lock(mutex_); if (api_ != NULL) { // NOLINT api_->AddCompletionCallbackLambda(handle_, callback, /*clear_existing_callbacks=*/true); @@ -335,7 +335,7 @@ inline void FutureBase::OnCompletion( #if defined(INTERNAL_EXPERIMENTAL) inline FutureBase::CompletionCallbackHandle FutureBase::AddOnCompletion( std::function callback) const { - MutexLock lock(mutex_); + std::lock_guard lock(mutex_); if (api_ != NULL) { // NOLINT return api_->AddCompletionCallbackLambda( handle_, callback, From 4aeb741d925664cc9394d7f4b76b106f862300a4 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Mon, 15 Nov 2021 15:19:40 -0500 Subject: [PATCH 7/7] release_build_files/readme.md: tweak release notes entry --- release_build_files/readme.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/release_build_files/readme.md b/release_build_files/readme.md index 09fe8a50d9..fb68eeb8aa 100644 --- a/release_build_files/readme.md +++ b/release_build_files/readme.md @@ -569,8 +569,8 @@ code. ## Release Notes ### Next Release - Changes - - All Products (Android): Fixed a data race that could manifest as null - pointer dereference in FutureBase::Release(). + - 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.