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.