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

[mathieucarbou]

Fix #384

Initial discussion: #383 (comment)

Wifi doc: https://github.com/ESP32Async/ESPAsyncWebServer/wiki#async-websocket-event

Doc requested by design that users add a null terminating char after the buffer ends

[mathieucarbou]

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.

[Copilot]

Pull request overview

Removes the previously “by design” out-of-bounds write pattern (data[len] = 0) in WebSocket receive callbacks by ensuring callbacks can still safely treat text payloads as null-terminated strings without requiring users to overflow the buffer.

Changes:

• Removed implicit data[len] = 0 behavior from AsyncWebSocketMessageHandler.
• Updated WebSocket receive path to provide a null-terminated buffer via a temporary copy instead of relying on an out-of-bounds byte.
• Updated examples to stop writing data[len] = 0 and refreshed command hints.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
src/AsyncWebSocket.h	Removes handler-side null-termination that could trigger an out-of-bounds write.
src/AsyncWebSocket.cpp	Replaces “backup/restore byte past end” with a temporary buffer copy that includes a terminator.
idf_component_examples/websocket/main/main.cpp	Updates example to no longer write beyond the received buffer.
examples/WebSocket/WebSocket.ino	Updates example to no longer write beyond the received buffer; refreshes usage comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

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

[mathieucarbou]

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.

[Copilot]

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

[mathieucarbou]

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.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.