Skip to content

Commit 0b4d39f

Browse files
cynthiajianga-maurice
authored andcommitted
[Auth] protect against auth deletion before schedule the ensure token task.
There are two bugs to fix here, both are race conditions while creating / destroying auth in very short time. The fix includes the following changes: 1. Previous Auth destruction is waiting for all async operation to complete, which would easily cause hang during destruction. In this change actually cancels all on going async operations, which live in two different schedulers. One scheduler is the auth's own scheduler, that can be cleaned up for no consequence. The other is persistence scheduler, which is globally shared among all FirebaseApp instances. 2. To clean up async ops in persistence scheduler, we restore the op handler in UserSecureManager, mapped by operation type. We make sure each operation type should only have one valid task. In destruction we can rely on active handlers to cancel all operations. 3. Even with cancelling the operation, the already running tasks will still finish, and thus are going to trigger the callbacks, which would still cause issue during destruction. To avoid this issue we introduced destructing flag for auth instance, and callbacks will abort if already destructing. PiperOrigin-RevId: 261781967
1 parent 7647936 commit 0b4d39f

File tree

9 files changed

+74
-78
lines changed

9 files changed

+74
-78
lines changed

app/src/secure/user_secure_manager.cc

Lines changed: 30 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,7 @@ UserSecureManager::UserSecureManager(
7878
}
7979

8080
UserSecureManager::~UserSecureManager() {
81+
CancelScheduledTasks();
8182
// Clear safe reference immediately so that scheduled callback can skip
8283
// executing code which requires reference to this.
8384
safe_this_.ClearReference();
@@ -115,7 +116,9 @@ Future<std::string> UserSecureManager::LoadUserData(
115116
}
116117
},
117118
safe_this_, data_handle, user_secure_.get());
118-
s_scheduler_->Schedule(callback);
119+
120+
CancelOperation(kLoadUserData);
121+
operation_handles_[kLoadUserData] = s_scheduler_->Schedule(callback);
119122
return MakeFuture(&future_api_, future_handle);
120123
}
121124

@@ -137,7 +140,9 @@ Future<void> UserSecureManager::SaveUserData(const std::string& app_name,
137140
}
138141
},
139142
safe_this_, data_handle, user_secure_.get());
140-
s_scheduler_->Schedule(callback);
143+
144+
CancelOperation(kSaveUserData);
145+
operation_handles_[kSaveUserData] = s_scheduler_->Schedule(callback);
141146
return MakeFuture(&future_api_, future_handle);
142147
}
143148

@@ -158,7 +163,9 @@ Future<void> UserSecureManager::DeleteUserData(const std::string& app_name) {
158163
}
159164
},
160165
safe_this_, data_handle, user_secure_.get());
161-
s_scheduler_->Schedule(callback);
166+
167+
CancelOperation(kDeleteUserData);
168+
operation_handles_[kDeleteUserData] = s_scheduler_->Schedule(callback);
162169
return MakeFuture(&future_api_, future_handle);
163170
}
164171

@@ -179,7 +186,9 @@ Future<void> UserSecureManager::DeleteAllData() {
179186
}
180187
},
181188
safe_this_, data_handle, user_secure_.get());
182-
s_scheduler_->Schedule(callback);
189+
190+
CancelOperation(kDeleteAllData);
191+
operation_handles_[kDeleteAllData] = s_scheduler_->Schedule(callback);
183192
return MakeFuture(&future_api_, future_handle);
184193
}
185194

@@ -278,6 +287,23 @@ void UserSecureManager::BinaryToAscii(const std::string& original,
278287
}
279288
}
280289

290+
void UserSecureManager::CancelScheduledTasks() {
291+
for (auto it = operation_handles_.begin(); it != operation_handles_.end();
292+
++it) {
293+
it->second.Cancel();
294+
}
295+
operation_handles_.clear();
296+
}
297+
298+
void UserSecureManager::CancelOperation(SecureOperationType operation_type) {
299+
// Cancel and remove existing handle.
300+
auto it = operation_handles_.find(operation_type);
301+
if (it != operation_handles_.end()) {
302+
it->second.Cancel();
303+
operation_handles_.erase(it);
304+
}
305+
}
306+
281307
} // namespace secure
282308
} // namespace app
283309
} // namespace firebase

app/src/secure/user_secure_manager.h

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,13 @@ namespace firebase {
2727
namespace app {
2828
namespace secure {
2929

30+
enum SecureOperationType {
31+
kLoadUserData,
32+
kSaveUserData,
33+
kDeleteUserData,
34+
kDeleteAllData,
35+
};
36+
3037
class UserSecureManager {
3138
public:
3239
explicit UserSecureManager(const char* domain, const char* app_id);
@@ -55,6 +62,11 @@ class UserSecureManager {
5562
static void BinaryToAscii(const std::string& original, std::string* encoded);
5663

5764
private:
65+
// Cancel already scheduled tasks in destruction.
66+
void CancelScheduledTasks();
67+
68+
void CancelOperation(SecureOperationType operation_type);
69+
5870
UniquePtr<UserSecureInternal> user_secure_;
5971
ReferenceCountedFutureImpl future_api_;
6072

@@ -66,6 +78,10 @@ class UserSecureManager {
6678
static scheduler::Scheduler* s_scheduler_;
6779
static int32_t s_scheduler_ref_count_;
6880

81+
// map from operation type to scheduled request handle. Make sure only one
82+
// request exist in scheduler for each type.
83+
std::map<SecureOperationType, scheduler::RequestHandle> operation_handles_;
84+
6985
// Safe reference to this. Set in constructor and cleared in destructor
7086
// Should be safe to be copied in any thread because the SharedPtr never
7187
// changes, until safe_this_ is completely destroyed.

auth/src/auth.cc

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -140,6 +140,11 @@ void Auth::DeleteInternal() {
140140

141141
if (!auth_data_) return;
142142

143+
{
144+
MutexLock destructing_lock(auth_data_->desctruting_mutex);
145+
auth_data_->destructing = true;
146+
}
147+
143148
CleanupNotifier* notifier = CleanupNotifier::FindByOwner(auth_data_->app);
144149
assert(notifier);
145150
notifier->UnregisterObject(this);

auth/src/data.h

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,8 @@ struct AuthData {
8383
listener_impl(nullptr),
8484
id_token_listener_impl(nullptr),
8585
expect_id_token_listener_callback(false),
86-
persistent_cache_load_pending(true) {}
86+
persistent_cache_load_pending(true),
87+
destructing(false) {}
8788

8889
~AuthData() {
8990
ClearUserInfos(this);
@@ -174,6 +175,12 @@ struct AuthData {
174175
// Mutex protecting `expect_id_token_listener_callback`
175176
Mutex expect_id_token_mutex;
176177

178+
// Tracks if auth is being destroyed.
179+
bool destructing;
180+
181+
// Mutex protecting destructing
182+
Mutex desctruting_mutex;
183+
177184
// Sets if the Id Token Listener is expecting a callback.
178185
// Used to workaround an issue where the Id Token is not reset with a new one,
179186
// and thus not triggered correctly.

auth/src/desktop/auth_desktop.cc

Lines changed: 7 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -222,7 +222,8 @@ void Auth::InitPlatformAuth(AuthData* const auth_data) {
222222

223223
void Auth::DestroyPlatformAuth(AuthData* const auth_data) {
224224
FIREBASE_ASSERT_RETURN_VOID(auth_data);
225-
WaitForAllAsyncToComplete(auth_data->auth_impl);
225+
auto auth_impl = static_cast<AuthImpl*>(auth_data->auth_impl);
226+
auth_impl->scheduler_.CancelAllAndShutdownWorkerThread();
226227
// Unregister from the function registry.
227228
auth_data->app->function_registry()->UnregisterFunction(
228229
internal::FnAuthGetCurrentToken);
@@ -235,14 +236,6 @@ void Auth::DestroyPlatformAuth(AuthData* const auth_data) {
235236

236237
DestroyTokenRefresher(auth_data);
237238

238-
{
239-
// Acquire listeners' mutex to avoid race condition if another thread is
240-
// about to notify listeners.
241-
MutexLock lock(auth_data->listeners_mutex);
242-
auth_data->listeners.clear();
243-
auth_data->id_token_listeners.clear();
244-
}
245-
246239
DestroyUserDataPersist(auth_data);
247240

248241
UserView::ClearUser(auth_data);
@@ -493,6 +486,11 @@ void DestroyUserDataPersist(AuthData* auth_data) {
493486
}
494487

495488
void LoadFinishTriggerListeners(AuthData* auth_data) {
489+
MutexLock destructing_lock(auth_data->desctruting_mutex);
490+
if (auth_data->destructing) {
491+
// If auth is destructing, abort.
492+
return;
493+
}
496494
// We would have to block other listener changes to protect race condition
497495
// on how many times a listener should be triggered. We would rely on first
498496
// listener trigger to flip the persistence loading bit.

auth/src/desktop/auth_desktop.h

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -122,7 +122,7 @@ struct AuthCompletionHandle {
122122

123123
// The desktop-specific Auth implementation.
124124
struct AuthImpl {
125-
AuthImpl() : async_sem(0), active_async_calls(0) {}
125+
AuthImpl() {}
126126

127127
// The application's API key.
128128
std::string api_key;
@@ -135,10 +135,6 @@ struct AuthImpl {
135135
IdTokenRefreshThread token_refresh_thread;
136136
// Instance responsible for user data persistence.
137137
UniquePtr<UserDataPersist> user_data_persist;
138-
// Synchronization primitives used for managing threads spawned by CallAsync.
139-
Semaphore async_sem;
140-
Mutex async_mutex;
141-
int active_async_calls;
142138

143139
// Serializes all REST call from this object.
144140
scheduler::Scheduler scheduler_;

auth/src/desktop/auth_util.cc

Lines changed: 0 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -47,35 +47,5 @@ void CompletePromise(Promise<void>* const promise,
4747
promise->Complete();
4848
}
4949

50-
// Utility functions for making sure that the CallAsync functions end before
51-
// we destruct, etc.
52-
void StartAsyncFunction(void* auth_impl_void) {
53-
auto auth_impl = static_cast<AuthImpl*>(auth_impl_void);
54-
MutexLock lock(auth_impl->async_mutex);
55-
auth_impl->active_async_calls++;
56-
}
57-
58-
void EndAsyncFunction(void* auth_impl_void) {
59-
auto auth_impl = static_cast<AuthImpl*>(auth_impl_void);
60-
{
61-
MutexLock lock(auth_impl->async_mutex);
62-
auth_impl->active_async_calls--;
63-
auth_impl->async_sem.Post();
64-
}
65-
}
66-
67-
void WaitForAllAsyncToComplete(void* auth_impl_void) {
68-
auto auth_impl = static_cast<AuthImpl*>(auth_impl_void);
69-
for (;;) {
70-
bool transfers_complete = false;
71-
{
72-
MutexLock lock(auth_impl->async_mutex);
73-
transfers_complete = auth_impl->active_async_calls == 0;
74-
}
75-
if (transfers_complete) break;
76-
auth_impl->async_sem.Wait();
77-
}
78-
}
79-
8050
} // namespace auth
8151
} // namespace firebase

auth/src/desktop/auth_util.h

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -73,12 +73,6 @@ Future<ResultT> CallAsync(
7373
std::unique_ptr<RequestT> request,
7474
const typename AuthDataHandle<ResultT, RequestT>::CallbackT callback);
7575

76-
// Utility functions for making sure that the CallAsync functions end before
77-
// we destruct, etc.
78-
void StartAsyncFunction(void* auth_impl_void);
79-
void EndAsyncFunction(void* auth_impl_void);
80-
void WaitForAllAsyncToComplete(void* auth_impl_void);
81-
8276
// Sends the given request on the network and returns the response. The response
8377
// is cast to the specified T without any checks, so it's the caller's
8478
// responsibility to ensure the correct type is given.
@@ -100,14 +94,12 @@ inline Future<ResultT> CallAsync(
10094
// doesn't need to access the request anyway.
10195
FIREBASE_ASSERT_RETURN(Future<ResultT>(), auth_data && callback);
10296

103-
StartAsyncFunction(auth_data->auth_impl);
10497
typedef AuthDataHandle<ResultT, RequestT> HandleT;
10598

10699
auto scheduler_callback = NewCallback(
107100
[](HandleT* const raw_auth_data_handle) {
108101
std::unique_ptr<HandleT> handle(raw_auth_data_handle);
109102
handle->callback(handle.get());
110-
EndAsyncFunction(handle->auth_data->auth_impl);
111103
},
112104
new HandleT(auth_data, promise, std::move(request), callback));
113105
auto auth_impl = static_cast<AuthImpl*>(auth_data->auth_impl);

auth/src/desktop/user_desktop.cc

Lines changed: 7 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -186,7 +186,6 @@ Future<ResultT> CallAsyncWithFreshToken(
186186
std::unique_ptr<RequestT> request,
187187
const typename AuthDataHandle<ResultT, RequestT>::CallbackT callback) {
188188
FIREBASE_ASSERT_RETURN(Future<ResultT>(), auth_data && request && callback);
189-
StartAsyncFunction(auth_data->auth_impl);
190189

191190
typedef AuthDataHandle<ResultT, RequestT> HandleT;
192191

@@ -198,13 +197,11 @@ Future<ResultT> CallAsyncWithFreshToken(
198197
EnsureFreshToken(handle->auth_data, false);
199198
if (!get_token_result.IsValid()) {
200199
FailPromise(&handle->promise, get_token_result.error());
201-
EndAsyncFunction(handle->auth_data->auth_impl);
202200
return;
203201
}
204202
handle->request->SetIdToken(get_token_result.token().c_str());
205203

206204
handle->callback(handle.get());
207-
EndAsyncFunction(handle->auth_data->auth_impl);
208205
},
209206
new HandleT(auth_data, promise, std::move(request), callback));
210207
auto auth_impl = static_cast<AuthImpl*>(auth_data->auth_impl);
@@ -482,38 +479,31 @@ void AssignLoadedData(const Future<std::string>& future, AuthData* auth_data) {
482479

483480
void HandleLoadedData(const Future<std::string>& future, void* auth_data) {
484481
auto cast_auth_data = static_cast<AuthData*>(auth_data);
482+
MutexLock destructing_lock(cast_auth_data->desctruting_mutex);
483+
if (cast_auth_data->destructing) {
484+
// If auth is destructing, abort.
485+
return;
486+
}
485487
AssignLoadedData(future, cast_auth_data);
486-
487-
// End async call for LoadUserData
488-
EndAsyncFunction(cast_auth_data->auth_impl);
489-
490488
auto scheduler_callback = NewCallback(
491489
[](AuthData* callback_auth_data) {
492490
// Don't trigger token listeners if token get refreshed, since
493491
// it will get triggered inside LoadFinishTriggerListeners anyways.
494492
const GetTokenResult get_token_result =
495493
EnsureFreshToken(callback_auth_data, false, false);
496494
LoadFinishTriggerListeners(callback_auth_data);
497-
EndAsyncFunction(callback_auth_data->auth_impl);
498495
},
499496
cast_auth_data);
500-
// Start Async call for EnsureFreshToken
501-
StartAsyncFunction(cast_auth_data->auth_impl);
497+
502498
auto auth_impl = static_cast<AuthImpl*>(cast_auth_data->auth_impl);
503499
auth_impl->scheduler_.Schedule(scheduler_callback);
504500
}
505501

506-
void HandlePersistenceComplete(const Future<void>& future, void* auth_data) {
507-
auto cast_auth_data = static_cast<AuthData*>(auth_data);
508-
EndAsyncFunction(cast_auth_data->auth_impl);
509-
}
510-
511502
Future<std::string> UserDataPersist::LoadUserData(AuthData* auth_data) {
512503
if (auth_data == nullptr) {
513504
return Future<std::string>();
514505
}
515506

516-
StartAsyncFunction(auth_data->auth_impl);
517507
Future<std::string> future =
518508
user_secure_manager_->LoadUserData(auth_data->app->name());
519509
future.OnCompletion(HandleLoadedData, auth_data);
@@ -565,11 +555,7 @@ Future<void> UserDataPersist::SaveUserData(AuthData* auth_data) {
565555
}
566556

567557
Future<void> UserDataPersist::DeleteUserData(AuthData* auth_data) {
568-
StartAsyncFunction(auth_data->auth_impl);
569-
Future<void> future =
570-
user_secure_manager_->DeleteUserData(auth_data->app->name());
571-
future.OnCompletion(HandlePersistenceComplete, auth_data);
572-
return future;
558+
return user_secure_manager_->DeleteUserData(auth_data->app->name());
573559
}
574560

575561
User::~User() {

0 commit comments

Comments
 (0)