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

diff --git a/folly/Singleton-inl.h b/folly/Singleton-inl.h
index e6675165a8d8b15b1ef3d593d801d1df8a6e2b03..cacfec18c64702f4b7558230505b410d11f2759f 100644 (file)
--- a/folly/Singleton-inl.h
+++ b/folly/Singleton-inl.h
@@ -69,7 +69,9 @@ void SingletonHolder<T>::registerSingletonMock(CreateFunc c, TeardownFunc t) {
     LOG(FATAL) << "Registering mock before singleton was registered: "
                << type().name();
   }
-  destroyInstance();
+  if (state_ == SingletonHolderState::Living) {
+    destroyInstance();
+  }
 
   {
     auto creationOrder = vault_.creationOrder_.wlock();

diff --git a/folly/test/SingletonTest.cpp b/folly/test/SingletonTest.cpp
index 36aeb4e1fe2322bd887d7d3bdb78f622021226d3..cf50c6876c0d12613b08a07c429d622525075dda 100644 (file)
--- a/folly/test/SingletonTest.cpp
+++ b/folly/test/SingletonTest.cpp
@@ -717,3 +717,51 @@ TEST(Singleton, MainThreadDestructor) {
 
   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);
+}