Websocket incoming frame parsing fixes

[willmmiles]

Fix handling of incoming frame headers that span multiple packets, a corner case processing very large frames, and an edge case handling disconnect frames that are split between packets.

Filed as draft for these open concerns:

• I am concerned about the performance of the state machine compared to the original implementation. It should be very robust but won't be anywhere near as fast. It does have the advantage of handling any and all tearing cases, though. (The verbose log message on every header byte is also maybe a bit much, but it was very helpful in validating correct behavior.)
• Safari missing-mask-on-disconnect bug handling still needs testing. (I don't own any Apple devices.)
• The very large frame handling fix still needs testing.
• Torn close reason fix still needs testing.

[willmmiles]

How do you folks feel about the state machine parser? Is the robustness worth the performance cost, or should we instead try buffering the header (spends more RAM and/or we're playing union tricks to overlay things in different parse states), or should we drop the connection on torn headers (safe but unfriendly)?

I don't have a good way to quantify the performance either, all of our benchmarks so far have focused on outgoing frames.

[mathieucarbou]

How do you folks feel about the state machine parser? Is the robustness worth the performance cost, or should we instead try buffering the header (spends more RAM and/or we're playing union tricks to overlay things in different parse states), or should we drop the connection on torn headers (safe but unfriendly)?

I don't have a good way to quantify the performance either, all of our benchmarks so far have focused on outgoing frames.

That's a tricky question!
I would say, let's test that and if the perf drop si reasonable that's fine IMO and brings a lot of improvements and stability.
I definitely prefer a lower heap / stack size (or at least constant).
I know some people having heavy websocket usage but more on the sending side indeed. And we never really tested how fast we parse incoming frames.

I think we could test that with websocat.

[willmmiles]

I'm working on some test cases for the fixes. Claude and I have found that the last fix (close reason handling) isn't really sufficient - we will have to defragment control frames for standards compliant operation. Stand by for more code.

[willmmiles]

I read recently that modern AI is really, really good at finding all the bugs you ask it for. This is definitely turning out to be my experience here! I'm trying to validate the close-on-error semantics and it's turning in to a rabbit hole.

There's a pernicious corner case with AsyncTCP where, should a client wish to destruct the AsyncClient from the onData callback, it'll get itself in to trouble with the ack handling. Calling ackLater() is no help: the object holding _ack_pcb might have been destructed at the point where it would be read back, so it causes a use-after-free adding bytes to _rx_ack_len. Not calling ackLater() still causes a problem where _pcb is used after free.

... and there's more: in most cases where we close as a result of the client asking (like AsyncWebSocket), we must ack the bytes read before closing. Otherwise the TCP stack generates a RST-close instead of a FIN-close, as is required by the TCP protocol, to indicate that the remote client did not in fact consume all bytes. Even if it was safe to call close(), AsyncTCP doesn't provide a way for us to ack a packet prior to the onData() returning, so there's no legal way to indicate how many bytes were accepted so we can generate the correct close. :(

I'm going to think about this one a bit -- wanted to share where it's at, though.

[mathieucarbou]

I'm going to think about this one a bit -- wanted to share where it's at, though.

it's like we need a "deferred" close ?

[willmmiles]

I'm going to think about this one a bit -- wanted to share where it's at, though.

it's like we need a "deferred" close ?

Yup, that's one approach. Basically the solution space breaks down to:

• Formally guarantee close() in callbacks is safe, and fully specify the ack semantics if that's done in a recv callback
  • Implementing this will require orchestrating some memory that has a different life cycle than the client object itself
• A deferred close API, which can put the AsyncClient in a "close pending" state where it'll behave as if closed, but the LwIP connection isn't torn down until the callback completes.
  • Lots and lots of bookkeeping and checking in the AsyncTCP layer
• Changing the onData callback API to allow a return value that says "please close the connection for me"
• A full on object life API redesign without bare pointers everywhere (ideal in many ways but very very breaking).

Lots to think about.

[willmmiles]

• Formally guarantee close() in callbacks is safe, and fully specify the ack semantics if that's done in a recv callback
  • Implementing this will require orchestrating some memory that has a different life cycle than the client object itself

ESP32Async/AsyncTCP#124 implents this solution - it adds that guarantee that it's safe to close() in any callback. (The callback std::function objects themselves are even held in scope so we don't hit any UB.) After testing a couple of different options, it ultimately wasn't that difficult to implement and seemed like the best approach. (Of course I found more bugs there along the way.. but that's another story.)

I've pushed one more commit here that reorders things so it's "as safe as possible" with older AsyncTCP. This is as good as it'll get, I think.

[macdylan]

Reporting back with the real-Safari disconnect testing you flagged as untestable — done against websocket-parsing @ ddf5ce9 (ESP32-C3 bench, AsyncTCP 3.5.0), serial diagnostics captured throughout.

Three disconnect paths from Safari (macOS, same machine the WS server sees):

Path	Server-side behavior
tab close	no immediate close frame from Safari; server reaped the dead connection via pull-idle (30.2 s) at the next connection event, clean close (id=1, pull idle=30207ms) diagnostic, no zombie left behind
Wi-Fi off ~10 s → back on	new connection on reconnect, stale one reaped at that moment (pull idle=10147ms), page data resumed
lock screen / background → wake	the WS connection survived — same client id kept pulling telemetry after wake, no reconnect needed

Across the whole session: zero panics, zero reboots, zero error lines, no dropped-frame accounting anomalies, heartbeat cadence continuous. Heap floor during the Safari load + reconnect burst was ~7.9 KB with all responses completing.

Honest caveat: this is black-box validation — I did not capture the wire bytes, so I cannot prove the specific "missing mask on close frame" Safari quirk actually fired; what I can say is that repeated real-Safari disconnects (all three styles, several rounds) never upset the parser. If you want wire-level evidence of that exact frame shape, I can arrange a capture.

Combined with the earlier torn-frame injection suite (64 injections across header/mask/payload/close tears + fuzz, all clean, connection survives torn headers and keeps answering — reported in #481), our side has nothing blocking this PR.

[willmmiles]

Honest caveat: this is black-box validation — I did not capture the wire bytes, so I cannot prove the specific "missing mask on close frame" Safari quirk actually fired; what I can say is that repeated real-Safari disconnects (all three styles, several rounds) never upset the parser. If you want wire-level evidence of that exact frame shape, I can arrange a capture.

Thanks, that's still very helpful. As part of the patch development process my harness and I ended up building a byte-by-byte socket test sequence for validating that the code works as designed, but I don't have any Apple devices on hand to see what any particular version of Safari actually sends. Thanks for giving it a try and providing feedback.