fix use-after-free when client disconnects intentionally

src/ProxyServer.cc

@@ -44,6 +44,7 @@ ProxyServer::ProxyServer(
     shared_ptr<struct event_base> base,
     shared_ptr<ServerState> state)
   : base(base),
+    destroy_sessions_ev(event_new(this->base.get(), -1, EV_TIMEOUT, &ProxyServer::dispatch_destroy_sessions, this), event_free),
     state(state),
     next_unlicensed_session_id(0xFF00000000000001) { }
 
@@ -356,6 +357,9 @@ void ProxyServer::UnlinkedSession::on_input(Channel& ch, uint16_t command, uint3
     should_close_unlinked_session = true;
   }
 
+  // Note that ch.bev will be moved from when the linked session is resumed, so
+  // we need to retain a copy of it in order to close the unlinked session
+  // afterward.
   struct bufferevent* session_key = ch.bev.get();
 
   // If license is non-null, then the client has a password and can be connected
@@ -425,9 +429,7 @@ void ProxyServer::UnlinkedSession::on_input(Channel& ch, uint16_t command, uint3
   }
 
   if (should_close_unlinked_session) {
-    session->log.info("Closing session");
+    session->server->delete_session(session_key);
-    session->server->bev_to_unlinked_session.erase(session_key);
-    // At this point, (*this) is destroyed! We must be careful not to touch it.
   }
 }
 
@@ -440,8 +442,8 @@ void ProxyServer::UnlinkedSession::on_error(Channel& ch, short events) {
         evutil_socket_error_to_string(err));
   }
   if (events & (BEV_EVENT_ERROR | BEV_EVENT_EOF)) {
-    session->log.info("Unlinked client has disconnected");
+    session->log.info("Client has disconnected");
-    session->server->bev_to_unlinked_session.erase(session->channel.bev.get());
+    session->server->delete_session(session->channel.bev.get());
   }
 }
 
@@ -823,6 +825,27 @@ void ProxyServer::delete_session(uint64_t id) {
   }
 }
 
+void ProxyServer::delete_session(struct bufferevent* bev) {
+  auto it = this->bev_to_unlinked_session.find(bev);
+  if (it == this->bev_to_unlinked_session.end()) {
+    throw logic_error("unlinked session exists but is not registered");
+  }
+  it->second->log.info("Closing session");
+  this->unlinked_sessions_to_destroy.emplace(move(it->second));
+  this->bev_to_unlinked_session.erase(it);
+
+  auto tv = usecs_to_timeval(0);
+  event_add(this->destroy_sessions_ev.get(), &tv);
+}
+
+void ProxyServer::dispatch_destroy_sessions(evutil_socket_t, short, void* ctx) {
+  reinterpret_cast<ProxyServer*>(ctx)->destroy_sessions();
+}
+
+void ProxyServer::destroy_sessions() {
+  this->unlinked_sessions_to_destroy.clear();
+}
+
 size_t ProxyServer::delete_disconnected_sessions() {
   size_t count = 0;
   for (auto it = this->id_to_session.begin(); it != this->id_to_session.end();) {

src/ProxyServer.hh

@@ -168,6 +168,7 @@ public:
     GameVersion version,
     const ClientConfigBB& newserv_client_config);
   void delete_session(uint64_t id);
+  void delete_session(struct bufferevent* bev);
 
   size_t delete_disconnected_sessions();
 
@@ -215,12 +216,17 @@ private:
   };
 
   std::shared_ptr<struct event_base> base;
+  std::shared_ptr<struct event> destroy_sessions_ev;
   std::shared_ptr<ServerState> state;
   std::map<int, std::shared_ptr<ListeningSocket>> listeners;
   std::unordered_map<struct bufferevent*, std::shared_ptr<UnlinkedSession>> bev_to_unlinked_session;
+  std::unordered_set<std::shared_ptr<UnlinkedSession>> unlinked_sessions_to_destroy;
   std::unordered_map<uint64_t, std::shared_ptr<LinkedSession>> id_to_session;
   uint64_t next_unlicensed_session_id;
 
+  static void dispatch_destroy_sessions(evutil_socket_t, short, void* ctx);
+  void destroy_sessions();
+
   void on_client_connect(
       struct bufferevent* bev,
       uint16_t listen_port,

src/Server.cc

@@ -46,8 +46,25 @@ void Server::disconnect_client(shared_ptr<Client> c) {
   } catch (const exception& e) {
     server_log.warning("Error during client disconnect cleanup: %s", e.what());
   }
-  // c is destroyed here (on_disconnect should remove any other references
+
-  // to it, e.g. from Lobby objects)
+  // We can't just let c be destroyed here, since disconnect_client can be
+  // called from within the client's channel's receive handler. So, we instead
+  // move it to another set, which we'll clear in an immediately-enqueued
+  // callback after the current event.
+  this->clients_to_destroy.insert(move(c));
+}
+
+void Server::enqueue_destroy_clients() {
+  auto tv = usecs_to_timeval(0);
+  event_add(this->destroy_clients_ev.get(), &tv);
+}
+
+void Server::dispatch_destroy_clients(evutil_socket_t, short, void* ctx) {
+  reinterpret_cast<Server*>(ctx)->destroy_clients();
+}
+
+void Server::destroy_clients() {
+  this->clients_to_destroy.clear();
 }
 
 void Server::dispatch_on_listen_accept(
@@ -177,7 +194,9 @@ void Server::on_client_error(Channel& ch, short events) {
 Server::Server(
     shared_ptr<struct event_base> base,
     shared_ptr<ServerState> state)
-  : base(base), state(state) { }
+  : base(base),
+    destroy_clients_ev(event_new(this->base.get(), -1, EV_TIMEOUT, &Server::dispatch_destroy_clients, this), event_free),
+    state(state) { }
 
 void Server::listen(
     const std::string& addr_str,

src/Server.hh

@@ -35,6 +35,7 @@ public:
 
 private:
   std::shared_ptr<struct event_base> base;
+  std::shared_ptr<struct event> destroy_clients_ev;
 
   struct ListeningSocket {
     std::string addr_str;
@@ -52,9 +53,14 @@ private:
   };
   std::unordered_map<int, ListeningSocket> listening_sockets;
   std::unordered_map<Channel*, std::shared_ptr<Client>> channel_to_client;
+  std::unordered_set<std::shared_ptr<Client>> clients_to_destroy;
 
   std::shared_ptr<ServerState> state;
 
+  void enqueue_destroy_clients();
+  static void dispatch_destroy_clients(evutil_socket_t, short, void* ctx);
+  void destroy_clients();
+
   static void dispatch_on_listen_accept(struct evconnlistener* listener,
       evutil_socket_t fd, struct sockaddr *address, int socklen, void* ctx);
   static void dispatch_on_listen_error(struct evconnlistener* listener, void* ctx);