[websocket] parser gets out of sync when header/size/mask not received contiguously (IDFGH-13951)

[bryghtlabs-richard]

Answers checklist.

• I have read the documentation for esp-protocols components and the issue is not addressed there.
• I have updated my esp-protocols branch (master or release) to the latest version and checked that the issue is present there.
• I have searched the issue tracker for a similar issue and not found a similar issue.

General issue report

Moving from espressif/esp-idf#14704 to here.

Under at least the following different cases, the ESP websocket parser gets out of sync with the byte stream, causing it to parse payload as headers.

1. When reading a websocket header, and the buffer layer has 1B inside it, esp_transport_read_internal() returns 1B, and ws_read_header() does not check for this, so the stream becomes misaligned by 1B. Then WS parsing is corrupt and parser cannot be realigned.
2. WS Buffer has 0B, underlying transport read returns 1B due to packet boundaries.
3. read > 125B packet, and size sub-header fields are fragmented(either due to buffer or packet boundaries).

A rough prototype fix is here: espressif/esp-idf#14706

[david-cermak]

Hi @bryghtlabs-richard

Thanks for reporting this problem and proposing the fix!
I was able to reproduce the issue simply with a unit test, see the attached.
Your patch fixes the issue, but I'd suggest revising it a bit:

• rename to ...read_exact_bytes()
• return -1 if we read less in given timeout (to indicate failure)
• remove those attempts

static int esp_transport_read_exact_size(transport_ws_t *ws, char *buffer, int requested_len, int timeout_ms)
{
    int total_read = 0;
    int len = requested_len;

    while (len > 0) {
        int bytes_read = esp_transport_read_internal(ws, buffer, len, timeout_ms);

        if (bytes_read < 0) {
            return bytes_read; // Return error from the underlying read
        }

        if (bytes_read == 0) {
            // If we read 0 bytes, we return an error, since reading exact number of bytes resulted in a timeout operation
            ESP_LOGW(TAG, "Requested to read %d, actually read %d bytes", requested_len, total_read);
            return -1;
        }

        // Update buffer and remaining length
        buffer += bytes_read;
        len -= bytes_read;
        total_read += bytes_read;

        ESP_LOGV(TAG, "Read fragment of %d bytes", bytes_read);
    }
    return total_read;
}

Here's the updated test:
feat-ws_transport-Add-unit-test-for-partial-readi.patch.txt

[bryghtlabs-richard]

Thanks @david-cermak , those updates look good to me, I'll integrate them and update my patch.

I was a little confused about where this bug lived - I had confused ESP-IDF's transport_ws.c and ESP-Protocol's esp_websocket_client in this repo. If so, I'm sorry about that, please let me know if we need to do anything to clean up the tickets/pulls.

[bryghtlabs-richard]

@david-cermak , that's a cool use of a unit-test for this! I've got your changes integrated. In the last day or two our data provider updated their data, and we can no longer reproduce on live data.