Fix segfault caused by destruction order of Event and Connection

Signed-off-by: Addisu Z. Taddese <[email protected]>
gazebosim · Jul 27, 2021 · d4f7867 · d4f7867
1 parent c6bddd8
commit d4f7867
Show file tree

Hide file tree

Showing 2 changed files with 45 additions and 4 deletions.
diff --git a/events/include/ignition/common/Event.hh b/events/include/ignition/common/Event.hh
@@ -156,8 +156,9 @@ namespace ignition
       private: class EventConnection
       {
         /// \brief Constructor
-        public: EventConnection(const bool _on, const std::function<T> &_cb)
-                : callback(_cb)
+        public: EventConnection(const bool _on, const std::function<T> &_cb,
+                        const ConnectionPtr &_publicConn)
+            : callback(_cb), publicConnection(_publicConn)
         {
           // Windows Visual Studio 2012 does not have atomic_bool constructor,
           // so we have to set "on" using operator=
@@ -169,6 +170,11 @@ namespace ignition
 
         /// \brief Callback function
         public: std::function<T> callback;
+
+        /// \brief A weak pointer to the Connection pointer returned by Connect.
+        /// This is used to clear the Connection's Event pointer during
+        /// destruction of an Event.
+        public: std::weak_ptr<Connection> publicConnection;
       };
 
       /// \def EvtConnectionMap
@@ -197,6 +203,16 @@ namespace ignition
     template<typename T, typename N>
     EventT<T, N>::~EventT()
     {
+      // Clear the Event pointer on all connections so that they are not
+      // accessed after this Event is destructed.
+      for (auto &conn : this->connections)
+      {
+        auto publicCon = conn.second->publicConnection.lock();
+        if (publicCon)
+        {
+          publicCon->event = nullptr;
+        }
+      }
       this->connections.clear();
     }
 
@@ -211,8 +227,10 @@ namespace ignition
         auto const &iter = this->connections.rbegin();
         index = iter->first + 1;
       }
-      this->connections[index].reset(new EventConnection(true, _subscriber));
-      return ConnectionPtr(new Connection(this, index));
+      auto connection = ConnectionPtr(new Connection(this, index));
+      this->connections[index].reset(
+          new EventConnection(true, _subscriber, connection));
+      return connection;
     }
 
     /// \brief Get the number of connections.
@@ -234,6 +252,14 @@ namespace ignition
       if (it != this->connections.end())
       {
         it->second->on = false;
+        // The destructor of std::function seems to crashes if the function it
+        // points to is in a shared library and has been unloaded by the time
+        // the destructor is invoked. It's not clear whether this is a bug in
+        // the implementation of std::function or not. To avoid the crash,
+        // we call the destructor here by setting `callback = nullptr` because
+        // it is likely that EventT::Disconnect is called before the shared
+        // library is unloaded via Connection::~Connection.
+        it->second->callback = nullptr;
         this->connectionsToRemove.push_back(it);
       }
     }

diff --git a/events/src/Event_TEST.cc b/events/src/Event_TEST.cc
@@ -283,6 +283,21 @@ TEST_F(EventTest, typeid_test)
   EXPECT_NE(typeid(Event3).name(), typeid(Event2).name());
 }
 
+/////////////////////////////////////////////////
+TEST_F(EventTest, DestructionOrder)
+{
+  auto evt = std::make_unique<common::EventT<void()>>();
+  common::ConnectionPtr conn = evt->Connect(callback);
+  evt->Signal();
+  evt.reset();
+  // Sleep to avoid warning about deleting a connection right after creation.
+  IGN_SLEEP_MS(1);
+
+  // Check that this doesn't segfault.
+  conn.reset();
+  SUCCEED();
+}
+
 /////////////////////////////////////////////////
 int main(int argc, char **argv)
 {