Skip to content

Fix #384: Remove the buffer overflow intended by design allowing users to add a null terminator after the buffer end #385

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

Open
mathieucarbou wants to merge 1 commit into
base: main
Choose a base branch
from
+55 −16
Open

Fix #384: Remove the buffer overflow intended by design allowing users to add a null terminator after the buffer end #385

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
8 changes: 4 additions & 4 deletions examples/WebSocket/WebSocket.ino
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,9 +76,11 @@ void setup() {
// Run in terminal 2: websocat ws://192.168.4.1/ws => should stream data
// Run in terminal 3: websocat ws://192.168.4.1/ws => should fail:
//
// To send a message to the WebSocket server:
// To send a message to the WebSocket server (\n at the end):
// > echo "Hello!" | websocat ws://192.168.4.1/ws
//
// echo "Hello!" | websocat ws://192.168.4.1/ws
// Generates 2001 characters (\n at the end) to cause a fragmentation (over TCP MSS):
// > openssl rand -hex 1000 | websocat ws://192.168.4.1/ws
//
ws.onEvent([](AsyncWebSocket *server, AsyncWebSocketClient *client, AwsEventType type, void *arg, uint8_t *data, size_t len) {
(void)len;
Expand Down Expand Up @@ -108,7 +110,6 @@ void setup() {
// 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();
}
Expand All @@ -130,7 +131,6 @@ void setup() {
);

if (info->message_opcode == WS_TEXT) {
data[len] = 0;
Serial.printf("%s\n", (char *)data);
} else {
for (size_t i = 0; i < len; i++) {
Expand Down
1 change: 0 additions & 1 deletion idf_component_examples/websocket/main/main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,6 @@ void setup() {
String msg = "";
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);
}
}
Expand Down
59 changes: 51 additions & 8 deletions src/AsyncWebSocket.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -586,7 +586,6 @@ void AsyncWebSocketClient::_onData(void *pbuf, size_t plen) {
}

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

if (_pinfo.masked) {
for (size_t i = 0; i < datalen; i++) {
Expand All @@ -606,7 +605,31 @@ void AsyncWebSocketClient::_onData(void *pbuf, size_t plen) {
}

if (datalen > 0) {
_server->_handleEvent(this, WS_EVT_DATA, (void *)&_pinfo, data, datalen);
// ------------------------------------------------------------
// Issue 384: https://github.com/ESP32Async/ESPAsyncWebServer/issues/384
// Discussion: https://github.com/ESP32Async/ESPAsyncWebServer/pull/383#discussion_r2760425739
// The initial design of the library was doing a backup of the byte following the data buffer because the client code
// was allowed and documented to do something like data[len] = 0; to facilitate null-terminated string handling.
// This was a bit hacky but it was working and it was documented, although completely incorrect because it was modifying a byte outside of the data buffer.
// So to fix this behavior and to avoid breaking existing client code that may be relying on this behavior, we now have to copy the data to a temporary buffer that has an extra byte for the null terminator.
// ------------------------------------------------------------
uint8_t *copy = (uint8_t *)malloc(datalen + 1);

if (copy == NULL) {
async_ws_log_e("Failed to allocate");
_status = WS_DISCONNECTED;
if (_client) {
_client->abort();
}
return;
}

memcpy(copy, data, datalen);
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.

Note: I decided to always do the copy regardless of the frame type (WS_TEXT, WS_BINARY, WS_CONTINUATION) because only the first frame holds the info WS_TEXT, WS_BINARY. Others are continuation frame.

So this is more correct by the spec since when in next frames we do not have the information about the message type, but we could decide to optimize and only do the copy when we encounter a WS_TEXT frame, because a frame max length is 2^63 so this is not likely that we would encounter continuation frames on a MCU...

I am opened to both solutions.

copy[datalen] = 0;

_server->_handleEvent(this, WS_EVT_DATA, (void *)&_pinfo, copy, datalen);

free(copy);
Comment on lines +615 to +632
Copy link

Copilot AI Feb 8, 2026

Choose a reason for hiding this comment

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

This allocates and frees a new buffer for every WS_EVT_DATA callback (including binary frames and each fragment). On embedded targets this can significantly increase heap churn/fragmentation and can turn high-throughput/binary traffic into disconnects due to OOM. Consider limiting the copy+NUL terminator behavior to text messages only (e.g., when frame/message opcode is WS_TEXT) and otherwise call the handler with the original data pointer/datalen, or reuse a per-client scratch buffer to avoid repeated malloc/free.

Suggested change
// ------------------------------------------------------------
uint8_t *copy = (uint8_t *)malloc(datalen + 1);
if (copy == NULL) {
async_ws_log_e("Failed to allocate");
_status = WS_DISCONNECTED;
if (_client) {
_client->abort();
}
return;
}
memcpy(copy, data, datalen);
copy[datalen] = 0;
_server->_handleEvent(this, WS_EVT_DATA, (void *)&_pinfo, copy, datalen);
free(copy);
// For non-text messages, however, we can avoid the extra allocation and pass the original data buffer directly.
// ------------------------------------------------------------
uint8_t *copy = nullptr;
void *eventData = (void *)data;
// Only allocate a NUL-terminated copy for text messages to preserve legacy behavior.
if (_pinfo.message_opcode == WS_TEXT) {
copy = (uint8_t *)malloc(datalen + 1);
if (copy == NULL) {
async_ws_log_e("Failed to allocate");
_status = WS_DISCONNECTED;
if (_client) {
_client->abort();
}
return;
}
memcpy(copy, data, datalen);
copy[datalen] = 0;
eventData = copy;
}
_server->_handleEvent(this, WS_EVT_DATA, (void *)&_pinfo, eventData, datalen);
if (copy != nullptr) {
free(copy);
}

Copilot uses AI. Check for mistakes.
Copy link
Member Author

@mathieucarbou mathieucarbou Feb 10, 2026

Choose a reason for hiding this comment

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

Si my comment above (#385 (comment)).
This would be an optimization, yes, but we won't be able to know for the next frames (WS_CONTINUATION) if we have to copy or not since we would have lost at that time the info about te previous frame type.

}

// track index for next fragment
Expand Down Expand Up @@ -652,12 +675,37 @@ void AsyncWebSocketClient::_onData(void *pbuf, size_t plen) {

} 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);

// ------------------------------------------------------------
// Issue 384: https://github.com/ESP32Async/ESPAsyncWebServer/issues/384
// Discussion: https://github.com/ESP32Async/ESPAsyncWebServer/pull/383#discussion_r2760425739
// The initial design of the library was doing a backup of the byte following the data buffer because the client code
// was allowed and documented to do something like data[len] = 0; to facilitate null-terminated string handling.
// This was a bit hacky but it was working and it was documented, although completely incorrect because it was modifying a byte outside of the data buffer.
// So to fix this behavior and to avoid breaking existing client code that may be relying on this behavior, we now have to copy the data to a temporary buffer that has an extra byte for the null terminator.
// ------------------------------------------------------------
uint8_t *copy = (uint8_t *)malloc(datalen + 1);

if (copy == NULL) {
async_ws_log_e("Failed to allocate");
_status = WS_DISCONNECTED;
if (_client) {
_client->abort();
}
return;
}

memcpy(copy, data, datalen);
copy[datalen] = 0;

_server->_handleEvent(this, WS_EVT_DATA, (void *)&_pinfo, copy, datalen);
if (_pinfo.final) {
_pinfo.num = 0;
} else {
_pinfo.num += 1;
}

free(copy);
Comment on lines +687 to +708
Copy link

Copilot AI Feb 8, 2026

Choose a reason for hiding this comment

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

The copy+NUL-termination logic (and the long explanatory comment) is duplicated in two branches. This makes future fixes easy to miss and increases maintenance cost. Consider extracting this into a small helper (e.g., a local lambda that returns a temporary buffer or invokes the handler) so the behavior stays consistent across fragmented/non-fragmented paths.

Suggested change
uint8_t *copy = (uint8_t *)malloc(datalen + 1);
if (copy == NULL) {
async_ws_log_e("Failed to allocate");
_status = WS_DISCONNECTED;
if (_client) {
_client->abort();
}
return;
}
memcpy(copy, data, datalen);
copy[datalen] = 0;
_server->_handleEvent(this, WS_EVT_DATA, (void *)&_pinfo, copy, datalen);
if (_pinfo.final) {
_pinfo.num = 0;
} else {
_pinfo.num += 1;
}
free(copy);
auto handleDataFramePayload = [this](const uint8_t *payload, size_t payloadLen) -> bool {
uint8_t *copy = static_cast<uint8_t *>(malloc(payloadLen + 1));
if (copy == nullptr) {
async_ws_log_e("Failed to allocate");
_status = WS_DISCONNECTED;
if (_client) {
_client->abort();
}
return false;
}
memcpy(copy, payload, payloadLen);
copy[payloadLen] = 0;
_server->_handleEvent(this, WS_EVT_DATA, (void *)&_pinfo, copy, payloadLen);
free(copy);
return true;
};
if (!handleDataFramePayload(data, datalen)) {
return;
}
if (_pinfo.final) {
_pinfo.num = 0;
} else {
_pinfo.num += 1;
}

Copilot uses AI. Check for mistakes.
Copy link
Member Author

@mathieucarbou mathieucarbou Feb 10, 2026

Choose a reason for hiding this comment

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

Si my comment above (#385 (comment)).
This would be an optimization, yes, but we won't be able to know for the next frames (WS_CONTINUATION) if we have to copy or not since we would have lost at that time the info about te previous frame type.

}

} else {
Expand All @@ -674,11 +722,6 @@ void AsyncWebSocketClient::_onData(void *pbuf, size_t plen) {
break;
}

// restore byte as _handleEvent may have added a null terminator i.e., data[len] = 0;
if (datalen) {
data[datalen] = datalast;
}

data += datalen;
plen -= datalen;
}
Expand Down
3 changes: 0 additions & 3 deletions src/AsyncWebSocket.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -554,9 +554,6 @@ class AsyncWebSocketMessageHandler {
}
} else if (type == WS_EVT_DATA) {
AwsFrameInfo *info = (AwsFrameInfo *)arg;
if (info->opcode == WS_TEXT) {
data[len] = 0;
}
if (info->final && info->index == 0 && info->len == len) {
if (_onMessage) {
_onMessage(server, client, data, len);
Expand Down