Boost ASIO segfault due to socket no longer existing after a second connection is accepted

Question

I'm currently trying to get a chatroom type program working with boost::asio. In the current state, the server is able to accept connections from clients, and then the clients are able to send messages to the server (at which point, the server formats the message a little bit and then sends it to every client currently connected).

The problem I am having is as follows:

server starts
client 0 connects
client 0 sends a message
  (the message is received by the server and then sent back to client 0 who receives it correctly)
client 1 connects
client 1 sends a message
  (the message is received by the server and then sent back to client 0 and client 1 who both receive it correctly)
client 0 tries to send a message again
  (the message is received by the server and the server processes the header then attempts to call async_read again to read the body of the message, however the socket member variable for client 0 no longer exists and I get a segfault)

I find this really strange because the server still has a valid socket object for client 0 (otherwise it wouldn't be able to send client 1's messages to client 0).

Here is the relevant code:

tcp_connection class (where the segfault occurs)

#include <deque>

#include <boost/asio.hpp>
#include <boost/bind.hpp>

using boost::asio::ip::tcp;

class tcp_connection {
public:
    tcp_connection(tcp::socket socket_, int id, std::function<void (std::size_t, char*, std::size_t)> read_handler) 
      : socket_(std::move(socket)), id_(id), read_handler_(read_handler) {
    }
    
    void start() {
        char first_message[] = "server: connected";
        net_message msg(first_message, strlen(first_message));
        send(msg);
          
        read_header();
    }
    
    void send(net_message msg) {
        bool write_in_progress = !write_messages_.empty();
        write_messages_.push_back(msg);
        if (!write_in_progress) {
            do_write();
        }
    }
    
    int get_id() { return id_; }
    
private:
    void read_header() {
        boost::asio::async_read(socket_, boost::asio::buffer(read_message_.get_data(), net_message::header_length),
          boost::bind(&tcp_connection::handle_read_header, this, boost::asio::placeholders::error,
          boost::asio::placeholders::bytes_transferred));
    }
    
    void handle_read_header(const boost::system::error_code e, std::size_t bytes_transferred) {
        read_message_.decode_header();
        read_body();
    }
    
    void read_body() {
        /*
        ######################
        THIS IS WHERE THE SEGFAULT OCCURS.
        socket_ is no longer valid for some reason
        despite socket_ still being valid for any async_write 
        operations that need to be handled by the do_write() function
        ######################
        */
        boost::asio::async_read(socket_, boost::asio::buffer(read_message_.get_data() + net_message::header_length, read_message_.get_body_length()),
          boost::bind(&tcp_connection::handle_read_body, this, boost::asio::placeholders::error,
          boost::asio::placeholders::bytes_transferred));
    }
    
    void handle_read_body(const boost::system::error_code e, std::size_t bytes_transferred) {
        char body[read_message_.get_body_length()];
        memcpy(body, read_message_.get_body(), read_message_.get_body_length());
        // call the read_handler from the net_server object
        read_handler_(id_, body, read_message_.get_body_length());
        read_header();
    }
    
    void handle_write(const boost::system::error_code e, std::size_t bytes_transferred) {
    }
    
    void do_write() {
        boost::asio::async_write(socket_, boost::asio::buffer(write_messages_.front().get_data(), 
            write_messages_.front().get_body_length() + net_message::header_length),
          [this] (boost::system::error_code ec, std::size_t /*length*/) {
              if (!ec) {
                  write_messages_.pop_front();
                  if (!write_messages_.empty()) {
                      do_write();
                  }
              } else {
                  std::cerr << "error with writing to client " << id_ << " with error code: " << ec << std::endl;
              }
          });
    }
    
    tcp::socket socket_;
    std::function<void (std::size_t, char*, std::size_t)> read_handler_;
    net_message read_message_;
    std::deque<net_message> write_messages_;
    int id_;
};

net_server class

class net_server {
public:
    net_server(boost::asio::io_context& io_context, std::size_t port, 
               std::function<void (std::size_t)> accept_handler,
               std::function<void (std::size_t, char*, std::size_t)> read_handler)
      : io_context_(io_context), acceptor_(io_context, tcp::endpoint(tcp::v4(), 1234)),
        accept_handler_(accept_handler), read_handler_(read_handler)  {
            start_accept();
    }
    
    void send_to(std::size_t id, const char* body, std::size_t length) {
        net_message msg(body, length);
        connections_[id].send(msg);
    }
    
    void send_to_all(const char* body, std::size_t length) {
        net_message msg(body, length);
        for (int i = 0; i < connections_.size(); i++) {
            connections_[i].send(msg);
        }
    }
    
    void send_to_all_except(std::size_t id, const char* body, std::size_t length) {
        net_message msg(body, length);
        for (int i = 0; i < connections_.size(); i++) {
            if (i == id) continue;
            connections_[i].send(msg);
        }
    }
    
private:
    void start_accept() {
        acceptor_.async_accept(
          [this](boost::system::error_code ec, tcp::socket socket) {
            if (!ec) {
                std::unique_lock lock(connections_mutex_);
                std::size_t index = connections_.size();
                connections_.emplace_back(std::move(socket), connections_.size(), read_handler_);
                lock.unlock();

                connections_[index].start();
                accept_handler_(index);
            }
            start_accept();
          });
    }

    boost::asio::io_context& io_context_;
    tcp::acceptor acceptor_;
    
    std::vector<tcp_connection> connections_;
    std::mutex connections_mutex_;
    
    std::function<void (std::size_t)> accept_handler_;
    std::function<void (std::size_t, char*, std::size_t)> read_handler_;
};

main cpp program that sets up the server

#include <iostream>

class client {
public:
    client()
      : valid_(false)
    {}
    client(int id)
      : id_(id), valid_(true)
    {}
    
    const char * get_name() const {
        std::string str("Client ");
        str += std::to_string(id_);
        return str.c_str();
    }
private:
    int id_;
    bool valid_;
};

class chat_server {
public:
    chat_server(boost::asio::io_context& io_context, std::size_t port)
      : server_(io_context, port, std::bind(&chat_server::handle_accept, this, std::placeholders::_1),
        std::bind(&chat_server::handle_read, this, std::placeholders::_1, std::placeholders::_2, std::placeholders::_3))
    {}
    
    void handle_accept(std::size_t client_index) {
        std::scoped_lock lock(clients_mutex_);
        if (clients_.size() != client_index) {
            std::cerr << "New client connecting at index " << client_index << 
                    " however, clients_ vector currently has size " << clients_.size() << std::endl;
            if (clients_.size() < client_index) {
                clients_.resize(client_index);
                clients_.emplace_back(client_index);
            } else {
                clients_[client_index] = client(client_index);
            }
        } else {
            clients_.emplace_back(client_index);
        }
        std::cout << "New client with id: " << client_index << std::endl;
    }
    
    void handle_read(std::size_t sender, char* body, std::size_t length) {
        // whenever the server receives a message, this function will be called
        // where clients[sender] will be the connection that sent the message
        // body will be a pointer to the start of the body of the message
        // and length will be the length of the body 
        // we will process the message here and decide if / what to send in response
        // (for example, in a chat server, we'd want to forward the message to every client
        // with the name of the sender attached to it so that clients can update the chat dialogue)
        std::size_t sender_name_len = strlen(clients_[sender].get_name());
        std::size_t new_message_length = sender_name_len + length + 3;
        char new_message[new_message_length];
        
        sprintf(new_message, "%s: ", clients_[sender].get_name());
        memcpy(new_message + sender_name_len + 2, body, length);
        new_message[new_message_length - 1] = '\0';
        
        std::cout << new_message << std::endl;
        
        server_.send_to_all(new_message, new_message_length-1);
    }
private:
    net_server server_;
    std::vector<client> clients_;
    std::mutex clients_mutex_;
};

int main() {
    try {
        boost::asio::io_context io_context;
        chat_server serv(io_context, 1234);
        io_context.run();
    } catch (std::exception& e) {
        std::cerr << e.what() << std::endl;
    }
    
    return 0;
}

What I want is for my server class to maintain a list of tcp_connections which each represent a client that has connected to the server. When the server accepts a connection, a tcp_connection object is created for that connection and then that tcp_connection object starts an infinite asynchronous "read_header -> read_body -> repeat" loop. Whenever the server receives a message from any of the clients, it should format the message and then send it to every tcp_connection in the list.

Answer 1

Your connections_ member variable is being reallocated when you add new elements to it. In your various handlers in tcp_connection you are capturing this, when the vector is reallocated the value of this will change and your handlers will then try to operate on the old copy of the object causing undefined behaviour.

The simple solution is to make your connections_ vector a vector of std::shared_ptr.

It is also best practice to capture your object's shared_ptr in your handlers so that the object can't go out of scope before your callbacks execute. e.g.:

void do_write() {
    auto self = shared_from_this();
    boost::asio::async_write(socket_, boost::asio::buffer(write_messages_.front().get_data(), 
        write_messages_.front().get_body_length() + net_message::header_length),
      [self, this] (boost::system::error_code ec, std::size_t /*length*/) {
          if (!ec) {
              write_messages_.pop_front();
              if (!write_messages_.empty()) {
                  do_write();
              }
          } else {
              std::cerr << "error with writing to client " << id_ << " with error code: " << ec << std::endl;
          }
      });
}

You'll need to derive tcp_connection from std::shared_from_this<tcp_connection> and make sure that you have created the shared_ptr before setting up any handlers (e.g. don't create handlers in the constructor).