From 259c8b8f8780c40fd89c9e589f1bb01aecda2295 Mon Sep 17 00:00:00 2001 From: Mathieu Carbou Date: Sun, 8 Feb 2026 23:58:50 +0100 Subject: [PATCH] Fix #384: Remove the buffer overflow intended by design allowing users to add a null terminator after the buffer end --- examples/WebSocket/WebSocket.ino | 8 +-- .../websocket/main/main.cpp | 1 - src/AsyncWebSocket.cpp | 59 ++++++++++++++++--- src/AsyncWebSocket.h | 3 - 4 files changed, 55 insertions(+), 16 deletions(-) diff --git a/examples/WebSocket/WebSocket.ino b/examples/WebSocket/WebSocket.ino index b8ad77a7..c82f2d30 100644 --- a/examples/WebSocket/WebSocket.ino +++ b/examples/WebSocket/WebSocket.ino @@ -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; @@ -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(); } @@ -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++) { diff --git a/idf_component_examples/websocket/main/main.cpp b/idf_component_examples/websocket/main/main.cpp index 5bbde4da..fe80d888 100644 --- a/idf_component_examples/websocket/main/main.cpp +++ b/idf_component_examples/websocket/main/main.cpp @@ -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); } } diff --git a/src/AsyncWebSocket.cpp b/src/AsyncWebSocket.cpp index d65e9113..f30c79dd 100644 --- a/src/AsyncWebSocket.cpp +++ b/src/AsyncWebSocket.cpp @@ -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++) { @@ -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[datalen] = 0; + + _server->_handleEvent(this, WS_EVT_DATA, (void *)&_pinfo, copy, datalen); + + free(copy); } // track index for next fragment @@ -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 { @@ -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; } diff --git a/src/AsyncWebSocket.h b/src/AsyncWebSocket.h index 9005bfa1..2e67760f 100644 --- a/src/AsyncWebSocket.h +++ b/src/AsyncWebSocket.h @@ -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);