Skip to content

Fix segfault when destroying Event objects #367

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from
Closed

Fix segfault when destroying Event objects #367

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ee66d3b
Fix segfault when destroying Event objects
azeey Jun 1, 2022
5a6d88d
Suppress windows warnings
azeey Jun 7, 2022
0c03f6b
Add test that crashes on main but passes with this PR.
azeey Jun 13, 2022
3a38196
Merge remote-tracking branch 'upstream/main' into azeey/crash_on_dest…
azeey Jun 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
207 changes: 78 additions & 129 deletions events/include/gz/common/Event.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#ifndef GZ_COMMON_EVENT_HH_
#define GZ_COMMON_EVENT_HH_

#include <any>
#include <atomic>
#include <functional>
#include <list>
Expand All @@ -28,6 +29,7 @@
#include <gz/common/config.hh>
#include <gz/common/events/Export.hh>
#include <gz/common/events/Types.hh>
#include <gz/utils/SuppressWarning.hh>

namespace gz
{
Expand All @@ -41,11 +43,21 @@ namespace gz
public: Event();

/// \brief Destructor
public: virtual ~Event();
// WARNING: This should never be changed to virtual. See the class
// documentation for EventT. Otherwise, segfaults may occur in downstream
// libraries.
public: ~Event();

/// \brief Disconnect
/// \param[in] _id Integer ID of a connection
public: virtual void Disconnect(int _id) = 0;
/// \brief Disconnect a callback to this event.
/// \param[in] _id The id of the connection to disconnect.
public: virtual void Disconnect(int _id);

/// \brief Get the number of connections.
/// \return Number of connections.
unsigned int ConnectionCount() const
{
return this->connections.size();
}

/// \brief Get whether this event has been signaled.
/// \return True if the event has been signaled.
Expand All @@ -55,8 +67,54 @@ namespace gz
/// \param[in] _sig True if the event has been signaled.
public: void SetSignaled(bool _sig);

/// \internal
/// \brief Removes queued connections.
/// We assume that this function is called from a Signal function.
protected: void Cleanup();

/// \brief True if the event has been signaled.
private: bool signaled;

/// \brief A private helper class used in maintaining connections.
protected: class EventConnection
{
/// \brief Constructor
public: EventConnection(bool _on, const std::any &_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=
this->on = _on;
}

/// \brief On/off value for the event callback
public: std::atomic_bool on;

/// \brief Callback function
public: std::any 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;
};
IGN_UTILS_WARN_IGNORE__DLL_INTERFACE_MISSING
/// \def EvtConnectionMap
/// \brief Event Connection map typedef.
protected: typedef std::map<int, std::unique_ptr<EventConnection>>
EvtConnectionMap;

/// \brief Array of connection callbacks.
protected: EvtConnectionMap connections;

/// \brief A thread lock.
private: std::mutex mutex;

/// \brief List of connections to remove
private: std::list<typename EvtConnectionMap::const_iterator>
connectionsToRemove;
IGN_UTILS_WARN_RESUME__DLL_INTERFACE_MISSING
};

/// \brief A class that encapsulates a connection.
Expand Down Expand Up @@ -93,39 +151,37 @@ namespace gz

/// \brief Friend class.
public: template<typename T, typename N> friend class EventT;
public: friend class Event;
};

/// \brief A class for event processing.
/// \tparam T function event callback function signature
/// \tparam N optional additional type to disambiguate events with same
/// function signature
//
// WARNING: This class should not have any data members. Instead all data
// members should go into the base class `Event`. This is to ensure that
// this class behaves as expected when used with shared libraries (plugins)
// that get unloaded after emitting an event.
//
// Explanation: TODO
//
template<typename T, typename N = void>
class EventT : public Event
class EventT final : public Event
{
public: using CallbackT = std::function<T>;
static_assert(std::is_same<typename CallbackT::result_type, void>::value,
"Event callback must have void return type");

/// \brief Constructor.
public: EventT();

/// \brief Destructor.
public: virtual ~EventT();
public: EventT() = default;

/// \brief Connect a callback to this event.
/// \param[in] _subscriber Pointer to a callback function.
/// \return A Connection object, which will automatically call
/// Disconnect when it goes out of scope.
public: ConnectionPtr Connect(const CallbackT &_subscriber);

/// \brief Disconnect a callback to this event.
/// \param[in] _id The id of the connection to disconnect.
public: virtual void Disconnect(int _id);

/// \brief Get the number of connections.
/// \return Number of connection to this Event.
public: unsigned int ConnectionCount() const;

/// \brief Access the signal.
public: template<typename ... Args>
void operator()(Args && ... args)
Expand All @@ -143,83 +199,18 @@ namespace gz
for (const auto &iter : this->connections)
{
if (iter.second->on)
iter.second->callback(std::forward<Args>(args)...);
{
auto callback = std::any_cast<CallbackT>(iter.second->callback);
callback(std::forward<Args>(args)...);
}
}
}

/// \internal
/// \brief Removes queued connections.
/// We assume that this function is called from a Signal function.
private: void Cleanup();

/// \brief A private helper class used in maintaining connections.
private: class EventConnection
{
/// \brief Constructor
public: EventConnection(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=
this->on = _on;
}

/// \brief On/off value for the event callback
public: std::atomic_bool on;

/// \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
/// \brief Event Connection map typedef.
typedef std::map<int, std::unique_ptr<EventConnection>> EvtConnectionMap;

/// \brief Array of connection callbacks.
private: EvtConnectionMap connections;

/// \brief A thread lock.
private: std::mutex mutex;

/// \brief List of connections to remove
private: std::list<typename EvtConnectionMap::const_iterator>
connectionsToRemove;
};

/// \brief Constructor.
template<typename T, typename N>
EventT<T, N>::EventT()
: Event()
{
}

/// \brief Destructor. Deletes all the associated connections.
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();
}

/// \brief Adds a connection.
/// \param[in] _subscriber the subscriber to connect.
template<typename T, typename N>
ConnectionPtr EventT<T, N>::Connect(const std::function<T> &_subscriber)
ConnectionPtr EventT<T, N>::Connect(const CallbackT &_subscriber)
{
int index = 0;
if (!this->connections.empty())
Expand All @@ -232,48 +223,6 @@ namespace gz
new EventConnection(true, _subscriber, connection));
return connection;
}

/// \brief Get the number of connections.
/// \return Number of connections.
template<typename T, typename N>
unsigned int EventT<T, N>::ConnectionCount() const
{
return this->connections.size();
}

/// \brief Removes a connection.
/// \param[in] _id the connection index.
template<typename T, typename N>
void EventT<T, N>::Disconnect(int _id)
{
// Find the connection
auto const &it = this->connections.find(_id);

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);
}
}

/////////////////////////////////////////////
template<typename T, typename N>
void EventT<T, N>::Cleanup()
{
std::lock_guard<std::mutex> lock(this->mutex);
// Remove all queue connections.
for (auto &conn : this->connectionsToRemove)
this->connections.erase(conn);
this->connectionsToRemove.clear();
}
}
}
#endif
41 changes: 41 additions & 0 deletions events/src/Event.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,17 @@ Event::Event()
//////////////////////////////////////////////////
Event::~Event()
{
// 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();
}

//////////////////////////////////////////////////
Expand All @@ -44,6 +55,35 @@ void Event::SetSignaled(bool _sig)
this->signaled = _sig;
}

//////////////////////////////////////////////////
void Event::Disconnect(int _id) {
// Find the connection
auto const &it = this->connections.find(_id);

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);
}
}

/////////////////////////////////////////////
void Event::Cleanup()
{
std::lock_guard<std::mutex> lock(this->mutex);
// Remove all queue connections.
for (auto &conn : this->connectionsToRemove)
this->connections.erase(conn);
this->connectionsToRemove.clear();
}

//////////////////////////////////////////////////
Connection::Connection(Event *_e, int _i)
: event(_e), id(_i)
Expand Down Expand Up @@ -75,3 +115,4 @@ int Connection::Id() const
{
return this->id;
}

11 changes: 11 additions & 0 deletions test/integration/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,10 @@ if (SKIP_av OR INTERNAL_SKIP_av)
list(REMOVE_ITEM tests video_encoder.cc)
endif()

if (WIN32)
list(REMOVE_ITEM tests events_with_plugins.cc)
endif()

ign_build_tests(
TYPE INTEGRATION
SOURCES ${tests}
Expand All @@ -29,3 +33,10 @@ endif()
if(TARGET INTEGRATION_video_encoder)
target_link_libraries(INTEGRATION_video_encoder ${PROJECT_LIBRARY_TARGET_NAME}-av)
endif()

if(TARGET INTEGRATION_events_with_plugins)
target_link_libraries(INTEGRATION_events_with_plugins ${PROJECT_LIBRARY_TARGET_NAME}-events dl)
add_dependencies(INTEGRATION_events_with_plugins EventEmitterPlugin)
target_compile_definitions(INTEGRATION_events_with_plugins PRIVATE
"EventEmitterPlugin_LIB=\"$<TARGET_FILE:EventEmitterPlugin>\"")
endif()
Loading