Off-by-one write in ws_connect reported by valgrind (IDFGH-13644) (IDFGH-13651)

[Sean-Der]

Answers checklist.

• I have read the documentation ESP-IDF Programming Guide and the issue is not addressed there.
• I have updated my IDF branch (master or release) to the latest version and checked that the issue is present there.
• I have searched the issue tracker for a similar issue and not found a similar issue.

IDF version.

master

Espressif SoC revision.

None/Linux build

Operating System used.

Linux

How did you build your project?

Command line with idf.py

If you are using Windows, please specify command line type.

None

Development Kit.

linux

Power Supply used.

USB

What is the expected behavior?

Not to crash

What is the actual behavior?

This results in memory corruption/crashing

==34079== Invalid write of size 1
==34079==    at 0x1214DB: ws_connect (transport_ws.c:294)
==34079==    by 0x11F55E: esp_transport_connect (transport.c:123)
==34079==    by 0x11C5C2: esp_websocket_client_task (esp_websocket_client.c:995)
==34079==    by 0x123271: pthread_task (freertos_linux.c:210)
==34079==    by 0x4BBDA93: start_thread (pthread_create.c:447)
==34079==    by 0x4C4AA33: clone (clone.S:100)
==34079==  Address 0x4e44400 is 0 bytes after a block of size 1,024 alloc'd
==34079==    at 0x4846828: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==34079==    by 0x121A29: esp_transport_ws_init (transport_ws.c:728)
==34079==    by 0x11D208: esp_websocket_client_create_transport (esp_websocket_client.c:497)
==34079==    by 0x11E2CF: esp_websocket_client_start (esp_websocket_client.c:1137)
==34079==    by 0x118181: app_websocket() (websocket.cpp:85)
==34079==    by 0x117674: main (main.cpp:25)

Steps to reproduce.

Run https://github.com/espressif/esp-protocols/tree/master/components/esp_websocket_client/examples/linux and use valgrind

Debug Logs.

==34079== Invalid write of size 1
==34079==    at 0x1214DB: ws_connect (transport_ws.c:294)
==34079==    by 0x11F55E: esp_transport_connect (transport.c:123)
==34079==    by 0x11C5C2: esp_websocket_client_task (esp_websocket_client.c:995)
==34079==    by 0x123271: pthread_task (freertos_linux.c:210)
==34079==    by 0x4BBDA93: start_thread (pthread_create.c:447)
==34079==    by 0x4C4AA33: clone (clone.S:100)
==34079==  Address 0x4e44400 is 0 bytes after a block of size 1,024 alloc'd
==34079==    at 0x4846828: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==34079==    by 0x121A29: esp_transport_ws_init (transport_ws.c:728)
==34079==    by 0x11D208: esp_websocket_client_create_transport (esp_websocket_client.c:497)
==34079==    by 0x11E2CF: esp_websocket_client_start (esp_websocket_client.c:1137)
==34079==    by 0x118181: app_websocket() (websocket.cpp:85)
==34079==    by 0x117674: main (main.cpp:25)

More Information.

N/A

[david-cermak]

Thanks for reporting! and using valgrind to identify for potential problems.

The issue you're seeing stems from reading the WebSocket header, a quick fix would be to:

diff --git a/components/tcp_transport/transport_ws.c b/components/tcp_transport/transport_ws.c
index 4aaa1845ed9..80a7c71c41d 100644
--- a/components/tcp_transport/transport_ws.c
+++ b/components/tcp_transport/transport_ws.c
@@ -285,13 +285,14 @@ static int ws_connect(esp_transport_handle_t t, const char *host, int port, int
     }
     int header_len = 0;
     do {
-        if ((len = esp_transport_read(ws->parent, ws->buffer + header_len, WS_BUFFER_SIZE - header_len, timeout_ms)) <= 0) {
+        if ((len = esp_transport_read(ws->parent, ws->buffer + header_len, WS_BUFFER_SIZE - header_len - 1, timeout_ms)) <= 0) {
             ESP_LOGE(TAG, "Error read response for Upgrade header %s", ws->buffer);
             return -1;
         }

PS: We received a similar report in espressif/esp-idf#14473

[Sean-Der]

Thank you @david-cermak I will close mine as a duplicate :)

[Sean-Der]

Should I submit a PR with the change above ^

[david-cermak]

Should I submit a PR with the change above ^

Thanks for offering! However, let's hold off for now. I'd like to give the original author of the issue, who pinpointed the root cause, the first opportunity to submit a PR.