fix(mbedtls): preserve pending write parameters across retries

docs/build.md

@@ -52,6 +52,11 @@ Options for building:
 * `-DUSE_WS=1` will build the ws interactive command line tool
 * `-DUSE_TEST=1` will build the unittest
 
+The default CTest suite uses loopback servers and in-memory TLS sessions; it does
+not require Internet access. TLS server tests generate a local CA and verify the
+client connection against it. The legacy external HTTP client tests in
+`test/IXHttpClientTest.cpp` are not part of the default suite.
+
 If you are on Windows, look at the [appveyor](https://github.com/machinezone/IXWebSocket/blob/master/appveyor.yml) file (not maintained much though) or rather the [github actions](https://github.com/machinezone/IXWebSocket/blob/master/.github/workflows/unittest_windows.yml) which have instructions for building dependencies.
 
 It is also possible to externally include the project, so that everything is fetched over the wire when you build like so:

ixwebsocket/IXSocketMbedTLS.cpp

@@ -14,6 +14,7 @@
 #include "IXNetSystem.h"
 #include "IXSocket.h"
 #include "IXSocketConnect.h"
+#include <algorithm>
 #include <cstdint>
 #include <string.h>
 
@@ -367,6 +368,7 @@ namespace ix
         mbedtls_x509_crt_free(&_cert);
         mbedtls_pk_free(&_pkey);
         _mbedtlsInitialized = false;
+        _sendBuffer.clear();
         // PSA crypto state is process-global; it is released in ix::uninitNetSystem(),
         // not per socket.
 
@@ -377,21 +379,41 @@ namespace ix
     {
         std::lock_guard<std::mutex> lock(_mutex);
 
-        std::ptrdiff_t res = mbedtls_ssl_write(&_ssl, (unsigned char*) buf, nbyte);
-
-        if (res > 0)
+        if (nbyte != 0 && buf == nullptr)
+        {
+            Socket::setErrno(EINVAL);
+            return -1;
+        }
+        if (_sendBuffer.empty())
+        {
+            if (nbyte == 0) return 0;
+            // Retain at most one TLS record, not the caller's growing send queue.
+            const size_t size = (std::min)(nbyte, size_t(16384));
+            _sendBuffer.assign(buf, buf + size);
+        }
+        else if (nbyte < _sendBuffer.size())
         {
-            return res;
+            Socket::setErrno(EINVAL);
+            return -1;
         }
-        else if (res == MBEDTLS_ERR_SSL_WANT_READ || res == MBEDTLS_ERR_SSL_WANT_WRITE)
+
+        // WANT_READ/WRITE must be retried with the same pointer and length.
+        // The caller may append to or reallocate its queue between calls.
+        const int res = mbedtls_ssl_write(&_ssl, _sendBuffer.data(), _sendBuffer.size());
+        if (res == MBEDTLS_ERR_SSL_WANT_READ || res == MBEDTLS_ERR_SSL_WANT_WRITE)
         {
             Socket::setErrno(EWOULDBLOCK);
             return -1;
         }
-        else
+
+        _sendBuffer.clear();
+        if (res < 0)
         {
+            // A previous retry may have left the socket's wait status set.
+            Socket::setErrno(EIO);
             return -1;
         }
+        return res;
     }
 
     std::ptrdiff_t SocketMbedTLS::recv(void* buf, size_t nbyte)

ixwebsocket/IXSocketMbedTLS.h

@@ -21,6 +21,7 @@
 #include <mbedtls/x509.h>
 #include <mbedtls/x509_crt.h>
 #include <mutex>
+#include <vector>
 
 namespace ix
 {
@@ -44,6 +45,7 @@ namespace ix
         virtual std::ptrdiff_t recv(void* buffer, size_t length) final;
 
     private:
+        friend struct SocketMbedTLSWriteTestAccess;
         mbedtls_ssl_context _ssl;
         mbedtls_ssl_config _conf;
 #if MBEDTLS_VERSION_MAJOR < 4
@@ -54,6 +56,7 @@ namespace ix
         mbedtls_x509_crt _cert;
         mbedtls_pk_context _pkey;
 
+        std::vector<unsigned char> _sendBuffer;
         std::mutex _mutex;
         SocketTLSOptions _tlsOptions;
         bool _mbedtlsInitialized = false;

test/CMakeLists.txt

@@ -52,6 +52,7 @@ endif()
 if (USE_TLS AND USE_MBED_TLS)
   list(APPEND TEST_TARGET_NAMES
     IXSocketMbedTLSPSATest
+    IXSocketMbedTLSWriteTest
   )
 endif()
 
@@ -85,6 +86,10 @@ target_compile_definitions(ixwebsocket_test PRIVATE ${TEST_PROGRAMS_DEFINITIONS}
 
 target_link_libraries(ixwebsocket_test Catch2)
 target_link_libraries(ixwebsocket_test ixwebsocket)
+if (WIN32 AND USE_TLS AND USE_MBED_TLS)
+  # Required by Mbed TLS builds that use the Windows CNG entropy source.
+  target_link_libraries(ixwebsocket_test bcrypt)
+endif()
 
 foreach(TEST_TARGET_NAME ${TEST_TARGET_NAMES})
   add_executable(${TEST_TARGET_NAME}

test/IXDNSLookupTest.cpp

@@ -14,29 +14,29 @@ using namespace ix;
 
 TEST_CASE("dns", "[net]")
 {
-    SECTION("Test resolving a known hostname")
+    SECTION("Test resolving a loopback address")
     {
-        auto dnsLookup = std::make_shared<DNSLookup>("www.google.com", 80);
+        auto dnsLookup = std::make_shared<DNSLookup>("127.0.0.1", 80);
 
         std::string errMsg;
         auto res = dnsLookup->resolve(errMsg, [] { return false; });
         std::cerr << "Error message: " << errMsg << std::endl;
         REQUIRE(res != nullptr);
     }
 
-    SECTION("Test resolving a non-existing hostname")
+    SECTION("Test resolving an invalid IPv6 literal")
     {
-        auto dnsLookup = std::make_shared<DNSLookup>("wwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwww", 80);
+        auto dnsLookup = std::make_shared<DNSLookup>("::invalid", 80);
 
         std::string errMsg;
         auto res = dnsLookup->resolve(errMsg, [] { return false; });
         std::cerr << "Error message: " << errMsg << std::endl;
         REQUIRE(res == nullptr);
     }
 
-    SECTION("Test resolving a good hostname, with cancellation")
+    SECTION("Test resolving a loopback address, with cancellation")
     {
-        auto dnsLookup = std::make_shared<DNSLookup>("www.google.com", 80, 1);
+        auto dnsLookup = std::make_shared<DNSLookup>("127.0.0.1", 80, 1);
 
         std::string errMsg;
         // The callback returning true means we are requesting cancellation

test/IXHttpServerTest.cpp

@@ -59,8 +59,13 @@ TEST_CASE("http server", "[httpd]")
 
         REQUIRE(response->errorCode == HttpErrorCode::Ok);
         REQUIRE(response->statusCode == 200);
+#ifdef IXWEBSOCKET_USE_ZLIB
         REQUIRE(response->headers["Accept-Encoding"] == "gzip");
         REQUIRE(response->headers["Content-Encoding"] == "gzip");
+#else
+        REQUIRE(response->headers["Accept-Encoding"].empty());
+        REQUIRE(response->headers["Content-Encoding"].empty());
+#endif
 
         server.stop();
     }
@@ -121,7 +126,8 @@ TEST_CASE("http server redirection", "[httpd_redirect]")
     {
         int port = getFreePort();
         ix::HttpServer server(port, "127.0.0.1");
-        server.makeRedirectServer("http://example.com");
+        const std::string targetUrl("http://127.0.0.1:1/not-followed");
+        server.makeRedirectServer(targetUrl);
 
         auto res = server.listen();
         REQUIRE(res.first);
@@ -163,16 +169,29 @@ TEST_CASE("http server redirection", "[httpd_redirect]")
 
         REQUIRE(response->errorCode == HttpErrorCode::Ok);
         REQUIRE(response->statusCode == 301);
-        REQUIRE(response->headers["Location"] == "http://example.com");
+        REQUIRE(response->headers["Location"] == targetUrl);
 
         server.stop();
     }
 
     SECTION("Connect to a local HTTP server, with redirection enabled, but we do follow redirects")
     {
+        ix::HttpServer target(getFreePort(), "127.0.0.1");
+        const std::string expectedBody("local redirect target");
+        target.setOnConnectionCallback(
+            [expectedBody](HttpRequestPtr request, std::shared_ptr<ConnectionState>) -> HttpResponsePtr {
+                if (request->uri != "/redirect-target")
+                    return std::make_shared<HttpResponse>(404, "NOT FOUND");
+                return std::make_shared<HttpResponse>(
+                    200, "OK", HttpErrorCode::Ok, WebSocketHttpHeaders(), expectedBody);
+            });
+        REQUIRE(target.listen().first);
+        target.start();
+
         int port = getFreePort();
         ix::HttpServer server(port, "127.0.0.1");
-        server.makeRedirectServer("http://www.google.com");
+        server.makeRedirectServer("http://127.0.0.1:" + std::to_string(target.getPort()) +
+                                  "/redirect-target");
 
         auto res = server.listen();
         REQUIRE(res.first);
@@ -214,7 +233,9 @@ TEST_CASE("http server redirection", "[httpd_redirect]")
 
         REQUIRE(response->errorCode == HttpErrorCode::Ok);
         REQUIRE(response->statusCode == 200);
+        REQUIRE(response->body == expectedBody);
 
         server.stop();
+        target.stop();
     }
 }

test/IXSocketConnectTest.cpp

@@ -26,11 +26,10 @@ TEST_CASE("socket_connect", "[net]")
         REQUIRE(fd != -1);
     }
 
-    SECTION("Test connecting to a non-existing hostname")
+    SECTION("Test connecting to an invalid IPv6 literal")
     {
         std::string errMsg;
-        std::string hostname("12313lhjlkjhopiupoijlkasdckljqwehrlkqjwehraospidcuaposidcasdc");
-        int fd = SocketConnect::connect(hostname, 80, errMsg, [] { return false; });
+        int fd = SocketConnect::connect("::invalid", 80, errMsg, [] { return false; });
         std::cerr << "Error message: " << errMsg << std::endl;
         REQUIRE(fd == -1);
     }