From 6208d7fa7c3c94a6d4445db85320bdfef45e6a74 Mon Sep 17 00:00:00 2001 From: Kevin Ahrendt Date: Mon, 18 May 2026 09:26:20 -0400 Subject: [PATCH 1/3] Have httpd session own the SendspinConnection --- docs/internals.md | 23 +++++++-- src/connection_manager.cpp | 46 ++++++----------- src/connection_manager.h | 8 +-- src/esp/server_connection.cpp | 94 ++++++++++++++++++++++++++--------- src/esp/server_connection.h | 4 +- src/esp/ws_server.cpp | 63 ++++++++++++----------- src/esp/ws_server.h | 39 ++++++++------- src/host/ws_server.cpp | 2 +- src/host/ws_server.h | 2 +- 9 files changed, 170 insertions(+), 111 deletions(-) diff --git a/docs/internals.md b/docs/internals.md index ec9bd80..e942bf4 100644 --- a/docs/internals.md +++ b/docs/internals.md @@ -382,13 +382,14 @@ The filter is protected by `state_mutex_` so that `compute_client_time()` can be ### Connection Management (`src/connection_manager.cpp`) -The `ConnectionManager` maintains up to three connection slots: +The `ConnectionManager` maintains two observer slots for the connections it routes messages through: | Slot | Purpose | |------|---------| | `current_connection_` | Active connection receiving messages | | `pending_connection_` | Handoff candidate completing its handshake | -| `dying_connection_` | Gracefully disconnecting (kept alive as shared_ptr until goodbye is sent) | + +Both are `std::shared_ptr`, and on the ESP server path they are observers rather than authoritative owners — the authoritative owner of a `SendspinServerConnection` is the httpd session itself (see [Server connection ownership (ESP)](#server-connection-ownership-esp)). On the host (IXWebSocket) client path the `shared_ptr` in these slots is the only owner. ### Handshake and Handoff @@ -419,7 +420,23 @@ When a connection is lost (`on_connection_lost`): ### Graceful Disconnect -`disconnect_and_release()` moves the connection into `dying_connection_` (as a `shared_ptr`) and sends a goodbye message with a completion callback. The callback sets a flag under the connection mutex, and the main loop's next tick destroys the connection. This two-phase approach prevents use-after-free when platform worker threads (e.g., ESP httpd) have pending work items referencing the connection. +`disconnect_and_release()` calls `conn->disconnect(reason, nullptr)` and lets the local `shared_ptr` go out of scope. + +- **ESP server**: the goodbye text is queued as an httpd worker job; the worker looks the connection up via `httpd_sess_get_ctx`, sends the frame, then runs the completion lambda that calls `trigger_close()`. The session slot installed in `open_callback` keeps the connection alive across that whole sequence even after `ConnectionManager`'s observer `shared_ptr` is dropped. The session is finally freed when httpd invokes the slot's `free_fn` (see [Server connection ownership (ESP)](#server-connection-ownership-esp)). The completion lambda captures a `weak_ptr` to make this lifetime explicit — `trigger_close()` is skipped if the conn has already been freed. +- **Host client**: the IXWebSocket send is synchronous, so the goodbye and close have both completed by the time `disconnect()` returns and the `shared_ptr` drops the last reference. + +### Server connection ownership (ESP) + +On the ESP build, `SendspinServerConnection` lifetime is pinned to the httpd session rather than to `ConnectionManager`: + +1. `SendspinWsServer::open_callback` (the httpd `open_fn`) creates the `shared_ptr`, heap-allocates a `shared_ptr*` slot, and calls `httpd_sess_set_ctx(handle, sockfd, slot, free_fn)` with a deleter that `delete`s the slot. That slot is the authoritative reference. +2. The same shared_ptr is forwarded into `ConnectionManager::on_new_connection`, which stores it in `current_connection_` or `pending_connection_` as a *secondary observer*. +3. The httpd WebSocket handler (`websocket_handler`) and queued workers (`async_send_text`, `async_send_time_text`) look the connection up by `httpd_sess_get_ctx(handle, sockfd)` at run time, copying the slot's `shared_ptr` for the duration of their work. They never assume the manager's observer slot is alive. +4. When the socket closes, httpd calls the `close_fn` first (which fires `connection_closed_callback_` so `ConnectionManager` can drop its observer in the next `loop()`), then later calls the slot's `free_fn` to release the authoritative reference once no workers are queued for that session. + +Queued workers carry only a `{httpd_handle_t, int sockfd}` pair (`AsyncRespArg` for text sends, `SessionLookup` for time sends) rather than capturing the connection directly. If the session has already been torn down by the time a worker runs, the lookup returns null and the worker no-ops cleanly. Both arg structs are allocated through `platform_malloc` / `platform_malloc_internal` and freed in the worker. + +The host build does not need this scheme: `SendspinWsServer` (host) routes IXWebSocket messages by calling `find_connection_callback_` to resolve a synthetic sockfd back to the connection that `ConnectionManager` is holding. The ESP build keeps the `set_find_connection_callback()` setter as a no-op stub for symmetry; see the comment at the call site in `ConnectionManager::init_server`. ## Ordering Guarantees Summary diff --git a/src/connection_manager.cpp b/src/connection_manager.cpp index 5ae86df..ded5af5 100644 --- a/src/connection_manager.cpp +++ b/src/connection_manager.cpp @@ -45,7 +45,6 @@ ConnectionManager::~ConnectionManager() { this->pending_connection_.reset(); } this->hello_retry_.conn.reset(); - this->dying_connection_.reset(); } // ============================================================================ @@ -101,20 +100,21 @@ void ConnectionManager::init_server(SendspinClient* client) { this->ws_server_->set_ctrl_port(this->client_->config_.httpd_ctrl_port); this->ws_server_->set_new_connection_callback( - [this](std::unique_ptr conn) { + [this](std::shared_ptr conn) { this->on_new_connection(std::move(conn)); }); this->ws_server_->set_connection_closed_callback([this](int sockfd) { SS_LOGD(TAG, "Connection closed callback for socket %d", sockfd); - // Defer the actual cleanup to loop() to avoid use-after-free. - // This callback runs in the httpd thread, but there may be pending httpd_queue_work items - // (e.g., async_send_text) with raw pointers to the connection object. If we destroy the - // connection here, those pending work items would dereference freed memory when processed. + // Defer the actual cleanup to loop() so on_connection_lost runs on the main thread + // alongside the rest of the connection state mutations. std::lock_guard lock(this->conn_mutex_); this->pending_close_events_.push_back(sockfd); }); + // Connection lookup-by-sockfd. Used by the host build's ws_server to route IXWebSocket + // messages; the ESP build ignores this and looks the connection up directly via + // httpd_sess_get_ctx (set in open_callback), so its setter is a no-op stub. this->ws_server_->set_find_connection_callback( [this](int sockfd) -> std::shared_ptr { std::lock_guard lock(this->conn_ptr_mutex_); @@ -126,9 +126,6 @@ void ConnectionManager::init_server(SendspinClient* client) { this->pending_connection_->get_sockfd() == sockfd) { return this->pending_connection_; } - // Deliberately excludes dying_connection_; it has already been through cleanup and its - // message dispatch is disabled. Returning it here would let httpd route stale messages - // from the old connection into freshly-reset role queues. return nullptr; }); } @@ -149,15 +146,12 @@ void ConnectionManager::loop() { std::vector> connected_events; std::vector> disconnect_events; std::vector hello_events; - bool release_dying = false; { std::lock_guard lock(this->conn_mutex_); close_events.swap(this->pending_close_events_); connected_events.swap(this->pending_connected_events_); disconnect_events.swap(this->pending_disconnect_events_); hello_events.swap(this->pending_hello_events_); - release_dying = this->dying_connection_ready_to_release_; - this->dying_connection_ready_to_release_ = false; } { @@ -214,10 +208,6 @@ void ConnectionManager::loop() { } } } - - if (release_dying) { - this->dying_connection_.reset(); - } } // Call loop on active connections using shared_ptr copies to avoid holding the lock @@ -320,10 +310,10 @@ void ConnectionManager::setup_connection_callbacks(SendspinConnection* conn) { }; } -void ConnectionManager::on_new_connection(std::unique_ptr conn) { - // Called from httpd open_callback thread. Connection pointer assignment must happen inline - // because the httpd find_connection_callback needs to locate this connection immediately - // for subsequent message routing on the same thread. +void ConnectionManager::on_new_connection(std::shared_ptr conn) { + // Called from the platform's new-connection callback (ESP: httpd open_callback). The + // authoritative owner is the platform's session/transport context; this observer shared_ptr + // can be reset at any time without freeing the conn out from under in-flight workers. conn->init_time_filter(); conn->set_websocket_payload_location(this->client_->config_.websocket_payload_location); @@ -340,8 +330,7 @@ void ConnectionManager::on_new_connection(std::unique_ptrpending_connection_ != nullptr) { SS_LOGW(TAG, "Already have pending connection, rejecting new connection"); - this->disconnect_and_release(std::shared_ptr(std::move(conn)), - SendspinGoodbyeReason::ANOTHER_SERVER); + this->disconnect_and_release(std::move(conn), SendspinGoodbyeReason::ANOTHER_SERVER); return; } this->pending_connection_ = std::move(conn); @@ -495,14 +484,11 @@ void ConnectionManager::complete_handoff(bool switch_to_new) { void ConnectionManager::disconnect_and_release(std::shared_ptr conn, SendspinGoodbyeReason reason) { - this->dying_connection_ = std::move(conn); - this->dying_connection_->disconnect(reason, [this]() { - // Defer the actual destruction to loop() to avoid use-after-free. - // This callback runs in the httpd worker thread (async_send_text), but the connection - // must stay alive until all pending httpd_queue_work items have been processed. - std::lock_guard lock(this->conn_mutex_); - this->dying_connection_ready_to_release_ = true; - }); + // Send the goodbye and let the conn shared_ptr go out of scope. On ESP the httpd session slot + // keeps the conn alive until the goodbye worker runs, calls trigger_close, the session tears + // down, and the slot's free_fn fires. On host the IXWebSocket send is synchronous, so the + // goodbye + close have both completed by the time disconnect() returns. + conn->disconnect(reason, nullptr); } } // namespace sendspin diff --git a/src/connection_manager.h b/src/connection_manager.h index 2511355..4aa1a2c 100644 --- a/src/connection_manager.h +++ b/src/connection_manager.h @@ -147,8 +147,10 @@ class ConnectionManager { /// @param conn The connection to configure. void setup_connection_callbacks(SendspinConnection* conn); /// @brief Accepts an incoming server connection as current or pending for handoff. - /// @param conn The newly accepted server connection. - void on_new_connection(std::unique_ptr conn); + /// @param conn The newly accepted server connection. The session slot keeps a parallel + /// refcount, so this observer can be reset at any time without freeing the conn + /// out from under in-flight httpd workers. + void on_new_connection(std::shared_ptr conn); // ======================================== // Hello handshake @@ -196,7 +198,6 @@ class ConnectionManager { // Pointer fields SendspinClient* client_; std::shared_ptr current_connection_; - std::shared_ptr dying_connection_; std::shared_ptr pending_connection_; std::unique_ptr ws_server_; @@ -204,7 +205,6 @@ class ConnectionManager { uint32_t last_played_server_hash_{0}; // 8-bit fields - bool dying_connection_ready_to_release_{false}; bool has_last_played_server_{false}; }; diff --git a/src/esp/server_connection.cpp b/src/esp/server_connection.cpp index f2cec5a..8186000 100644 --- a/src/esp/server_connection.cpp +++ b/src/esp/server_connection.cpp @@ -31,15 +31,38 @@ static const char* const TAG = "sendspin.server_connection"; // Static helpers // ============================================================================ -/// @brief Structure holding connection context and payload data for async send operations +/// @brief Structure holding the session identity and payload data for async send operations +/// +/// Workers look the connection up at run time via `httpd_sess_get_ctx(server, sockfd)`. The +/// session-pinned shared_ptr keeps the conn alive until httpd has drained queued work for the +/// session and called the slot's free_fn, so the worker can either find a valid conn or cleanly +/// no-op if the session is already gone. struct AsyncRespArg { - void* context; - uint8_t* payload; - size_t len; + httpd_handle_t server{nullptr}; + int sockfd{-1}; + uint8_t* payload{nullptr}; + size_t len{0}; bool has_callback{false}; SendCompleteCallback on_complete; }; +/// @brief Small heap struct used by the time-message worker to identify its session +struct SessionLookup { + httpd_handle_t server; + int sockfd; +}; + +/// @brief Looks up the session-pinned connection shared_ptr, returning a copy or empty if gone +static std::shared_ptr lookup_session_conn(httpd_handle_t server, + int sockfd) { + auto* slot = + static_cast*>(httpd_sess_get_ctx(server, sockfd)); + if (slot == nullptr) { + return nullptr; + } + return *slot; +} + // ============================================================================ // SendspinConnection interface implementation // ============================================================================ @@ -71,11 +94,19 @@ void SendspinServerConnection::disconnect(SendspinGoodbyeReason reason, return; } - // Send goodbye message, then trigger close, then invoke user callback - // Capture on_complete by value to keep it alive until async callback fires - this->send_goodbye_reason(reason, [this, on_complete](bool success) { - // Trigger close regardless of send success - this->trigger_close(); + // Send goodbye, then trigger close, then invoke the user callback. Capture a weak_ptr to + // self instead of raw `this`: the worker normally finds the conn via the session slot + // (keeping it alive through the completion), but a weak_ptr makes that invariant explicit + // and avoids any UAF if the worker ever runs after the slot has been freed (e.g., across + // ESP-IDF versions whose httpd drain-before-free_fn ordering differs from the version we + // designed against). Skipping trigger_close() when the conn is already gone is harmless -- + // the session is gone too. + std::weak_ptr weak_self = + std::static_pointer_cast(this->shared_from_this()); + this->send_goodbye_reason(reason, [weak_self, on_complete](bool /*success*/) { + if (auto self = weak_self.lock()) { + self->trigger_close(); + } // Invoke user-provided completion callback if provided. // Already running in httpd worker thread context (async_send_text), @@ -113,7 +144,8 @@ SsErr SendspinServerConnection::send_text_message(const std::string& message, // Use placement new to properly construct the struct with the callback new (resp_arg) AsyncRespArg(); - resp_arg->context = (void*)this; + resp_arg->server = this->server_; + resp_arg->sockfd = this->sockfd_; resp_arg->payload = static_cast(platform_malloc(message.size())); if (resp_arg->payload == nullptr) { SS_LOGE(TAG, "Failed to allocate %zu bytes for message payload", message.size()); @@ -214,20 +246,34 @@ bool SendspinServerConnection::send_time_message() { return false; } - // The worker job only needs the connection pointer (no buffer to free), so pass `this` - // directly. The JSON is built inside the worker so client_transmitted is captured as - // close to the wire send as possible. - if (httpd_queue_work(this->server_, async_send_time_text, this) != ESP_OK) { + // The worker looks up the conn by sockfd at run time via `httpd_sess_get_ctx`. The session + // slot keeps the conn alive past any ConnectionManager teardown that races the httpd ctrl + // queue. The JSON is built inside the worker so client_transmitted is captured as close to + // the wire send as possible. SessionLookup is POD so a raw malloc/free pair (matching the + // AsyncRespArg allocator convention) is sufficient; no constructor/destructor to invoke. + auto* lookup = static_cast(platform_malloc_internal(sizeof(SessionLookup))); + if (lookup == nullptr) { + SS_LOGE(TAG, "Failed to allocate SessionLookup for time message"); + return false; + } + lookup->server = this->server_; + lookup->sockfd = this->sockfd_; + if (httpd_queue_work(this->server_, async_send_time_text, lookup) != ESP_OK) { SS_LOGE(TAG, "httpd_queue_work failed for time message"); + platform_free(lookup); return false; } return true; } void SendspinServerConnection::async_send_time_text(void* arg) { - auto* this_conn = static_cast(arg); + auto* lookup = static_cast(arg); + const httpd_handle_t server = lookup->server; + const int sockfd = lookup->sockfd; + platform_free(lookup); - if (!this_conn->is_connected()) { + auto conn = lookup_session_conn(server, sockfd); + if (!conn || !conn->is_connected()) { return; } @@ -249,9 +295,9 @@ void SendspinServerConnection::async_send_time_text(void* arg) { ws_pkt.payload = reinterpret_cast(buf); ws_pkt.len = len; - this_conn->update_serialize_ema(esp_timer_get_time() - client_transmitted); + conn->update_serialize_ema(esp_timer_get_time() - client_transmitted); - httpd_ws_send_frame_async(this_conn->server_, this_conn->sockfd_, &ws_pkt); + httpd_ws_send_frame_async(conn->server_, conn->sockfd_, &ws_pkt); } void SendspinServerConnection::async_send_text(void* arg) { @@ -259,19 +305,21 @@ void SendspinServerConnection::async_send_text(void* arg) { httpd_ws_frame_t ws_pkt; memset(&ws_pkt, 0, sizeof(httpd_ws_frame_t)); - SendspinServerConnection* this_conn = (SendspinServerConnection*)resp_arg->context; - ws_pkt.payload = resp_arg->payload; ws_pkt.len = resp_arg->len; ws_pkt.type = HTTPD_WS_TYPE_TEXT; + // Look up the conn via the session slot. If the session has already been torn down (e.g., + // trigger_close was processed first) the slot is gone and we treat this as a failed send. + auto conn = lookup_session_conn(resp_arg->server, resp_arg->sockfd); bool send_success = false; - if (this_conn->is_connected()) { - esp_err_t err = httpd_ws_send_frame_async(this_conn->server_, this_conn->sockfd_, &ws_pkt); + if (conn && conn->is_connected()) { + esp_err_t err = httpd_ws_send_frame_async(conn->server_, conn->sockfd_, &ws_pkt); send_success = (err == ESP_OK); } - // Call the completion callback if provided + // Call the completion callback if provided. Always invoke (rather than dropping silently) so + // chained logic such as disconnect()'s trigger_close still runs for the no-session case. if (resp_arg->has_callback) { resp_arg->on_complete(send_success); } diff --git a/src/esp/server_connection.h b/src/esp/server_connection.h index 28e6c78..810f394 100644 --- a/src/esp/server_connection.h +++ b/src/esp/server_connection.h @@ -134,7 +134,9 @@ class SendspinServerConnection : public SendspinConnection { /// /// Captures the client_transmitted timestamp inside the worker (just before /// `httpd_ws_send_frame_async`), serializes the JSON, then sends. - /// @param arg The owning `SendspinServerConnection*` (passed directly, not heap-allocated). + /// @param arg A heap-allocated `SessionLookup` carrying the server handle and sockfd. The + /// worker looks the connection up via `httpd_sess_get_ctx`, sends the frame if the + /// session is still alive, and frees the arg on return. static void async_send_time_text(void* arg); // Pointer fields diff --git a/src/esp/ws_server.cpp b/src/esp/ws_server.cpp index d7b5681..8a2d494 100644 --- a/src/esp/ws_server.cpp +++ b/src/esp/ws_server.cpp @@ -125,12 +125,20 @@ esp_err_t SendspinWsServer::open_callback(httpd_handle_t handle, int sockfd) { return ESP_FAIL; } - // Create a new connection instance - auto connection = std::make_unique(handle, sockfd); + // Pin the connection to the httpd session: a heap-allocated shared_ptr is set as the session + // context, with a free_fn that drops the refcount when the session is torn down. httpd invokes + // the close_fn before the free_fn, so observers (ConnectionManager) get notified first and any + // queued workers that have already started look up the same shared_ptr via httpd_sess_get_ctx + // and run safely until the session is freed. + auto conn = std::make_shared(handle, sockfd); + auto* slot = new std::shared_ptr(conn); + httpd_sess_set_ctx(handle, sockfd, slot, [](void* p) { + delete static_cast*>(p); + }); // Notify the client of the new connection (client decides whether to keep it) if (server->new_connection_callback_) { - server->new_connection_callback_(std::move(connection)); + server->new_connection_callback_(std::move(conn)); } else { SS_LOGW(TAG, "No new connection callback set, connection will be dropped"); } @@ -143,7 +151,10 @@ void SendspinWsServer::close_callback(httpd_handle_t handle, int sockfd) { SendspinWsServer* server = (SendspinWsServer*)httpd_get_global_user_ctx(handle); - // Notify the client so it can identify and clean up the connection + // Notify ConnectionManager so it can drop its observer shared_ptr. The session slot (set in + // open_callback) keeps the connection alive until httpd invokes the slot's free_fn next, which + // ensures any in-flight workers that are still looking it up via httpd_sess_get_ctx see a + // valid object. if (server != nullptr && server->connection_closed_callback_) { server->connection_closed_callback_(sockfd); } @@ -160,39 +171,33 @@ SS_HOT esp_err_t SendspinWsServer::websocket_handler(httpd_req_t* req) { // Capture timestamp immediately for accurate time synchronization int64_t receive_time = esp_timer_get_time(); - SendspinWsServer* server = (SendspinWsServer*)req->user_ctx; + // Look up the connection via httpd's per-session ctx. The slot was pinned in open_callback and + // is freed only after this handler unwinds for a given session, so the shared_ptr copy below + // is always valid. Copying the shared_ptr keeps the conn alive for the duration of dispatch + // even if a teardown is racing on another thread. This replaces the previous cross-thread + // find_connection_callback + mutex. + int sockfd = httpd_req_to_sockfd(req); + auto* slot = static_cast*>( + httpd_sess_get_ctx(req->handle, sockfd)); + if (slot == nullptr || !*slot) { + SS_LOGE(TAG, "No connection found for sockfd %d", sockfd); + return ESP_FAIL; + } + std::shared_ptr conn_holder = *slot; + SendspinServerConnection* conn = conn_holder.get(); // Handle WebSocket handshake (HTTP_GET) if (req->method == HTTP_GET) { - // Find the connection and invoke its on_connected_cb callback - int sockfd = httpd_req_to_sockfd(req); - std::shared_ptr conn_holder; - SendspinServerConnection* conn = nullptr; - if (server->find_connection_callback_) { - conn_holder = server->find_connection_callback_(sockfd); - conn = static_cast(conn_holder.get()); - } - if (conn != nullptr && conn->on_connected_cb) { + if (conn->on_connected_cb) { conn->on_connected_cb(conn); } return ESP_OK; } - // Find connection by sockfd; hold shared_ptr to keep it alive during dispatch - int sockfd = httpd_req_to_sockfd(req); - std::shared_ptr conn_holder; - SendspinServerConnection* conn = nullptr; - if (server->find_connection_callback_) { - conn_holder = server->find_connection_callback_(sockfd); - conn = static_cast(conn_holder.get()); - } - - if (conn == nullptr) { - SS_LOGE(TAG, "No connection found for sockfd %d", sockfd); - return ESP_FAIL; - } - - // Delegate to connection's handle_data + // Delegate to connection's handle_data. Stale messages (i.e., after the connection has been + // dropped from ConnectionManager's observer slots) are short-circuited inside the connection + // via disable_message_dispatch(), so a still-alive session-pinned conn does not leak messages + // into freshly-reset role queues. return conn->handle_data(req, receive_time); } diff --git a/src/esp/ws_server.h b/src/esp/ws_server.h index 023d434..52d846f 100644 --- a/src/esp/ws_server.h +++ b/src/esp/ws_server.h @@ -33,35 +33,36 @@ class SendspinServerConnection; * @brief WebSocket server listener for Sendspin * * Manages the ESP-IDF HTTP server (httpd) that listens for incoming WebSocket - * connections from Sendspin servers. It does not own the connections long-term; - * instead, it creates SendspinServerConnection instances and hands them to the - * client, which decides whether to accept or reject them based on handoff logic. + * connections from Sendspin servers. The authoritative owner of each accepted + * SendspinServerConnection is the httpd session itself: open_callback() pins a + * shared_ptr onto the session via httpd_sess_set_ctx with a free_fn deleter, and + * the websocket_handler / queued workers look the connection up at run time via + * httpd_sess_get_ctx. ConnectionManager receives the same shared_ptr as a secondary + * observer for routing and handoff decisions. * * Capabilities: * - Accepts incoming WebSocket connections on a dedicated port - * - Routes WebSocket messages to the appropriate connection object + * - Routes WebSocket messages directly via the session-pinned shared_ptr (no cross-thread + * find-by-sockfd lookup is needed) * - Manages open/close callbacks to notify the client of connection lifecycle events * - Supports up to max_connections simultaneous sockets (default: 2) to enable the * handoff protocol where a second server can connect while one is already active * * Usage: * 1. Construct with a SendspinClient pointer (passed to start()) - * 2. Register callbacks via set_new_connection_callback(), set_connection_closed_callback(), - * and set_find_connection_callback() + * 2. Register callbacks via set_new_connection_callback() and set_connection_closed_callback() + * (set_find_connection_callback() is a no-op stub on ESP, kept for symmetry with the host build) * 3. Call start() to begin listening for incoming connections * 4. Call stop() to shut down the server * * @code * SendspinWsServer ws_server; - * ws_server.set_new_connection_callback([&](std::unique_ptr conn) { + * ws_server.set_new_connection_callback([&](std::shared_ptr conn) { * client.on_new_server_connection(std::move(conn)); * }); * ws_server.set_connection_closed_callback([&](int sockfd) { * client.on_server_connection_closed(sockfd); * }); - * ws_server.set_find_connection_callback([&](int sockfd) -> SendspinServerConnection* { - * return client.find_server_connection(sockfd); - * }); * ws_server.start(&client, true, 5); * @endcode */ @@ -71,8 +72,9 @@ class SendspinWsServer { ~SendspinWsServer(); /// @brief Callback type for notifying the client of new connections - /// The client receives ownership of the connection and must decide whether to keep it. - using NewConnectionCallback = std::function)>; + /// The client receives a shared_ptr; the connection is also pinned to the httpd session via + /// httpd_sess_set_ctx, which acts as the authoritative owner for the connection's lifetime. + using NewConnectionCallback = std::function)>; /// @brief Callback type for notifying the client when a socket closes /// The client needs to identify which connection owns this sockfd and clean it up. @@ -99,10 +101,12 @@ class SendspinWsServer { } /// @brief Sets callback to find a connection by socket fd - /// @param callback The callback function. - void set_find_connection_callback(FindConnectionCallback&& callback) { - this->find_connection_callback_ = std::move(callback); - } + /// + /// On ESP this is a no-op: `websocket_handler` runs on the httpd task and looks the connection + /// up directly via `httpd_sess_get_ctx`, which is also where the connection's authoritative + /// owner lives. The setter is kept for API symmetry with the host build. + /// @param callback Ignored. + void set_find_connection_callback(FindConnectionCallback&& /*callback*/) {} /// @brief Configures the maximum number of simultaneous connections /// Default is 2 to support the handoff protocol (one active + one pending). @@ -157,9 +161,6 @@ class SendspinWsServer { /// @brief Callback to notify the client when a socket closes ConnectionClosedCallback connection_closed_callback_; - /// @brief Callback to find a connection by socket fd - FindConnectionCallback find_connection_callback_; - /// @brief Callback to notify the client of new connections NewConnectionCallback new_connection_callback_; diff --git a/src/host/ws_server.cpp b/src/host/ws_server.cpp index 4e29086..df453f9 100644 --- a/src/host/ws_server.cpp +++ b/src/host/ws_server.cpp @@ -59,7 +59,7 @@ bool SendspinWsServer::start(SendspinClient* client, bool /*task_stack_in_psram* SS_LOGD(TAG, "New client connection (synthetic sockfd %d)", synthetic_sockfd); // Create the server connection - auto connection = std::make_unique(ws, synthetic_sockfd); + auto connection = std::make_shared(ws, synthetic_sockfd); // Set up the message callback on the websocket to route data through the connection ws->setOnMessageCallback([this, synthetic_sockfd](const ix::WebSocketMessagePtr& msg) { diff --git a/src/host/ws_server.h b/src/host/ws_server.h index eaf907f..2fa9913 100644 --- a/src/host/ws_server.h +++ b/src/host/ws_server.h @@ -61,7 +61,7 @@ class SendspinWsServer { ~SendspinWsServer(); /// @brief Callback type for notifying the client of new connections - using NewConnectionCallback = std::function)>; + using NewConnectionCallback = std::function)>; /// @brief Callback type for notifying the client when a socket closes using ConnectionClosedCallback = std::function; From 6e499a6f94ff6ade60c675a5428cf7ea908f79bb Mon Sep 17 00:00:00 2001 From: Kevin Ahrendt Date: Mon, 18 May 2026 09:47:38 -0400 Subject: [PATCH 2/3] Fix clang-tidy issue --- src/connection_manager.cpp | 14 ++++++++------ src/connection_manager.h | 7 ++++--- 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/src/connection_manager.cpp b/src/connection_manager.cpp index ded5af5..d170e7b 100644 --- a/src/connection_manager.cpp +++ b/src/connection_manager.cpp @@ -482,13 +482,15 @@ void ConnectionManager::complete_handoff(bool switch_to_new) { } } -void ConnectionManager::disconnect_and_release(std::shared_ptr conn, +void ConnectionManager::disconnect_and_release(std::shared_ptr&& conn, SendspinGoodbyeReason reason) { - // Send the goodbye and let the conn shared_ptr go out of scope. On ESP the httpd session slot - // keeps the conn alive until the goodbye worker runs, calls trigger_close, the session tears - // down, and the slot's free_fn fires. On host the IXWebSocket send is synchronous, so the - // goodbye + close have both completed by the time disconnect() returns. - conn->disconnect(reason, nullptr); + // Take ownership of the caller's shared_ptr into a local, send the goodbye, then let the + // local go out of scope. On ESP the httpd session slot keeps the conn alive until the + // goodbye worker runs, calls trigger_close, the session tears down, and the slot's free_fn + // fires. On host the IXWebSocket send is synchronous, so the goodbye + close have both + // completed by the time disconnect() returns. + auto local = std::move(conn); + local->disconnect(reason, nullptr); } } // namespace sendspin diff --git a/src/connection_manager.h b/src/connection_manager.h index 4aa1a2c..5929079 100644 --- a/src/connection_manager.h +++ b/src/connection_manager.h @@ -180,10 +180,11 @@ class ConnectionManager { /// @brief Completes a server handoff by promoting or discarding the pending connection. /// @param switch_to_new True to promote the pending connection, false to discard it. void complete_handoff(bool switch_to_new); - /// @brief Sends a goodbye, then defers destruction of the connection until loop() runs. - /// @param conn The connection to disconnect and release. + /// @brief Sends a goodbye and takes ownership of the caller's shared_ptr so it drops at + /// function exit. + /// @param conn The connection to disconnect and release. Caller's shared_ptr is left empty. /// @param reason The goodbye reason to send before closing. - void disconnect_and_release(std::shared_ptr conn, + void disconnect_and_release(std::shared_ptr&& conn, SendspinGoodbyeReason reason); // Struct fields From add52af9c2f4b3eac69c46cf89c941771ff63fb1 Mon Sep 17 00:00:00 2001 From: Kevin Ahrendt Date: Mon, 18 May 2026 09:54:24 -0400 Subject: [PATCH 3/3] Copilot feedback --- src/esp/server_connection.cpp | 20 ++++++++++---------- src/esp/ws_server.cpp | 16 +++++++++------- 2 files changed, 19 insertions(+), 17 deletions(-) diff --git a/src/esp/server_connection.cpp b/src/esp/server_connection.cpp index 8186000..69e03a0 100644 --- a/src/esp/server_connection.cpp +++ b/src/esp/server_connection.cpp @@ -309,19 +309,19 @@ void SendspinServerConnection::async_send_text(void* arg) { ws_pkt.len = resp_arg->len; ws_pkt.type = HTTPD_WS_TYPE_TEXT; - // Look up the conn via the session slot. If the session has already been torn down (e.g., - // trigger_close was processed first) the slot is gone and we treat this as a failed send. + // Look up the conn via the session slot and invoke on_complete only when the conn is still + // alive. If the session has already been torn down the slot is gone and we drop the completion + // callback silently: callers frequently capture the connection by raw pointer (e.g., + // ConnectionManager::send_hello_message), and dereferencing it after teardown would be a UAF. + // Lifetime-safe completion logic (e.g., disconnect()'s weak_ptr-guarded trigger_close) already + // tolerates not firing in this case. The AsyncRespArg destructor below releases any captured + // state held by the std::function. auto conn = lookup_session_conn(resp_arg->server, resp_arg->sockfd); - bool send_success = false; if (conn && conn->is_connected()) { esp_err_t err = httpd_ws_send_frame_async(conn->server_, conn->sockfd_, &ws_pkt); - send_success = (err == ESP_OK); - } - - // Call the completion callback if provided. Always invoke (rather than dropping silently) so - // chained logic such as disconnect()'s trigger_close still runs for the no-session case. - if (resp_arg->has_callback) { - resp_arg->on_complete(send_success); + if (resp_arg->has_callback) { + resp_arg->on_complete(err == ESP_OK); + } } platform_free(ws_pkt.payload); diff --git a/src/esp/ws_server.cpp b/src/esp/ws_server.cpp index 8a2d494..85bd192 100644 --- a/src/esp/ws_server.cpp +++ b/src/esp/ws_server.cpp @@ -125,6 +125,14 @@ esp_err_t SendspinWsServer::open_callback(httpd_handle_t handle, int sockfd) { return ESP_FAIL; } + // Reject the session before allocating anything if there is nobody to deliver the connection + // to; otherwise the session would sit pinned with no one driving its handshake until the peer + // gives up. + if (!server->new_connection_callback_) { + SS_LOGW(TAG, "No new connection callback set, rejecting session"); + return ESP_FAIL; + } + // Pin the connection to the httpd session: a heap-allocated shared_ptr is set as the session // context, with a free_fn that drops the refcount when the session is torn down. httpd invokes // the close_fn before the free_fn, so observers (ConnectionManager) get notified first and any @@ -136,13 +144,7 @@ esp_err_t SendspinWsServer::open_callback(httpd_handle_t handle, int sockfd) { delete static_cast*>(p); }); - // Notify the client of the new connection (client decides whether to keep it) - if (server->new_connection_callback_) { - server->new_connection_callback_(std::move(conn)); - } else { - SS_LOGW(TAG, "No new connection callback set, connection will be dropped"); - } - + server->new_connection_callback_(std::move(conn)); return ESP_OK; }