Repository navigation
Fix use-after-free when Firebase instances are destroyed during static destruction - #1949
AustinBenoit wants to merge 6 commits into
Conversation
Each product keeps a process-wide map of App -> instance plus a mutex, and both are touched from the instance's destructor via DeleteInternal(). When a user holds an instance in a static (e.g. a static std::unique_ptr<Auth>) that is destroyed after the SDK's own globals, the destructor accesses an already-destroyed map or mutex. Auth and Remote Config stored the map as a static std::map, which produced a heap-use-after-free in ~Auth()/~RemoteConfig(). Database, Storage, Functions, Installations and App Check already heap-allocated the map but still used a static Mutex, which is destroyed at exit before the lock in DeleteInternal(). Bring every product in line with the layout App and Firestore already use: the map is lazily allocated and freed when the last instance goes away, and the mutex is allocated once and never freed. Fixes #110
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses a potential crash at process exit by ensuring that global mutexes and maps for various Firebase SDK components (Auth, RemoteConfig, Database, Storage, Functions, Installations, and AppCheck) are handled safely. The changes involve dynamically allocating these objects to prevent destruction order issues during C++ runtime shutdown. The reviewer correctly identified that the current implementation of global mutex initialization is susceptible to the Static Initialization Order Fiasco (SIOF) and suggested using a function-local static pointer ('construct on first use' idiom) to mitigate this risk.
Replace the namespace-scope `Mutex* g_x = new Mutex();` globals with accessor functions that own a function-local static. Dynamic initialization of a namespace-scope pointer is subject to static initialization order, so a static initializer in another translation unit that called GetInstance() would have found a null mutex. A function-local static is initialized on first use, thread-safely, regardless of translation unit order, and the pointer it holds is still never freed. Auth and Remote Config now also keep their map behind the same kind of accessor instead of lazily allocating and freeing a raw pointer, which removes the null checks and manual lifetime management from the GetInstance/Find/DeleteInternal paths. Callers only ever see references.
Introduce firebase::NoDestructor<T>, a small wrapper (modelled on absl::NoDestructor) that constructs a T in place and never runs its destructor. Use it for the registry mutexes of every product and the registry maps of Auth and Remote Config in place of `static T* const t = new T;`. The objects are now stored inline in the function-local static rather than on the heap, there is no `new` or raw pointer anywhere in the registry code, and leak checkers have nothing to report since nothing is allocated. The semantics are otherwise unchanged: constructed on first use, thread-safely, regardless of translation unit order, and never destroyed. Includes a unit test for NoDestructor.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a NoDestructor utility class to wrap function-local statics, such as global maps and mutexes, ensuring they are constructed on first use and never destroyed. This change prevents crashes at process exit when Firebase SDK instances (such as Auth, RemoteConfig, Database, Storage, Functions, Installations, or AppCheck) are owned by static objects and destroyed after the SDK's globals. The review feedback highlights several critical issues regarding redundant recursive locking or potential deadlocks in Auth, RemoteConfig, and Installations where locked functions call other functions that attempt to acquire the same mutex. Additionally, minor improvements are suggested for NoDestructor to use decayed array pointers instead of pointer-to-array types for cleaner and more portable casts.
- In NoDestructor, cast the decayed array pointer `storage_` rather than `&storage_` in placement new and `get()`. - In Auth, RemoteConfig, and Installations, factor out `Find*Locked(App*)` helpers (and inline the internal cleanup in `~Installations()`) so callers that already hold the registry mutex do not recursively re-lock it.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the NoDestructor template class to wrap function-local statics, preventing process-exit crashes when Firebase SDK instances (such as Auth, RemoteConfig, Database, Storage, Functions, Installations, and AppCheck) are owned by static objects and destroyed after the SDK's globals. The feedback suggests optimizing the Auth map deletion by performing a direct key lookup instead of iterating through the map, and refactoring the Installations cleanup logic into a helper function to eliminate code duplication.
| AuthMap& auths = Auths(); | ||
| for (auto it = auths.begin(); it != auths.end(); ++it) { | ||
| if (it->second == this) { | ||
| LogDebug("Deleting Auth %p for App %p", this, it->first); | ||
| g_auths.erase(it); | ||
| auths.erase(it); | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Instead of iterating through the entire Auths map to find the entry for this instance, you can perform a direct lookup using auth_data_->app as the key. This is more efficient (RemoteConfig) remove themselves from their respective registry maps.
AuthMap& auths = Auths();\n auto it = auths.find(auth_data_->app);\n if (it != auths.end() && it->second == this) {\n LogDebug(\"Deleting Auth %p for App %p\", this, it->first);\n auths.erase(it);\n }There was a problem hiding this comment.
Done in f0bad88 — replaced the linear scan in Auth::DeleteInternal() with a direct auths.find(auth_data_->app) key lookup (verifying it->second == this before erasing).
| static Mutex& InstallationsMutex() { | ||
| static NoDestructor<Mutex> mutex; | ||
| return *mutex; | ||
| } | ||
|
|
||
| using internal::InstallationsInternal; |
There was a problem hiding this comment.
To avoid duplicating the cleanup logic for installations_internal_ in both the destructor and DeleteInternal(), we can define a static helper function CleanupInternal in this file.
static Mutex& InstallationsMutex() {\n static NoDestructor<Mutex> mutex;\n return *mutex;\n}\n\nusing internal::InstallationsInternal;\n\nstatic void CleanupInternal(InstallationsInternal*& installations_internal) {\n if (installations_internal) {\n installations_internal->Cleanup();\n delete installations_internal;\n installations_internal = nullptr;\n }\n}There was a problem hiding this comment.
Done in f0bad88 — deduplicated teardown by having Installations::~Installations() delegate directly to DeleteInternal(), which also erases app_ from InstallationsByApp() on both App cleanup and ~Installations().
| if (installations_internal_) { | ||
| installations_internal_->Cleanup(); | ||
| delete installations_internal_; | ||
| installations_internal_ = nullptr; | ||
| } |
There was a problem hiding this comment.
Use the new CleanupInternal helper function to clean up installations_internal_ without duplicating the code.
CleanupInternal(installations_internal_);There was a problem hiding this comment.
Done in f0bad88 — ~Installations() now delegates directly to DeleteInternal() instead of duplicating the installations_internal_ teardown.
…odules - Harden NoDestructor<T> with SFINAE forwarding-ctor guard, brace-init overloads, and std::launder access - Migrate firebase_app, app/rest, and all product libraries (Analytics, AppCheck, Auth, Database, Firestore, Functions, Installations, Messaging, RemoteConfig, Storage, UMP) across Desktop, Android, and iOS onto function-local NoDestructor accessors - Fix Installations out-of-order App deletion map retention, ABA safety, failed-init cleanup in Installations/RemoteConfig, and O(log N) Auth map removal - Enable -Werror=exit-time-destructors on Clang/AppleClang across all 12 static library targets - Add firebase_app_static_destruction_test, expanded NoDestructor unit tests, and Installations out-of-order teardown regression test
Description
Fixes #110.
Eliminates C++ static initialization order fiasco (SIOF) and exit-time static destruction order hazards across
firebase_app(App,CleanupNotifier,AppCallback,callback,log,ReferenceCountedFutureImpl,UserSecureManager,app/rest) and all C++ SDK product libraries (Analytics,AppCheck,Auth,Database,Firestore,Functions,Installations,Messaging,RemoteConfig,Storage, andUMP).Root Cause
If an application holds
firebase::Appor any Firebase product instance in an object with static storage duration (for example, a namespace-scopestatic std::unique_ptr<Auth>), the C++ runtime destroys static objects in reverse order of completion of their construction (__cxa_atexit/atexit). Whenever any SDK-internal registry map, mutex, logger,CleanupNotifierowner table, or constant object was constructed after the user's static wrapper, its destructor ran before the user's destructor—causing heap-use-after-free or destroyed-mutex UB duringexit()(as reported in #110). Conversely, namespace-scopenew Mutex()pointers avoided exit-time destruction at the cost of cross-TU dynamic initialization hazards beforemain().Solution
firebase::NoDestructor<T>(app/src/no_destructor.h):Tinline inalignas(T)storage via placementnew, never invokes~T(), and is trivially destructible (std::is_trivially_destructible<NoDestructor<T>>::value == true), so no__cxa_atexit/atexithandler is registered and static storage remains reachable for leak checkers.NoDestructorarguments cannot hijack types with broad converting constructors (such asfirebase::Variant), supports brace-enclosed initializers, and usesstd::launderon storage access.firebase_app&app/restCore Infrastructure:CleanupNotifierowner maps, cURL/REST globals, andInvalidHandle()sentinels to function-localstatic NoDestructor<T>accessors—initialized thread-safely on first call (C++11[stmt.dcl]) and alive through process termination.main()AppCallback::AddCallbackregistration duringFIREBASE_APP_REGISTER_CALLBACKSwhile synchronizing runtimeApplifecycle notifications.Analytics,AppCheck,Auth,Database,Firestore,Functions,Installations,Messaging,RemoteConfig,Storage,UMP):Appinstance registries, mutexes, JNI class-init locks/initializers, iOS delegate/instance locks, and desktop internal static constants (Variant/QuerySpec/OperationSource/PruneForest) ontoNoDestructor<T>accessors across Desktop, Android, and iOS.Installations&RemoteConfiglifecycle fixes: FixesInstallationszombie-pointer retention whenAppis destroyed beforeInstallations(erasing fromInstallationsByApp()inDeleteInternal()and delegating~Installations()toDeleteInternal()without recursive locking or ABA eviction), null-guards guttedInstallationsshells, and cleans up failedGetInstance()allocations inInstallationsandRemoteConfig.Authteardown lookup: Uses directO(log N)key lookup (auths.find(auth_data_->app)) inAuth::DeleteInternal().-Werror=exit-time-destructorsforClangandAppleClangacrossfirebase_app,firebase_rest_lib, and all 11 product CMake targets so accidental introduction of non-trivially destructible statics fails at compile time on macOS/iOS/Android Clang builds.firebase_app_static_destruction_test(exercising real post-main()__cxa_atexitteardown in both normal and reverseApp/subscriber destruction orders plus multi-cycleAppreinitialization), expandsfirebase_app_no_destructor_test, and addsInstallationsTest.DestroyAppBeforeInstallationsClearsCachedInstanceso GCC (Linux) and MSVC (Windows) CI also enforce exit-time and lifecycle invariants at runtime.Testing
__cxa_atexitaudit (Linux desktop build): Builtfirebase_app,firebase_rest_lib,firebase_analytics,firebase_app_check,firebase_auth,firebase_database,firebase_functions,firebase_installations,firebase_messaging,firebase_remote_config,firebase_storage, andfirebase_umpand verified vianm -uthat 0 object files across all 12 static archives reference__cxa_atexit.-Werror=exit-time-destructorsverification: Verified modified C++ translation units (includingfirestore/src/common/) compile cleanly withclang++ -Werror=exit-time-destructors.firebase_app_no_destructor_test(type traits,VariantSFINAE guard, brace init, over-aligned storage, non-destruction)firebase_app_static_destruction_test(real post-main()static destructor teardown & multi-cycleAppreinitialization)firebase_installations_test(including out-of-orderApp-before-Installationsdeletion andApprecreation)firebase_app_cleanup_notifier_test,firebase_app_callback_test,firebase_app_log_test,firebase_app_desktop_test, andfirebase_rtdb_desktop_*unit testsstatic std::unique_ptrinstances destroyed aftermain()in bothApp-first and product-first construction orders exit cleanly (exit 0, zero ASan findings) vs. theheap-use-after-freein__run_exit_handlersonmain.Type of Change
Place an
xthe applicable box:#110
Notes
Release Notessection ofrelease_build_files/readme.md.