fix(ws): Fully handle websocket frame tearing (closes #453) - #462
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.
|
@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 |
…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.
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); | ||
|
|
There was a problem hiding this comment.
@willmmiles : I think this one is valid ? Not harmful but this would be a defensive fix.
There was a problem hiding this comment.
Yup, it's technically correct in the most annoying of ways (avoiding UB means making something "simple" into way too many lines of code :( )
| // 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); | ||
| } |
There was a problem hiding this comment.
@willmmiles : should document the invariant that zero-length frames are never masked ? Or fix like suggested ?
There was a problem hiding this comment.
I think Copilot is correct here, I'll fix it. I had Claude do the porting work from my old branch, possibly this was a pre-existing bug in main or my previous work that it brought over.
|
@willmmiles : is this PR ready to be reviewed and merged ? I see some copilot comments still not resolved. Also, FYI, I did some deeper testing with WebSocket (in main) and saw that we really have a lot of other issues in main such as:
... which leads to browser disconnection die to invalid frames upon sending big fragmented messages
I saw that this PR rewrites all the |
There was a problem hiding this comment.
@willmmiles : I tested this PR and all the issues I saw in main are gone.
I will let you review the copilot comments first before merging.
Thanks!
|
Another AI reviewer I've been using locally pointed out that that the combination of (mask + a shared buffer) is not correct in this implementation -- it applies the mask in place, mutating the shared space. Will fix ASAP. |
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.
If the header flags call for it, it needs to be there.
1254fc7 to
a3b0e4c
Compare
| const size_t remaining = target.size() - _sent; | ||
| _framePayloadLen = std::min(remaining, window); |
There was a problem hiding this comment.
This is not a new issue in this PR, we have never supported large frames. I don't think clamping the size is a valid fix; even if the framing is correct. The user's data stream is still broken.
I'm inclined to log a feature request rather than hold up this PR for a feature that never existed.
There was a problem hiding this comment.
Frame size can be quite huge indeed - I don’t see what could be a use case. I saw we have this limitation indeed. Supporting fragmentation and defragmentation is required yes.
but why wouldn’t we accept copilot suggestion? As I understand it, it is enforces a max frame length which is consistent with the max length that the esp can send and which is reported as payload Len right ?
or, should we log a warning in case we reach the limit and we put a hard cap to avoid sending more data than we can set in the payload Len ?
There was a problem hiding this comment.
Right, frames are already fragmented based on c->space() -- this will only matter on a device with TCP windows >64kb. I don't think we support any such devices (yet)?
but why wouldn’t we accept copilot suggestion?
Because it doesn't fix anything, it just moves the brokenness around. If a large-window-device exists, and someone tries to send a large packet, then current code will cause a protocol error -- resulting in a dropped connection and an obvious fault. Applying copilot's suggestion will instead truncate the output data - valid frames will arrive, but user data is discarded, making a debugging challenge. The worst case is fragmentation with copilot's suggestion, eg. sending a 1MB message with 128kb window: chunks of data will simply be missing in the middle of the reassembled frame.
we log a warning in case we reach the limit and we put a hard cap to avoid sending more data than we can set in the payload Len ?
We could, but it would probably be less work to support the 128-bit frame size format.
There was a problem hiding this comment.
I see! Thanks for the explanation!
There was a problem hiding this comment.
Applying copilot's suggestion will instead truncate the output data - valid frames will arrive, but user data is discarded, making a debugging challenge. The worst case is fragmentation with copilot's suggestion, eg. sending a 1MB message with 128kb window: chunks of data will simply be missing in the middle of the reassembled frame.
Arg, sorry, I'm wrong about this - the frame segmentation layer will work with the suggested patch, but only for non-control messages. So the patch does fix one case. Large control messages would still be misframed, though.
There was a problem hiding this comment.
I though control frames were small ?
There was a problem hiding this comment.
All of the common use cases are, but I don't think there's a blanket prohibition in the protocol specification. Ping frames can be arbitrarily large.
| _maskKey[0] = rand() % 0xFF; // NOLINT(runtime/threadsafe_fn) | ||
| _maskKey[1] = rand() % 0xFF; // NOLINT(runtime/threadsafe_fn) | ||
| _maskKey[2] = rand() % 0xFF; // NOLINT(runtime/threadsafe_fn) | ||
| _maskKey[3] = rand() % 0xFF; // NOLINT(runtime/threadsafe_fn) |
There was a problem hiding this comment.
I spotted this one as well. I don't know why four calls to rand() were made. I left it alone cf. Chesterton's fence, I presume the original author had some reason.
There was a problem hiding this comment.
Remember the perf issue we fixed in asynctcp calling Rand : if we can avoid consecutive calls to Rand this would be better to avoid potential busy waits right ?
There was a problem hiding this comment.
Yes, exactly -- that's why I was reluctant to change it here. It seemed so obviously weird that I took that to be stronger evidence of intentionality. (Perhaps 0xff is an invalid mask character? I don't know.)
There was a problem hiding this comment.
Commit fc5296d: I found this useful during testing, but it had an interesting interaction with the fragmentation code. When the large packet is sent, it's fragmented. The fragmentation logic takes the available TCP window and subtracts 8 bytes, enough for the largest possible header (ie. including a mask). But we don't send a mask: this leaves 4 bytes in the TCP window, which is enough to fit the first 0-byte ping from pingAll().
Adding the timestamp no longer fits in that window, so it adds a minimum of 2 RTTs to collect the ping response. (We have to get at least one ack back before the remainder of the control packet can be sent.) This results in some fairly large round trip delays.
I found it useful but we do not need to accept this commit upstream.
There was a problem hiding this comment.
That’s fine let’s leave it.
We cannot optimize more anywhere since the ping data part is user controled so if larger it could even be 3 rtts right ?
There was a problem hiding this comment.
Commit a3b0e4c: This is also somewhat dubious -- while it's a useful test for a key property (the shared data is not modified by the library), the specific test approach is a standards violation. I've separated this in to an isolated commit so we can drop it if we don't want to keep it.
There was a problem hiding this comment.
Same, let’s leave it. Anything that can help us test is good I think. Maybe one day we can think about separating examples from tests and make tests a little bit clever so that we could eventually partly automate them.
There was a problem hiding this comment.
Commit 30cdb53b: This approach could be extended further upwards through the API: we could define a typedef AsyncWebSocketConstBuffer = std::shared_ptr<const std::vector<uint8_t>> that allows the compiler to validate things on our behalf all the way up to the user API.
Taken to the limit, though, the catch is that it makes the user API awkward: one is required to populate a AsyncWebSocketSharedBuffer, but then pass it to an API that accepts AsyncWebSocketConstBuffer. The shared-ptrs are freely castable, so the compiler won't complain, but the function signature no longer reflects the expected usage. Continuing to expose an AsyncWebSocketSharedBuffer API the implicit contract that the library is allowed to mutate the data, ie. it's not const when passed in.
Finally, a clearly const API would also beg the question as to why we have to copy the data to a std::vector heap buffer at all: -- some use cases might be plausably sending static content. In the WLED fork I'd opted to use a custom buffer type so I could handle the "maybe-owned" case. std::shared_ptr<> gets us most of the way as we can specify a custom deleter, but there's no way to wrap a static buffer in std::vector<>.
Future work, perhaps!
There was a problem hiding this comment.
Yes indeed! And more generally anything else that improves and can help you adop the lib in Wled.
For example I was thinking about the firmware size: if too big, maybe have a way to awitch some features off through macros like middleware, request continuation, etc.
There was a problem hiding this comment.
The biggest challenge is ESP8266 support which has both very little RAM (so OOM events are likely); and no support for exceptions, so anything that depends on std::bad_alloc, such as std::vector, std::deque, std::shared_ptr, etc., is unsafe under memory pressure on that platform. So to be reliable, I'd basically need to replace all these std structures with exception-safe alternatives.
Still, it's on my radar for the 17.x line -- these improvements in websocket safety alone will make it worth considering.
|
@willmmiles PR is ready for test / review / merge ? |
I tested the latest changes with the WS example and also with a new ai-generated example I pushed in PR #467. All good for me. I am ready to merge if you are finished.
|
|
I've go no further work planned in this PR. Let's do it! |


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.