Skip to content
This repository was archived by the owner on Jan 20, 2025. It is now read-only.

Conversation

@depau
Copy link

@depau depau commented Mar 10, 2021

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::_onData is 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 to headerBuf for 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):

  • If !_pstate (== if we're not waiting for the remaining part of a frame truncated mid-payload)
    • Set headerBuf to the current position in the input buffer
    • If we have a previous header stored in the heap
      • Fill the rest of it with data from the input buffer
      • Set headerBuf to this buffer
      • Clear class attribute reference to the stored header
    • Continue parsing the header the same as before, but if at any step we run out of header bytes, we jump to a handler
    • If we ran out of header bytes
      • malloc a new 16 bytes buffer
      • memcpy the current header data into it
      • Set class attribute stored header references to point to this buffer
    • If headerBuf was allocated in heap
      • Free it
    • If we ran out of header bytes
      • return

You 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:

image
image

@grm1209
Copy link

grm1209 commented Apr 9, 2021

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.

@depau
Copy link
Author

depau commented Aug 1, 2021

I reworked this patch so that the partial header is stored in the stack class instance.

Since I no longer need to free() the buffer I was also able to (slightly) simplify the control flow.

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants