diff --git a/src/common/restapi/http/ServerConnectionManager.h b/src/common/restapi/http/ServerConnectionManager.h index 214db239..a8c0cdd1 100644 --- a/src/common/restapi/http/ServerConnectionManager.h +++ b/src/common/restapi/http/ServerConnectionManager.h @@ -5,7 +5,7 @@ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * Copyright (c) 2003-2013 Christopher M. Kohlhoff - * Copyright (C) 2023 Bryan Biedenkapp, N2PLL + * Copyright (C) 2023,2026 Bryan Biedenkapp, N2PLL * */ /** @@ -52,8 +52,8 @@ namespace restapi */ void start(ConnectionPtr c) { - std::lock_guard guard(m_lock); { + std::lock_guard guard(m_lock); m_connections.insert(c); } c->start(); @@ -65,8 +65,8 @@ namespace restapi */ void stop(ConnectionPtr c) { - std::lock_guard guard(m_lock); { + std::lock_guard guard(m_lock); m_connections.erase(c); } c->stop(); @@ -77,11 +77,16 @@ namespace restapi */ void stopAll() { - std::lock_guard guard(m_lock); - for (auto c : m_connections) - c->stop(); + std::set connections; - m_connections.clear(); + // scope is intentional + { + std::lock_guard guard(m_lock); + connections.swap(m_connections); + } + + for (const ConnectionPtr& c : connections) + c->stop(); } private: diff --git a/tests/network/HTTP_RestAPI_Tests.cpp b/tests/network/HTTP_RestAPI_Tests.cpp index 502bc5c2..4e1bb5a9 100644 --- a/tests/network/HTTP_RestAPI_Tests.cpp +++ b/tests/network/HTTP_RestAPI_Tests.cpp @@ -14,12 +14,15 @@ #include "common/restapi/http/HTTPClient.h" #include "common/restapi/http/HTTPPayload.h" #include "common/restapi/http/HTTPServer.h" +#include "common/restapi/http/ServerConnectionManager.h" #include "common/restapi/RequestDispatcher.h" #include +#include #include #include #include +#include #include #include #include @@ -67,7 +70,7 @@ namespace { for (const std::string& fragment : fragments) { asio::write(socket, asio::buffer(fragment)); if (fragments.size() > 1U) - std::this_thread::sleep_for(std::chrono::milliseconds(10)); + std::this_thread::sleep_for(std::chrono::milliseconds(1)); } std::string response; @@ -82,6 +85,34 @@ namespace { return response; } + /** + * @brief Sends a partial request, closes the sending direction, and collects any response. + * @param port The port to which the partial request is sent. + * @param request Partial HTTP request bytes. + * @return Raw response bytes received before the server closes the connection. + */ + std::string sendPartialRequest(uint16_t port, const std::string& request) + { + asio::io_service ioService; + asio::ip::tcp::socket socket(ioService); + socket.connect(asio::ip::tcp::endpoint(asio::ip::address_v4::loopback(), port)); + asio::write(socket, asio::buffer(request)); + + asio::error_code ec; + socket.shutdown(asio::ip::tcp::socket::shutdown_send, ec); + REQUIRE_FALSE(ec); + + std::string response; + std::array buffer {}; + do { + const size_t length = socket.read_some(asio::buffer(buffer), ec); + response.append(buffer.data(), length); + } while (!ec); + + REQUIRE(ec == asio::error::eof); + return response; + } + /** * @brief Sends a raw HTTP request without using test assertions in the calling thread. * @param port The port to which the HTTP request should be sent. @@ -117,6 +148,36 @@ namespace { REQUIRE(separator != std::string::npos); return response.substr(separator + 4U); } + + struct ManagedConnection; + using ManagedConnectionPtr = std::shared_ptr; + using TestConnectionManager = ServerConnectionManager; + + /** + * @brief Minimal managed connection used to verify manager callback lock scope. + */ + struct ManagedConnection { + std::function onStart; + std::function onStop; + std::atomic starts { 0U }; + std::atomic stops { 0U }; + + /** @brief Records start and invokes the configured callback. */ + void start() + { + ++starts; + if (onStart) + onStart(); + } + + /** @brief Records stop and invokes the configured callback. */ + void stop() + { + ++stops; + if (onStop) + onStop(); + } + }; } // --------------------------------------------------------------------------- @@ -397,6 +458,49 @@ TEST_CASE("HTTP server accumulates a fragmented request body", "[restapi][http][ REQUIRE(fixture.state->calls == 1U); } +TEST_CASE("HTTP server accepts every request header and body split", "[restapi][http][framing][e2e]") +{ + LoopbackHTTPServer fixture; + const std::string body("split\0body", 10U); + const std::string request = + "PUT /split HTTP/1.1\r\n" + "Host: localhost\r\n" + "Content-Length: " + std::to_string(body.size()) + "\r\n" + "\r\n" + body; + + for (size_t split = 1U; split < request.size(); ++split) { + CAPTURE(split); + const std::string response = sendRawRequest(fixture.port, + { request.substr(0U, split), request.substr(split) }); + REQUIRE(response.find("HTTP/1.0 200 OK\r\n") == 0U); + REQUIRE(responseBody(response) == body); + } + + REQUIRE(fixture.state->calls == request.size() - 1U); +} + +TEST_CASE("HTTP server preserves fragmented binary bodies", "[restapi][http][framing][e2e]") +{ + LoopbackHTTPServer fixture; + std::string body(20000U, 'a'); + body[0U] = '\0'; + body[8191U] = '\0'; + body[19999U] = '\0'; + + const std::string header = + "PUT /binary HTTP/1.1\r\nHost: localhost\r\nContent-Length: " + + std::to_string(body.size()) + "\r\n\r\n"; + std::vector fragments { header }; + for (size_t offset = 0U; offset < body.size(); offset += 997U) + fragments.push_back(body.substr(offset, std::min(997U, body.size() - offset))); + + const std::string response = sendRawRequest(fixture.port, fragments); + + REQUIRE(response.find("HTTP/1.0 200 OK\r\n") == 0U); + REQUIRE(responseBody(response) == body); + REQUIRE(fixture.state->calls == 1U); +} + TEST_CASE("HTTP server rejects malformed Content-Length", "[restapi][http][e2e][negative]") { LoopbackHTTPServer fixture; @@ -409,6 +513,39 @@ TEST_CASE("HTTP server rejects malformed Content-Length", "[restapi][http][e2e][ REQUIRE(fixture.state->calls == 0U); } +TEST_CASE("HTTP server rejects ambiguous Content-Length syntax", "[restapi][http][framing][e2e][negative]") +{ + LoopbackHTTPServer fixture; + const std::vector values { + "+1", + "-1", + "1, 1", + "1x", + "1844674407370955161600000" + }; + + for (const std::string& value : values) { + CAPTURE(value); + const std::string request = + "PUT /length HTTP/1.1\r\nHost: localhost\r\nContent-Length: " + value + "\r\n\r\n"; + const std::string response = sendRawRequest(fixture.port, { request }); + REQUIRE(response.find("HTTP/1.0 400 Bad Request\r\n") == 0U); + } + + REQUIRE(fixture.state->calls == 0U); +} + +TEST_CASE("HTTP server never dispatches truncated requests", "[restapi][http][framing][e2e][negative]") +{ + LoopbackHTTPServer fixture; + + REQUIRE(sendPartialRequest(fixture.port, + "PUT /partial HTTP/1.1\r\nHost: localhost\r\nContent-Len").empty()); + REQUIRE(sendPartialRequest(fixture.port, + "PUT /partial HTTP/1.1\r\nHost: localhost\r\nContent-Length: 5\r\n\r\nhe").empty()); + REQUIRE(fixture.state->calls == 0U); +} + TEST_CASE("HTTP server rejects an oversized declared body", "[restapi][http][e2e][negative]") { LoopbackHTTPServer fixture; @@ -479,3 +616,39 @@ TEST_CASE("HTTP server services another connection while a REST handler is runni REQUIRE(fastElapsed < std::chrono::milliseconds(400)); REQUIRE(responseBody(slowResponse) == "slow"); } + +TEST_CASE("HTTP connection manager invokes lifecycle callbacks outside its lock", "[restapi][http][connection-manager]") +{ + TestConnectionManager manager; + ManagedConnectionPtr first = std::make_shared(); + ManagedConnectionPtr second = std::make_shared(); + + first->onStart = [&manager, second]() { manager.start(second); }; + first->onStop = [&manager, second]() { manager.stop(second); }; + + manager.start(first); + REQUIRE(first->starts == 1U); + REQUIRE(second->starts == 1U); + + manager.stop(first); + REQUIRE(first->stops == 1U); + REQUIRE(second->stops == 1U); +} + +TEST_CASE("HTTP connection manager drains before invoking stop callbacks", "[restapi][http][connection-manager]") +{ + TestConnectionManager manager; + ManagedConnectionPtr first = std::make_shared(); + ManagedConnectionPtr replacement = std::make_shared(); + + first->onStop = [&manager, replacement]() { manager.start(replacement); }; + manager.start(first); + manager.stopAll(); + + REQUIRE(first->stops == 1U); + REQUIRE(replacement->starts == 1U); + REQUIRE(replacement->stops == 0U); + + manager.stopAll(); + REQUIRE(replacement->stops == 1U); +}