diff --git a/src/AsyncWebSocket.cpp b/src/AsyncWebSocket.cpp index 9268eb19..92e0bfe0 100644 --- a/src/AsyncWebSocket.cpp +++ b/src/AsyncWebSocket.cpp @@ -35,6 +35,12 @@ #define STATE_FRAME_MASK 1 #define STATE_FRAME_DATA 2 +// Sentinel returned by webSocketSendFrame() when a torn frame is detected +// (partial add() after header committed, or header add() / malloc failure). +// Distinct from 0, which means "busy, try again later". The caller must not +// roll back its send offset and must schedule a connection close. +constexpr size_t WS_FRAME_TORN = static_cast(-1); + using namespace asyncsrv; size_t webSocketSendFrameWindow(AsyncClient *client) { @@ -83,8 +89,9 @@ size_t webSocketSendFrame(AsyncClient *client, bool final, uint8_t opcode, bool uint8_t *buf = (uint8_t *)malloc(headLen); if (buf == NULL) { async_ws_log_e("Failed to allocate"); - client->abort(); - return 0; + // Nothing is on the wire yet. Signal hard error so the caller closes + // via the safe onDisconnect path (abort() leaks: no onDisconnect fires). + return WS_FRAME_TORN; } buf[0] = opcode & 0x0F; @@ -103,10 +110,11 @@ size_t webSocketSendFrame(AsyncClient *client, bool final, uint8_t opcode, bool memcpy(buf + (headLen - 4), mbuf, 4); } if (client->add((const char *)buf, headLen) != headLen) { - // os_printf("error adding %lu header bytes\n", headLen); free(buf); - // Serial.println("SF 4"); - return 0; + // Header could not be fully committed; nothing usable is on the wire. + // Signal hard error so the caller closes via the safe onDisconnect path + // (abort() leaks: no onDisconnect fires on ESP32 or ESP8266). + return WS_FRAME_TORN; } free(buf); @@ -118,17 +126,19 @@ size_t webSocketSendFrame(AsyncClient *client, bool final, uint8_t opcode, bool } } if (client->add((const char *)data, len) != len) { - // os_printf("error adding %lu data bytes\n", len); - // Serial.println("SF 5"); - return 0; + // The header is already committed but the payload was only partially + // accepted (lwIP segment exhaustion under load). The stream is now + // unrecoverable. Returning 0 would make the caller roll back its offset + // and re-send with a NEW frame header injected mid-payload. Signal hard + // error so the caller closes via the safe onDisconnect path (abort() + // leaks: no onDisconnect fires on ESP32 or ESP8266). + return WS_FRAME_TORN; } } - if (!client->send()) { - // os_printf("error sending frame: %lu\n", headLen+len); - // Serial.println("SF 6"); - return 0; - } - // Serial.println("SF"); + // A failing send() is not an error: the frame is fully queued in lwIP and + // will be flushed by the next poll/ack cycle. Returning 0 here caused a + // rollback and a duplicate re-send of already-queued bytes. + client->send(); return len; } @@ -228,6 +238,14 @@ size_t AsyncWebSocketMessage::send(AsyncClient *client) { size_t sent = webSocketSendFrame(client, final, opCode, _mask, dPtr, toSend); _status = WS_MSG_SENDING; + if (sent == WS_FRAME_TORN) { + // Torn frame: header (and possibly partial payload) is already on the wire. + // Do NOT roll back _sent/_ack — re-sending from a rolled-back offset would + // inject a new frame header mid-payload and corrupt the stream. Mark the + // message errored so _runQueue schedules a safe connection close. + _status = WS_MSG_ERROR; + return 0; + } if (toSend && sent != toSend) { _sent -= (toSend - sent); _ack -= (toSend - sent); @@ -361,6 +379,26 @@ void AsyncWebSocketClient::_onPoll() { return; } + // A torn frame was detected during a previous _runQueue() call. We cannot + // close from inside _runQueue: its callers hold _queue_lock, and close() + // fires _onDisconnect which re-acquires _queue_lock (non-recursive → deadlock) + // and would delete *this mid-iteration. Defer to here: unlock, then abort() + // to send an RST (immediate reset, no TIME_WAIT) and manually run _onDisconnect() + // + delete c to replicate what the onDisconnect callback would have done. + // abort() alone does not fire onDisconnect on any platform (ESP32 AsyncTCP, + // ESP8266 ESPAsyncTCP, RPAsyncTCP), which would permanently leak the + // AsyncWebSocketClient + AsyncClient + queues. + if (_abortPending) { + AsyncClient *c = _client; + if (c) { + lock.unlock(); + c->abort(); // RST + free pcb + _onDisconnect(); // nulls _client, erases *this from _clients → ~AsyncWebSocketClient + delete c; // free the AsyncClient object (normally done by the onDisconnect lambda) + } + return; + } + if (_client && _client->canSend() && (!_controlQueue.empty() || !_messageQueue.empty())) { _runQueue(); } else if (_keepAlivePeriod > 0 && (millis() - _lastMessageTime) >= _keepAlivePeriod && (_controlQueue.empty() && _messageQueue.empty())) { @@ -374,6 +412,9 @@ void AsyncWebSocketClient::_runQueue() { if (!_client) { return; } + if (_abortPending) { + return; // connection is being torn down; do not attempt further sends + } _clearQueue(); @@ -392,14 +433,20 @@ void AsyncWebSocketClient::_runQueue() { } if (space > (size_t)(ctrl.len() - 1)) { async_ws_log_v("[%s][%" PRIu32 "] SEND CTRL %" PRIu8, _server->url(), _clientId, ctrl.opcode()); - ctrl.send(_client); + size_t sent = ctrl.send(_client); + if (sent == WS_FRAME_TORN) { + async_ws_log_e("[%s][%" PRIu32 "] TORN CTRL FRAME: scheduling close", _server->url(), _clientId); + _abortPending = true; + _status = WS_DISCONNECTING; + break; + } space = webSocketSendFrameWindow(_client); } } } - // then we can send message frames if there is space - if (space) { + // then we can send message frames if there is space and no abort is pending + if (!_abortPending && space) { for (auto &msg : _messageQueue) { if (msg._remainingBytesToSend()) { async_ws_log_v( @@ -411,6 +458,13 @@ void AsyncWebSocketClient::_runQueue() { msg.send(_client); space = webSocketSendFrameWindow(_client); + if (msg._status == WS_MSG_ERROR) { + async_ws_log_e("[%s][%" PRIu32 "] TORN FRAME: scheduling close", _server->url(), _clientId); + _abortPending = true; + _status = WS_DISCONNECTING; + break; + } + // If we haven't finished sending this message, we must stop here to preserve WebSocket ordering. // We can only pipeline subsequent messages if the current one is fully passed to TCP buffer. if (msg._remainingBytesToSend()) { @@ -526,15 +580,21 @@ void AsyncWebSocketClient::close(uint16_t code, const char *message) { free(buf); return; } else { + // Could not allocate the close-frame payload. We can't send a proper WS + // close frame, so abort() to send an RST and manually run _onDisconnect() + // + delete _client to replicate what the onDisconnect callback would have + // done. abort() alone leaks (no onDisconnect fires on any platform). + // close() on the AsyncClient is unsuitable here because this method may + // be called with _queue_lock held (it isn't held right now, but the + // existing _onAck close path handles that separately). async_ws_log_e("Failed to allocate"); - // Reads _client, then dereference it without any lock. - // A concurrent _onDisconnect could null + delete the client between the check and the use. - // Local capture ensures the pointer is read exactly once, eliminating the null-dereference. - // (TOCTOU) AsyncClient *c = _client; if (c) { c->abort(); + _onDisconnect(); + delete c; } + return; } } _queueControl(WS_DISCONNECT); @@ -790,8 +850,17 @@ void AsyncWebSocketClient::_handleDataEvent(uint8_t *data, size_t len, bool endO _server->_handleEvent(this, WS_EVT_DATA, (void *)&_pinfo, copy.get(), len); } else { async_ws_log_e("Failed to allocate"); - if (_client) { - _client->abort(); + // abort() to send an RST, then manually run _onDisconnect() + delete _client + // to replicate what the onDisconnect callback would have done. abort() alone + // leaks (no onDisconnect fires on any platform). close() is unsafe here + // because _handleDataEvent is called from _onData which does not hold + // _queue_lock, but close() would fire _onDisconnect → _handleDisconnect + // → erase *this from _clients mid-call. + AsyncClient *c = _client; + if (c) { + c->abort(); + _onDisconnect(); + delete c; } } } else { diff --git a/src/AsyncWebSocket.h b/src/AsyncWebSocket.h index 16cd8fc2..f0c44632 100644 --- a/src/AsyncWebSocket.h +++ b/src/AsyncWebSocket.h @@ -226,8 +226,9 @@ class AsyncWebSocketClient { std::deque _controlQueue; std::deque _messageQueue; bool _closeWhenFull = false; + bool _abortPending = false; // set when a torn frame is detected; _onPoll closes the connection safely - AwsFrameInfo _pinfo; + AwsFrameInfo _pinfo; bool _queueControl(uint8_t opcode, const uint8_t *data = NULL, size_t len = 0, bool mask = false); bool _queueMessage(AsyncWebSocketSharedBuffer buffer, uint8_t opcode = WS_TEXT, bool mask = false); diff --git a/src/WebHandlers.cpp b/src/WebHandlers.cpp index ad3c1c1e..76c5500d 100644 --- a/src/WebHandlers.cpp +++ b/src/WebHandlers.cpp @@ -178,8 +178,11 @@ bool AsyncStaticWebHandler::_searchFile(AsyncWebServerRequest *request, const St char *_tempPath = (char *)malloc(pathLen + 1); if (_tempPath == NULL) { async_ws_log_e("Failed to allocate"); - request->abort(); + // abort() now triggers _onDisconnect() → delete request (it no longer + // just sends an RST), so we must close the file BEFORE aborting to avoid + // a use-after-free on the destroyed request object. request->_tempFile.close(); + request->abort(); return false; } snprintf_P(_tempPath, pathLen + 1, PSTR("%s"), path.c_str()); diff --git a/src/WebRequest.cpp b/src/WebRequest.cpp index a3ea772b..00327e7e 100644 --- a/src/WebRequest.cpp +++ b/src/WebRequest.cpp @@ -1067,7 +1067,18 @@ void AsyncWebServerRequest::abort() { _paused = false; _this.reset(); // async_ws_log_e("AsyncWebServerRequest::abort"); + // abort() sends a TCP RST (immediate reset, no TIME_WAIT) and frees the lwIP + // pcb — the desired behavior for protocol violations. However, abort() does + // NOT fire the onDisconnect (_discard_cb) callback on any platform (ESP32 + // AsyncTCP, ESP8266 ESPAsyncTCP, RPAsyncTCP), so the AsyncWebServerRequest + + // AsyncClient + _response + _tempObject + _tempFile + _itemBuffer would + // leak permanently. Manually trigger _onDisconnect() to run the user + // callback and _server->_handleDisconnect(this) → delete this → ~AsyncWebServerRequest + // → delete _client (pcb is already null from abort, so ~AsyncClient is a no-op). + // Safe because all call sites return immediately after abort() — no member + // is accessed after _onDisconnect() destroys *this. _client->abort(); + _onDisconnect(); } }