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

Merged
mathieucarbou merged 1 commit into from
Feb 12, 2026
+55 −16
Merged

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 link

@willmmiles willmmiles Feb 13, 2026

Choose a reason for hiding this comment

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

Strictly speaking, we only need to make a copy if the message is at the end of the pbuf; for messages that don't consume the entire packet, we could pre-read or null out and restore the last byte, which would be faster.

Copy link
Member Author

@mathieucarbou mathieucarbou Feb 13, 2026

Choose a reason for hiding this comment

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

Right but we don't know that right ? We only have the pointer to the buffer and the quantity that can be read.

Copy link

@willmmiles willmmiles Feb 13, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

At this point in the processing loop, we do know: if datalen == plen, we must copy to add a null terminator, otherwise it's safe to "borrow" the next byte as there's at least one byte of the next message header still in the packet buffer. (We could pass the result of that test as an argument to #388 _handleDataEvent to choose the handling algorithm.)

Copy link
Member Author

@mathieucarbou mathieucarbou Feb 13, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

@willmmiles I will try this optimization.

when the content of data frame is handled, we have: const size_t datalen = std::min((size_t)(_pinfo.len - _pinfo.index), plen);.

datalen will basically be always equal to plen, except if the network buffer contains several fragments of frames inside, in this case it will be less.

So it would be in the cases where browsers are sending several ws messages within the same paquet without waiting for a ack...

Let me add a commit in #388 and test it.

copy[datalen] = 0;

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

free(copy);
}

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

} 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