Skip to content

Fix broken WebSocket defragmentation #383

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mathieucarbou merged 1 commit into from
Feb 8, 2026
+116 −51
Merged

Fix broken WebSocket defragmentation #383

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 40 additions & 3 deletions examples/WebSocket/WebSocket.ino
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,14 +101,51 @@ void setup() {

} else if (type == WS_EVT_DATA) {
AwsFrameInfo *info = (AwsFrameInfo *)arg;
Serial.printf("index: %" PRIu64 ", len: %" PRIu64 ", final: %" PRIu8 ", opcode: %" PRIu8 "\n", info->index, info->len, info->final, info->opcode);
String msg = "";
Serial.printf(
"index: %" PRIu64 ", len: %" PRIu64 ", final: %" PRIu8 ", opcode: %" PRIu8 ", framelen: %d\n", info->index, info->len, info->final, info->opcode, len
);

// complete frame
if (info->final && info->index == 0 && info->len == len) {
if (info->opcode == WS_TEXT) {
data[len] = 0;
Serial.printf("ws text: %s\n", (char *)data);
client->ping();
}

} else {
// incomplete frame
if (info->index == 0) {
if (info->num == 0) {
Serial.printf(
"ws[%s][%" PRIu32 "] [%" PRIu32 "] MSG START %s\n", server->url(), client->id(), info->num, (info->message_opcode == WS_TEXT) ? "text" : "binary"
);
}
Serial.printf("ws[%s][%" PRIu32 "] [%" PRIu32 "] FRAME START len=%" PRIu64 "\n", server->url(), client->id(), info->num, info->len);
}

Serial.printf(
"ws[%s][%" PRIu32 "] [%" PRIu32 "] FRAME %s, index=%" PRIu64 ", len=%" PRIu32 "]: ", server->url(), client->id(), info->num,
(info->message_opcode == WS_TEXT) ? "text" : "binary", info->index, (uint32_t)len
);

if (info->message_opcode == WS_TEXT) {
data[len] = 0;
Serial.printf("%s\n", (char *)data);
} else {
for (size_t i = 0; i < len; i++) {
Serial.printf("%02x ", data[i]);
}
Serial.printf("\n");
}

if ((info->index + len) == info->len) {
Serial.printf("ws[%s][%" PRIu32 "] [%" PRIu32 "] FRAME END\n", server->url(), client->id(), info->num);

if (info->final) {
Serial.printf("ws[%s][%" PRIu32 "] [%" PRIu32 "] MSG END\n", server->url(), client->id(), info->num);
}
}
}
}
});
Expand All @@ -131,7 +168,7 @@ void setup() {
}

static uint32_t lastWS = 0;
static uint32_t deltaWS = 100;
static uint32_t deltaWS = 500;

static uint32_t lastHeap = 0;

Expand Down
124 changes: 76 additions & 48 deletions src/AsyncWebSocket.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,10 @@
#include <memory>
#include <utility>

#define STATE_FRAME_START 0
#define STATE_FRAME_MASK 1
#define STATE_FRAME_DATA 2

using namespace asyncsrv;

size_t webSocketSendFrameWindow(AsyncClient *client) {
Expand Down Expand Up @@ -226,8 +230,8 @@ const char *AWSC_PING_PAYLOAD = "ESPAsyncWebServer-PING";
const size_t AWSC_PING_PAYLOAD_LEN = 22;

AsyncWebSocketClient::AsyncWebSocketClient(AsyncClient *client, AsyncWebSocket *server)
: _client(client), _server(server), _clientId(_server->_getNextId()), _status(WS_CONNECTED), _pstate(0), _lastMessageTime(millis()), _keepAlivePeriod(0),
_tempObject(NULL) {
: _client(client), _server(server), _clientId(_server->_getNextId()), _status(WS_CONNECTED), _pstate(STATE_FRAME_START), _lastMessageTime(millis()),
_keepAlivePeriod(0), _tempObject(NULL) {

_client->setRxTimeout(0);
_client->onError(
Expand Down Expand Up @@ -508,8 +512,11 @@ void AsyncWebSocketClient::_onDisconnect() {
void AsyncWebSocketClient::_onData(void *pbuf, size_t plen) {
_lastMessageTime = millis();
uint8_t *data = (uint8_t *)pbuf;

while (plen > 0) {
if (!_pstate) {
async_ws_log_v("WS[%" PRIu32 "]: _onData: plen=%" PRIu32 ", _pstate=%" PRIu8 ", _status=%" PRIu8, _clientId, plen, _pstate, static_cast<uint8_t>(_status));
Copy link
Member Author

@mathieucarbou mathieucarbou Feb 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kept all logs async_ws_log_v to still be able to easily debug


if (_pstate == STATE_FRAME_START) {
const uint8_t *fdata = data;

_pinfo.index = 0;
Expand All @@ -518,13 +525,6 @@ void AsyncWebSocketClient::_onData(void *pbuf, size_t plen) {
_pinfo.masked = ((fdata[1] & 0x80) != 0) ? 1 : 0;
_pinfo.len = fdata[1] & 0x7F;

// async_ws_log_w("WS[%" PRIu32 "]: _onData: %" PRIu32, _clientId, plen);
// async_ws_log_w("WS[%" PRIu32 "]: _status = %" PRIu32, _clientId, _status);
// async_ws_log_w(
// "WS[%" PRIu32 "]: _pinfo: index: %" PRIu64 ", final: %" PRIu8 ", opcode: %" PRIu8 ", masked: %" PRIu8 ", len: %" PRIu64, _clientId, _pinfo.index,
// _pinfo.final, _pinfo.opcode, _pinfo.masked, _pinfo.len
// );

data += 2;
plen -= 2;
Copy link

@willmmiles willmmiles Feb 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Availability of these bytes is not checked - in theory plen could be 1 here. This is problematic throughout the header analysis. Not a new problem for this PR but I'd like to note it nonetheless.

Copy link
Member Author

@mathieucarbou mathieucarbou Feb 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I saw that, but we have to look at the WS spec also because these are the first 2 bytes received for the first frame: there shouldn't be any fragmentation there ?
The only case where fragmentation is happening within the assembly of the frame header is for the case of Safari which is sending the mask bytes in 2 shots.


Expand All @@ -541,47 +541,52 @@ void AsyncWebSocketClient::_onData(void *pbuf, size_t plen) {
}
}

if (_pinfo.masked) {
// Read mask bytes (may be fragmented across packets in Safari)
size_t mask_offset = 0;

// If we're resuming from a previous fragmented read, check _pinfo.index
if (_pstate == 1 && _pinfo.index < 4) {
mask_offset = _pinfo.index;
}

// Read as many mask bytes as available
while (mask_offset < 4 && plen > 0) {
_pinfo.mask[mask_offset++] = *data++;
plen--;
}

// Check if we have all 4 mask bytes
if (mask_offset < 4) {
// Incomplete mask
if (_pinfo.opcode == WS_DISCONNECT && plen == 0) {
// Safari close frame edge case: masked bit set but no mask data
// async_ws_log_w("WS[%" PRIu32 "]: close frame with incomplete mask, treating as unmasked", _clientId);
async_ws_log_v(
"WS[%" PRIu32 "]: _pinfo: index: %" PRIu64 ", final: %" PRIu8 ", opcode: %" PRIu8 ", masked: %" PRIu8 ", len: %" PRIu64, _clientId, _pinfo.index,
_pinfo.final, _pinfo.opcode, _pinfo.masked, _pinfo.len
);

// Handle fragmented mask data - Safari may split the 4-byte mask across multiple packets
// _pinfo.masked is 1 if we need to start reading mask bytes
// _pinfo.masked is 2, 3, or 4 if we have partially read the mask
// _pinfo.masked is 5 if the mask is complete
while (_pinfo.masked && _pstate <= STATE_FRAME_MASK && _pinfo.masked < 5) {
// check if we have some data
if (plen == 0) {
// Safari close frame edge case: masked bit set but no mask data
if (_pinfo.opcode == WS_DISCONNECT) {
async_ws_log_v("WS[%" PRIu32 "]: close frame with incomplete mask, treating as unmasked", _clientId);
_pinfo.masked = 0;
_pinfo.index = 0;
} else {
// Wait for more data
// async_ws_log_w("WS[%" PRIu32 "]: waiting for more mask data: read=%zu/4", _clientId, mask_offset);
_pinfo.index = mask_offset; // Save progress
_pstate = 1;
return;
_pinfo.len = 0;
Comment on lines 560 to +561
Copy link
Member Author

@mathieucarbou mathieucarbou Feb 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To go into the disconnect use case below

_pstate = STATE_FRAME_START;
break;
}
} else {
// All mask bytes received
// async_ws_log_w("WS[%" PRIu32 "]: mask complete", _clientId);
_pinfo.index = 0; // Reset index for payload processing

//wait for more data
_pstate = STATE_FRAME_MASK;
async_ws_log_v("WS[%" PRIu32 "]: waiting for more mask data: read=%" PRIu8 "/4", _clientId, _pinfo.masked - 1);
return;
}

// accumulate mask bytes
_pinfo.mask[_pinfo.masked - 1] = data[0];
data += 1;
plen -= 1;
_pinfo.masked++;
}

const size_t datalen = std::min((size_t)(_pinfo.len - _pinfo.index), plen);
const auto datalast = data[datalen];
// all mask bytes read if we were reading them
_pstate = STATE_FRAME_DATA;

// async_ws_log_w("WS[%" PRIu32 "]: _processing data: datalen=%" PRIu32 ", plen=%" PRIu32, _clientId, datalen, plen);
// restore masked to 1 for backward compatibility
if (_pinfo.masked >= 5) {
async_ws_log_v("WS[%" PRIu32 "]: mask read complete", _clientId);
_pinfo.masked = 1;
}

const size_t datalen = std::min((size_t)(_pinfo.len - _pinfo.index), plen);
const auto datalast = datalen ? data[datalen] : 0;
Copy link
Member Author

@mathieucarbou mathieucarbou Feb 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really hate this thing... users have to add \0 and overflow the data array (see WS examples). That's not normal. I don't know if there is a way to refactor that in a backward compatible way one day.

Copy link

@willmmiles willmmiles Feb 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This hack is outright wrong. Writing off the end of the pbuf pointer is just not safe in a preemptive system. I've seen crashes because the onEvent handler wrote off the end of the buffer, corrupting the heap, and then got pre-empted by another task that tried to do heap allocation before AsyncWebSocket could "undo" the damage. When I fixed the client, I still saw a crash because of the attempt to restore the byte; same setup - preempted during the onEvent callback - but the (now fixed) client didn't overwrite the value; so the heap manager wrote something new to that location, which then had one byte "restored" to an old value when AsyncWebSocket got the CPU back.

Ultimately I had to remove this from the WLED fork and we fixed all our clients to respect the buffer boundaries. Users don't have to write off the end of the array. It is true that when data arrives as a Pascal-style string (pointer+length), it cannot be used as a C-style string without a copy. But that's only necessary if you want to interact with C functions; modern tools like std::string_view can be used directly. (And if you must use a C API, std::string has a convenient pointer+length constructor for making a quick copy.)

That said: given the current example code, if we consider "onEvent handlers are permitted to write off the end of the buffer by one byte" as part of our backwards compatibilty API contract, then we really have no safe choice but to copy the data into a bigger buffer before calling the callback if the frame ends at the packet boundary.

Copy link
Member Author

@mathieucarbou mathieucarbou Feb 8, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That said: given the current example code, if we consider "onEvent handlers are permitted to write off the end of the buffer by one byte" as part of our backwards compatibilty API contract, then we really have no safe choice but to copy the data into a bigger buffer before calling the callback if the frame ends at the packet boundary.

What I thought too which is very ugly.
Let’s merge this PR I will open an issue for that. What we can do for now is to create a copy buffer but only for opcode TEXT with a \0 at the end.

Copy link
Member Author

@mathieucarbou mathieucarbou Feb 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created issue here: #384


if (_pinfo.masked) {
for (size_t i = 0; i < datalen; i++) {
Expand All @@ -590,22 +595,30 @@ void AsyncWebSocketClient::_onData(void *pbuf, size_t plen) {
}

if ((datalen + _pinfo.index) < _pinfo.len) {
_pstate = 1;
_pstate = STATE_FRAME_DATA;
async_ws_log_v("WS[%" PRIu32 "]: processing next fragment index=%" PRIu64 ", len=%" PRIu32 "", _clientId, _pinfo.index, (uint32_t)datalen);

if (_pinfo.index == 0) {
if (_pinfo.opcode) {
_pinfo.message_opcode = _pinfo.opcode;
_pinfo.num = 0;
}
}

if (datalen > 0) {
_server->_handleEvent(this, WS_EVT_DATA, (void *)&_pinfo, data, datalen);
}

// track index for next fragment
_pinfo.index += datalen;

} else if ((datalen + _pinfo.index) == _pinfo.len) {
_pstate = 0;
_pstate = STATE_FRAME_START;
async_ws_log_v("WS[%" PRIu32 "]: processing final fragment index=%" PRIu64 ", len=%" PRIu32 "", _clientId, _pinfo.index, (uint32_t)datalen);

if (_pinfo.opcode == WS_DISCONNECT) {
async_ws_log_v("WS[%" PRIu32 "]: processing disconnect", _clientId);

if (datalen) {
uint16_t reasonCode = (uint16_t)(data[0] << 8) + data[1];
char *reasonString = (char *)(data + 2);
Expand All @@ -625,24 +638,39 @@ void AsyncWebSocketClient::_onData(void *pbuf, size_t plen) {
}
_queueControl(WS_DISCONNECT, data, datalen);
}

} else if (_pinfo.opcode == WS_PING) {
async_ws_log_v("WS[%" PRIu32 "]: processing ping", _clientId);
_server->_handleEvent(this, WS_EVT_PING, NULL, NULL, 0);
_queueControl(WS_PONG, data, datalen);

} else if (_pinfo.opcode == WS_PONG) {
async_ws_log_v("WS[%" PRIu32 "]: processing pong", _clientId);
if (datalen != AWSC_PING_PAYLOAD_LEN || memcmp(AWSC_PING_PAYLOAD, data, AWSC_PING_PAYLOAD_LEN) != 0) {
_server->_handleEvent(this, WS_EVT_PONG, NULL, NULL, 0);
}

} else if (_pinfo.opcode < WS_DISCONNECT) { // continuation or text/binary frame
async_ws_log_v("WS[%" PRIu32 "]: processing data frame num=%" PRIu32 "", _clientId, _pinfo.num);
_server->_handleEvent(this, WS_EVT_DATA, (void *)&_pinfo, data, datalen);
if (_pinfo.final) {
_pinfo.num = 0;
} else {
_pinfo.num += 1;
}
}

} else {
// async_ws_log_w("frame error: len: %u, index: %llu, total: %llu\n", datalen, _pinfo.index, _pinfo.len);
// what should we do?
// unexpected frame error, close connection
_pstate = STATE_FRAME_START;

async_ws_log_v("frame error: len: %u, index: %llu, total: %llu\n", datalen, _pinfo.index, _pinfo.len);

_status = WS_DISCONNECTING;
if (_client) {
_client->ackLater();
}
_queueControl(WS_DISCONNECT, data, datalen);
Comment on lines +669 to +673
Copy link
Member Author

@mathieucarbou mathieucarbou Feb 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we receive an invalid frame, nothing were done so the connection would be left in an unknown state, including frame defragmentation... So let's force a disconnect to restore the connection.

it could also be a rogue client...

Copy link

@willmmiles willmmiles Feb 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call.

break;
}

Expand Down