Commit c84f5347 authored by Yedidya Feldblum's avatar Yedidya Feldblum Committed by Facebook Github Bot

Fix double-delete in Singleton::make_mock

Summary:
[Folly] Fix double-`delete` in `Singleton::make_mock`.

In that function, we unconditionally destroy any existing singleton instance of the same singleton. Actually, we must conditionally destroy it - conditionally on one having been created and not yet destroyed.

This problem only rarely appears because it is masked by `delete static_cast<T*>(nullptr)` being a no-op. For example, if we call `make_mock` before actually instantiating the singleton, we call that no-op. The way to make it appear is to instantiate the singleton, and then to call `make_mock` twice in a row. The first call to `make_mock` unconditionally destroys the existing instance (it still should have checked) and the second call does it again, but because the existing instance is not `nullptr`, the second call is a double-delete of a non-`nullptr` instance and crashes. In the simple case, as reproduced in an attached test, the failure is observable with ASAN. In other cases, the double-`free` is tolerated, but the failure may only be observable depending on the singleton object's state - if running the dtor twice fails.

Reviewed By: ivmaykov

Differential Revision: D4798163

fbshipit-source-id: e7b65d030d89225dfdc2fad4c778d3653460806e
parent 382b70ad
...@@ -69,7 +69,9 @@ void SingletonHolder<T>::registerSingletonMock(CreateFunc c, TeardownFunc t) { ...@@ -69,7 +69,9 @@ void SingletonHolder<T>::registerSingletonMock(CreateFunc c, TeardownFunc t) {
LOG(FATAL) << "Registering mock before singleton was registered: " LOG(FATAL) << "Registering mock before singleton was registered: "
<< type().name(); << type().name();
} }
destroyInstance(); if (state_ == SingletonHolderState::Living) {
destroyInstance();
}
{ {
auto creationOrder = vault_.creationOrder_.wlock(); auto creationOrder = vault_.creationOrder_.wlock();
......
...@@ -717,3 +717,51 @@ TEST(Singleton, MainThreadDestructor) { ...@@ -717,3 +717,51 @@ TEST(Singleton, MainThreadDestructor) {
t.join(); t.join();
} }
TEST(Singleton, DoubleMakeMockAfterTryGet) {
// to keep track of calls to ctor and dtor below
struct Counts {
size_t ctor = 0;
size_t dtor = 0;
};
// a test type which keeps track of its ctor and dtor calls
struct VaultTag {};
struct PrivateTag {};
struct Object {
explicit Object(Counts& counts) : counts_(counts) {
++counts_.ctor;
}
~Object() {
++counts_.dtor;
}
Counts& counts_;
};
using SingletonObject = Singleton<Object, PrivateTag, VaultTag>;
// register everything
Counts counts;
auto& vault = *SingletonVault::singleton<VaultTag>();
auto new_object = [&] { return new Object(counts); };
SingletonObject object_(new_object);
vault.registrationComplete();
// no eager inits, nada (sanity)
EXPECT_EQ(0, counts.ctor);
EXPECT_EQ(0, counts.dtor);
// explicit request, ctor
SingletonObject::try_get();
EXPECT_EQ(1, counts.ctor);
EXPECT_EQ(0, counts.dtor);
// first make_mock, dtor (ctor is lazy)
SingletonObject::make_mock(new_object);
EXPECT_EQ(1, counts.ctor);
EXPECT_EQ(1, counts.dtor);
// second make_mock, nada (dtor already ran, ctor is lazy)
SingletonObject::make_mock(new_object);
EXPECT_EQ(1, counts.ctor);
EXPECT_EQ(1, counts.dtor);
}
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment