Handle edge-case in which we receive a partial WS header#953
Handle edge-case in which we receive a partial WS header#953depau wants to merge 1 commit intome-no-dev:masterfrom
Conversation
|
Thanks for fixing this. I also ran into the problem and was about to fix it. Yours is much cleaner. Consider making the header buffer just a static array instead of malloc() and free() ing it. It's only 16 bytes. |
|
I reworked this patch so that the partial header is stored in the Since I no longer need to While at it I added extensive comments to explain what is going on, since this probably will take 1h to read to anyone trying to understand what's going on. The actual parsing definitely needs explaining since it took me way more than 1h to understand what is going on when I first tried to fix this issue, but that is out of the scope of this pull request. |
Since in my use-case I use this library to send and receive lots of data at high rates I spotted a quite infrequent bug that does not occur with occasional sending/receiving.
The issue occurs when receiving lots of data.
Since WebSocket and TCP framing are independent it occasionally happens that the buffer passed to
AsyncWebSocketClient::_onDatais truncated. At the current state the library handles the case in which the truncation happens mid-payload.However, truncation may happen anywhere and it occasionally happens in the WS frame header too. This happens extremely rarely in practice, since most ESP applications predominantly send data - not receive it, and since if data is sent one frame at a time the frames are unlikely to be truncated, and if it happens it happens mid payload.
Sending lots of data increases the probability by a lot, since the frames will most likely be delivered one after the other in one big buffer; there is going to be a lot more headers and as a consequence more places where this bug could occur.
The fix consists in storing a partial header in the heap temporarily, then merging the remaining chunk later once it is received.
In order to do so, what used to be
uint8_t *fdata(which I renamed toheaderBuffor better readability) can now be either a pointer to an offset inside the input buffer or a heap-allocated buffer. The buffer is always allocated to 16 bytes, which I determined is the maximum, worst-case scenario size of the WS frame header.The header parsing part of the code now performs the following operations (I'd like to explain the logic since the code is complex):
!_pstate(== if we're not waiting for the remaining part of a frame truncated mid-payload)headerBufto the current position in the input bufferYou will notice I had to use a bunch of
gotos to handle the "failure - truncated header" scenario. It could have been a try-catch-finally, but as far as I know ESP8266 binaries are built with-fno-exceptions.This code, combined with my other PR #952 was tested with Valgrind and I can claim it works very well: