From a4a1e7b7f2e03e18ecea2d7f6b83d43d4b3b4250 Mon Sep 17 00:00:00 2001 From: David Cermak Date: Tue, 17 Sep 2024 18:09:07 +0200 Subject: [PATCH 1/2] feat(transport): Add more testcases for ws-connect to fail Test case "ws connect fails (big response)" should fail on this commit due to an off-by-one issue in ws_connect() read when chunk reading http header(s). Related to https://github.com/espressif/esp-idf/issues/14473 --- .../main/test_websocket_transport.cpp | 49 +++++++++++++++++-- 1 file changed, 45 insertions(+), 4 deletions(-) diff --git a/components/tcp_transport/host_test/main/test_websocket_transport.cpp b/components/tcp_transport/host_test/main/test_websocket_transport.cpp index f5c2ccc449..2d00bb143f 100644 --- a/components/tcp_transport/host_test/main/test_websocket_transport.cpp +++ b/components/tcp_transport/host_test/main/test_websocket_transport.cpp @@ -21,7 +21,7 @@ #include "esp_transport_tcp.h" #include "esp_transport_ws.h" -#define WS_BUFFER_SIZE 1024 +#define WS_BUFFER_SIZE CONFIG_WS_BUFFER_SIZE extern "C" { #include "Mockmock_transport.h" @@ -103,7 +103,8 @@ int mock_write_callback(esp_transport_handle_t transport, const char *request_se } // Callback function for mock_read -int mock_read_callback(esp_transport_handle_t transport, char *buffer, int len, int timeout_ms, int num_call) { +int mock_valid_read_callback(esp_transport_handle_t transport, char *buffer, int len, int timeout_ms, int num_call) +{ std::string websocket_response = make_response(); std::memcpy(buffer, websocket_response.data(), websocket_response.size()); return websocket_response.size(); @@ -111,7 +112,7 @@ int mock_read_callback(esp_transport_handle_t transport, char *buffer, int len, } -TEST_CASE("WebSocket Transport Connection", "[websocket_transport]") { +void test_ws_connect(bool expect_valid_connection, CMOCK_mock_read_CALLBACK read_callback) { constexpr static auto timeout = 50; constexpr static auto port = 8080; constexpr static auto host = "localhost"; @@ -134,10 +135,18 @@ TEST_CASE("WebSocket Transport Connection", "[websocket_transport]") { mock_write_Stub(mock_write_callback); mock_connect_ExpectAndReturn(parent_handle.get(), host, port, timeout, ESP_OK); // Set the callback function for mock_read - mock_read_Stub(mock_read_callback); + mock_read_Stub(read_callback); mock_poll_read_ExpectAnyArgsAndReturn(1); esp_crypto_base64_encode_ExpectAnyArgsAndReturn(0); mock_destroy_ExpectAnyArgsAndReturn(ESP_OK); + + if (!expect_valid_connection) { + // for invalid connections we only check that the connect() function fails + REQUIRE(esp_transport_connect(websocket_transport.get(), host, port, timeout) != 0); + // and we're done here + return; + } + REQUIRE(esp_transport_connect(websocket_transport.get(), host, port, timeout) == 0); char buffer[WS_BUFFER_SIZE]; @@ -147,5 +156,37 @@ TEST_CASE("WebSocket Transport Connection", "[websocket_transport]") { std::string response(buffer, read_len); REQUIRE(response == "Test"); + } } + +// Happy flow +TEST_CASE("WebSocket Transport Connection", "[websocket_transport]") +{ + test_ws_connect(true, mock_valid_read_callback); +} + +// Some corner cases where we expect the ws connection to fail + +TEST_CASE("ws connect fails (0 len response)", "[websocket_transport]") +{ + test_ws_connect(false, [](esp_transport_handle_t h, char *buf, int len, int tout, int n) { + return 0; + }); +} + +TEST_CASE("ws connect fails (invalid response)", "[websocket_transport]") +{ + test_ws_connect(false, [](esp_transport_handle_t h, char *buf, int len, int tout, int n) { + int resp_len = len/2; + std::memset(buf, '?', resp_len); + return resp_len; + }); +} + +TEST_CASE("ws connect fails (big response)", "[websocket_transport]") +{ + test_ws_connect(false, [](esp_transport_handle_t h, char *buf, int len, int tout, int n) { + return WS_BUFFER_SIZE; + }); +} From 53e63eb1251c26d748d3fcbe910c762b595f3943 Mon Sep 17 00:00:00 2001 From: David Cermak Date: Tue, 17 Sep 2024 18:14:54 +0200 Subject: [PATCH 2/2] fix(transport): Fix websocket mem-corruption while reading headers Closes https://github.com/espressif/esp-idf/issues/14473 --- components/tcp_transport/transport_ws.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/components/tcp_transport/transport_ws.c b/components/tcp_transport/transport_ws.c index 4aaa1845ed..b2c6362b39 100644 --- a/components/tcp_transport/transport_ws.c +++ b/components/tcp_transport/transport_ws.c @@ -285,7 +285,7 @@ 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 - 1 - header_len, timeout_ms)) <= 0) { ESP_LOGE(TAG, "Error read response for Upgrade header %s", ws->buffer); return -1; }