fix(ws): Fully handle websocket frame tearing (closes #453)#462
fix(ws): Fully handle websocket frame tearing (closes #453)#462willmmiles wants to merge 6 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Refactors WebSocket send-path handling to safely resume after partial/tearing AsyncClient::add() commits, prioritizing control frames and reducing per-message send bookkeeping by moving in-flight state onto AsyncWebSocketClient.
Changes:
- Replaces per-message ack/send state with per-client in-flight frame tracking to support partial frame commits and resumption.
- Folds control-frame handling into
AsyncWebSocketMessageand prioritizes control frames over data frames in the unified queue runner. - Expands PONG handling to forward payload data (example updated to measure ping RTT).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/AsyncWebSocket.h | Removes AsyncWebSocketControl/message status state; introduces per-client in-flight frame tracking fields and new _runQueue(lock) contract. |
| src/AsyncWebSocket.cpp | Implements resumable frame commit logic (webSocketAddFrame) and unified queue runner; adjusts ack/poll behavior and PONG payload forwarding. |
| examples/arduino/WebSocket/WebSocket.ino | Updates example to send ping timestamps and print RTT on pong. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const size_t frameLen = webSocketFrameHeaderLen(_framePayloadLen, target.mask()) + _framePayloadLen; | ||
| if (_frameSent < frameLen) { | ||
| if (added == 0) { | ||
| return; // stalled; resume on the next trigger | ||
| } | ||
| continue; // made some progress -- keep pushing while there's headway | ||
| } |
| AsyncWebSocketSharedBuffer buffer = len ? makeSharedBuffer(data, len) : std::make_shared<std::vector<uint8_t>>(); | ||
| _controlQueue.emplace_back(buffer, opcode, len && mask); |
|
@willmmiles : do you mind if we merge #454 first ? #454 focuses on fixing the bug about torn frames, and does not include a huge refactoring, it is tested and can be merged in main. This PR is a bigger refactoring so it would probably need more testing and code review before merging. There is no point in delaying the above fix. What I would like ideally is:
I would like to separate the fixes (delivered in a next release) from a potential more risky refactoring, that could sit in main for a longer time. Thanks ! |
|
#454 has outstanding bugs - sorry I haven't completed a full review, I've been focusing on getting this finished instead. I can do that but it feels like a waste of time vs testing a better solution. |
But existing ones or bugs created by the fix ? If created by the fix then yes we cannot merge #454. I just wanted to make sure we go on the right path: either we merge #454 for the fix and refactor after, or #454 introduces more issues (which ones ?) in that case we have to close it and focus on your PR. #454 was tested so we know it fixes the torn frame issue. |
I posted the full review. One new bug (although arguably it's introduced by AsyncTCP 3.5.0, not the new code); one questionable case; and the usual slew of AI code quality issues. |
|
FTR, I would also like to note that Copilot's suggestions here are both good ones and I'll address them as soon as I can. |
You'r right for the |
mathieucarbou
added
Type: Bug
…syncTCP 3.5.0) AsyncTCP 3.5.0 (commit ESP32Async/AsyncTCP@beb0e95) changed abort() to fire the error and disconnect (discard) callbacks synchronously instead of deferring them to the async event queue: int8_t AsyncClient::abort() { int8_t err = _tcp_abort(&_pcb, this); if (err != ERR_CONN) { _error(ERR_ABRT); // <-- now synchronous: calls onError + onDisconnect } return err; } close() has the same behaviour — it calls _discard_cb inline when the pcb is closed successfully. This breaks ESPAsyncWebServer's HTTP request path. When abort() (or close() from a response) is called from inside a TCP callback (e.g. _onData -> _parseLine -> abort()), the synchronous _onDisconnect() runs _server->_handleDisconnect(this) -> delete this -> ~AsyncWebServerRequest() -> delete _client -> ~AsyncClient(), destroying the AsyncClient while it is still on the call stack inside _recv()/_sent()/_poll(). After the callback returns, AsyncTCP accesses the freed client (use-after-free), corrupting the heap. Over time this can cause LwIP's tcp_accept() to receive a NULL pcb: [E][AsyncTCP.cpp:1572] tcp_accept(): _accept failed: pcb is NULL All ~15 abort() call sites in WebRequest.cpp (SSL detection, null bytes in headers, allocation failures, multipart/chunked parse errors, boundary validation, auth header allocation), plus close() calls in _onAck/_onTimeout and response write paths, are re-entrancy hazards. Fix: defer `delete this` when _onDisconnect() runs re-entrantly. - Added two flags to AsyncWebServerRequest: _inCallback — set by each top-level TCP callback _disconnectPending — set by _onDisconnect() when it cannot delete yet - _onDisconnect(): if _inCallback is true, sets _disconnectPending and returns without deleting; the AsyncClient stays alive until the stack unwinds. If _inCallback is false (normal async disconnect), it deletes immediately as before. - New helper _onTcpCallbackExit() consolidates the deferred-delete check at every exit point of _onData, _onAck, _onPoll, _onTimeout: it clears _inCallback, performs `delete this` via _handleDisconnect() if pending, and returns true so the caller can `return` without touching `this`. - Parse functions (_onData body loop, _parseLine, _parseChunkedBytes, _parseMultipartPostByte) check _disconnectPending and break/return before accessing members after a nested abort(). - Fixed requestAuthentication(): two paths did `abort(); break; send(r);` which accessed `this` after deletion and leaked the response. Now `delete r; abort(); return;`. The WebSocket side has the same class of issue and is addressed separately in PR #462. Builds: latest-asynctcp (AsyncTCP 3.5.0), arduino-3, esp8266.
Control frames (ping/pong/close) and data frames (text/binary) were two separate types with duplicated send logic. Merging them onto one type is prep for making the send path resumable/resilient to add() failures uniformly for both, instead of only for data messages. No behavior change: control frames still send unfragmented in one shot and are marked finished before their send is verified, same as before.
AsyncClient::space() reflects TCP window room, not whether lwIP can allocate a pbuf for the write -- add() can still fail even when space() said there was room. The old webSocketSendFrame() issued two add() calls per frame (header, then payload) and assumed each fully succeeded or fully failed; if the header committed but the payload didn't, the next retry generated a brand new header for the same bytes, corrupting the stream with a duplicate/orphaned header. Control frames had no retry at all -- send() marked them finished before checking whether anything was actually sent, silently dropping pings/pongs/closes on failure. webSocketAddFrame() replaces webSocketSendFrame(): it builds the frame header into a small stack buffer (no more per-frame malloc, closing a latent header-buffer-lifetime hazard on backends that default add() to zero-copy) and submits only whatever byte range of header+payload hasn't been committed yet, tracking real progress via `frameSent` rather than assuming add() is all-or-nothing (ESPAsyncTCP's SSL path can return a genuine partial count). AsyncWebSocketMessage::send() drives this to resume a frame exactly where a prior attempt left off -- mask key, frame length, and opcode are only chosen once and held fixed the instant any header byte is committed, so a retry can never re-derive a different, mismatched header for bytes already on the wire. Both control and data frames now go through the same resumable path.
… frame AsyncWebSocketMessage is now a pure payload holder (buffer + opcode + mask), with no per-instance send-progress fields. All in-flight-frame state moves to a single, flat block of scalars on AsyncWebSocketClient (_sendingControl, _sent, _maskKey, _framePayloadLen, _frameSent, _unacked) shared uniformly between control and data frames -- there's deliberately one set of these, not one per queue, enforcing that exactly one WS frame may be outstanding (added to TCP but not yet acked) at a time. Consequences of that invariant: - A message is only popped from its queue once fully sent *and* fully acked, so queueLen()/WS_MAX_QUEUED_MESSAGES need no changes -- a message in progress is still physically sitting in the deque, exactly as before. - The next queued item (control or data) is only picked up once the in-flight frame is acked, which is also what lets control frames interleave between fragments of a data message: the priority check runs at every such boundary, not just at message boundaries. - No ack-tracking FIFO is needed: since only one frame is ever outstanding, _onAck is a single clamped subtraction instead of a per-message loop -- this also fixes a latent underflow in the old control-frame ack handling (len -= head.len() was unclamped). - _clearQueue(), AsyncWebSocketMessage::ack()/send(), and AwsMessageStatus are all gone: nothing is left to sweep once queue fronts are popped directly at the point they're known to be done. Net effect is a memory-footprint reduction, not increase: the per-message fields this removes cost more (spread across up to WS_MAX_QUEUED_MESSAGES queued messages) than the one fixed block this adds to the client.
Ack-based dequeuing existed to give queueLen()/WS_MAX_QUEUED_MESSAGES real backpressure against a slow-acking peer, and to make sure a WS_DISCONNECT frame was actually delivered before tearing down the connection. Neither turns out to require tracking acked bytes: - AsyncClient::close() (not abort()) is lwIP's graceful close: it hands the pcb to lwIP, which flushes and retransmits whatever was already add()'ed -- including a just-sent close frame -- independent of the AsyncClient/AsyncWebSocketClient objects, which may already be gone by the time it's acked. Waiting for the ack before calling close() was unnecessary caution, not a correctness requirement. - Backpressure from local TCP buffering (add()/space()) is enough; nothing else in the library inspects ack completion. _runQueue() now pops a queue's front the moment its frame is fully add()'ed, and loops to immediately start the next item (control or data) instead of waiting for a trigger -- restoring the pipelining the original code had, and additionally letting a queued control frame be serviced as soon as the in-flight data fragment is locally buffered rather than after a full ack round-trip. The WS_DISCONNECT-close-on-completion check moves into that same loop. _runQueue() now takes the caller's lock by reference so it can unlock() before calling close() (which can synchronously destroy *this via _onDisconnect -> AsyncWebSocket::_handleDisconnect -> list::erase) from any of its four call sites, not just _onAck as before; _queueControl switches from lock_guard_type to unique_lock_type to support this. _unacked is gone entirely; _onAck is now just a trigger to retry _runQueue() since space() may have grown, with no accounting of its own.
bb57cb0 to
834ec81
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/AsyncWebSocket.cpp:96
- Similarly, the header builder currently sets the MASK bit / appends the masking key only when
len && mask. If this helper is meant to implement RFC 6455 framing generically, it should honormaskregardless of payload length (or the comment above should state the narrower invariant).
if (len && mask) {
hdr[1] |= 0x80;
memcpy(hdr + (headLen - 4), maskKey, 4);
}
| AsyncWebSocketMessage &target = _sendingControl ? _controlQueue.front() : _messageQueue.front(); | ||
| const bool final = _sendingControl || (_sent + _framePayloadLen == target.size()); | ||
| const uint8_t frameOpcode = (_sendingControl || _sent == 0) ? target.opcode() : (uint8_t)WS_CONTINUATION; | ||
| uint8_t *payload = target.data() + (_sendingControl ? 0 : _sent); | ||
|
|
| // wire header length for a frame with this payload length/masking -- pure | ||
| // function of RFC 6455 framing rules, cheap enough to recompute on demand | ||
| // rather than cache | ||
| static uint8_t webSocketFrameHeaderLen(size_t payloadLen, bool mask) { | ||
| return 2 + ((payloadLen && mask) ? 4 : 0) + ((payloadLen > 125) ? 2 : 0); | ||
| } |
The more-complete websocket output refactor: handle partial frame sends safely, so we can resume where we left off at the next opportunity.
There's a few key changes:
AsyncWebSocketControlwas folded in toAsyncWebSocketMessage; both performed the same fundamental task.AsyncClientobject, instead of being held on every message.