zaphoyd / websocketpp

C++ websocket client/server library
http://www.zaphoyd.com/websocketpp
Other
6.88k stars 1.95k forks source link

Issue with thread safety of endpoint::stop_listening() #1028

Open karlisolte opened 2 years ago

karlisolte commented 2 years ago

Library claims to be thread safe, so I would expect endpoint to be thread safe, but stop_listening() methods modifies m_state while is_listening() reads m_state (m_state is just an enum).

This was pointed out by thread sanitizer.

Radrik5 commented 6 months ago

Moreover, stop_listening() calls m_acceptor->close(); which is not thread-safe per boost::asio documentation:

synchronous operations, such as open or close, are not thread safe.

And it does cause crashes if another thread executes async_accept() from handle_accept() at the same time:

boost::asio::detail::epoll_reactor::start_op epoll_reactor.ipp:245
boost::asio::detail::reactive_socket_service_base::start_op reactive_socket_service_base.ipp:245
boost::asio::detail::reactive_socket_service_base::start_op reactive_socket_service_base.ipp:234
boost::asio::detail::reactive_socket_service_base::start_accept_op reactive_socket_service_base.ipp:259
boost::asio::detail::reactive_socket_service::async_accept<…>(boost::asio::detail::reactive_socket_service<…>::implementation_type &, boost::asio::basic_socket<…> &, boost::asio::ip::basic_endpoint<…> *, boost::asio::detail::wrapped_handler<…> &, const boost::asio::detail::io_object_executor<…> &) reactive_socket_service.hpp:435
boost::asio::basic_socket_acceptor::initiate_async_accept::operator()<…>(boost::asio::detail::wrapped_handler<…> &&, boost::asio::basic_socket_acceptor<…> *, boost::asio::basic_socket<…> *, boost::asio::ip::basic_endpoint<…> *) const basic_socket_acceptor.hpp:2338
boost::asio::async_result::initiate<…>(boost::asio::basic_socket_acceptor<…>::initiate_async_accept &&, boost::asio::detail::wrapped_handler<…> &&, boost::asio::basic_socket_acceptor<…> *&&, boost::asio::basic_socket<…> *&&, boost::asio::ip::basic_endpoint<…> *&&) async_result.hpp:82
boost::asio::async_initiate<…>(boost::asio::basic_socket_acceptor<…>::initiate_async_accept &&, boost::asio::detail::wrapped_handler<…> &, boost::asio::basic_socket_acceptor<…> *&&, boost::asio::basic_socket<…> *&&, boost::asio::ip::basic_endpoint<…> *&&) async_result.hpp:257
boost::asio::basic_socket_acceptor::async_accept<…>(boost::asio::basic_socket<…> &, boost::asio::detail::wrapped_handler<…> &&, std::enable_if<…>::type *) basic_socket_acceptor.hpp:1336
websocketpp::transport::asio::endpoint::async_accept(boost::shared_ptr<…>, std::function<…>, std::error_code &) endpoint.hpp:790
websocketpp::server::start_accept server_endpoint.hpp:135
websocketpp::server::handle_accept server_endpoint.hpp:182
std::__invoke_impl<…> invoke.h:73

The crash in epoll_reactor::start_op() is caused by null pointer dereference of descriptor_data pointer:

void epoll_reactor::start_op(int op_type, socket_type descriptor,
    epoll_reactor::per_descriptor_data& descriptor_data, reactor_op* op,
    bool is_continuation, bool allow_speculative)
{
  if (!descriptor_data)
  {
    op->ec_ = boost::asio::error::bad_descriptor;
    post_immediate_completion(op, is_continuation);
    return;
  }

  mutex::scoped_lock descriptor_lock(descriptor_data->mutex_);

  if (descriptor_data->shutdown_)  // descriptor_data is nullptr here but wasn't nullptr before the mutex lock above
  {
    post_immediate_completion(op, is_continuation);
    return;
  }