From 902d8908c94dbe837d712a408d63dcbac634d4c5 Mon Sep 17 00:00:00 2001 From: Matthew Zipkin Date: Mon, 13 Jul 2026 10:41:33 -0400 Subject: [PATCH 1/5] http: only read one HTTPRequest at a time per client --- src/httpserver.cpp | 22 ++++++++++++---------- src/httpserver.h | 14 +++++++++----- 2 files changed, 21 insertions(+), 15 deletions(-) diff --git a/src/httpserver.cpp b/src/httpserver.cpp index 3f271da389e..dd17754bfac 100644 --- a/src/httpserver.cpp +++ b/src/httpserver.cpp @@ -30,7 +30,6 @@ #include #include #include -#include #include #include #include @@ -996,13 +995,13 @@ void HTTPServer::ThreadSocketHandler() void HTTPServer::MaybeDispatchRequestsFromClient(const std::shared_ptr& client) const { - // Try reading (potentially multiple) HTTP requests from the buffer - while (!client->m_recv_buffer.empty()) { + // Try reading the next HTTP request from the buffer + if (!client->m_req) { // Create a new request object and try to fill it with data from the receive buffer auto req = std::make_unique(client); try { // Stop reading if we need more data from the client to parse a complete request - if (!client->ReadRequest(*req)) break; + if (!client->ReadRequest(*req)) return; } catch (const ContentTooLargeError& e) { LogDebug( BCLog::HTTP, @@ -1037,8 +1036,8 @@ void HTTPServer::MaybeDispatchRequestsFromClient(const std::shared_ptrm_origin, client->m_id); - // add request to client queue - client->m_req_queue.push_back(std::move(req)); + // Move request to client + client->m_req = std::move(req); } // If we are already handling a request from @@ -1046,12 +1045,11 @@ void HTTPServer::MaybeDispatchRequestsFromClient(const std::shared_ptrm_req_busy) return; - // Otherwise, if there is a pending request in the queue, handle it. - if (!client->m_req_queue.empty()) { + // Otherwise, if there is a request ready to go, handle it. + if (client->m_req) { LOCK(m_request_dispatcher_mutex); client->m_req_busy = true; - m_request_dispatcher(std::move(client->m_req_queue.front())); - client->m_req_queue.pop_front(); + m_request_dispatcher(std::move(client->m_req)); } } @@ -1102,6 +1100,7 @@ void HTTPServer::DisconnectClients() "Disconnecting HTTP client %s (id=%llu)", client->m_origin, client->m_id); + client->ReleaseRequest(); return true; }); if (erased > 0) { @@ -1116,6 +1115,9 @@ void HTTPServer::ClearConnectedClients() if (m_connected.empty()) return; LogWarning("Force-disconnecting %d HTTP client(s) that did not disconnect gracefully", m_connected.size()); m_connected_size.fetch_sub(m_connected.size(), std::memory_order_relaxed); + for (auto& client : m_connected) { + client->ReleaseRequest(); + } m_connected.clear(); } diff --git a/src/httpserver.h b/src/httpserver.h index 943dc12def4..1ae4a5e5ee5 100644 --- a/src/httpserver.h +++ b/src/httpserver.h @@ -6,7 +6,6 @@ #define BITCOIN_HTTPSERVER_H #include -#include #include #include #include @@ -479,10 +478,9 @@ public: std::string m_recv_buffer{}; //! Requests from a client must be processed in the order in which - //! they were received, blocking on a per-client basis. We won't - //! process the next request in the queue if we are currently busy - //! handling a previous request. - std::deque> m_req_queue; + //! they were received, blocking on a per-client basis. We read + //! one request at a time from the socket buffer then pass it to a worker. + std::unique_ptr m_req; //! Set to true by the I/O thread when a request is popped off //! and passed to a worker thread, reset to false by the worker thread. @@ -555,6 +553,12 @@ public: HTTPRemoteClient(const HTTPRemoteClient&) = delete; HTTPRemoteClient& operator=(const HTTPRemoteClient&) = delete; + //! Release any in-progress request. HTTPRequest holds a shared_ptr back to its + //! HTTPRemoteClient to keep the client alive from a worker thread. If a request + //! hasn't been moved to a worker yet it will prevent the client from destructing + //! and never close the socket. Therefore this must be called when disconnecting. + void ReleaseRequest() { m_req.reset(); } + /** * Try to read an HTTP request from the receive buffer. * @param[in] req A HTTPRequest to read into From 507e528e845034583dd21b884e6debb1ff5307e3 Mon Sep 17 00:00:00 2001 From: Matthew Zipkin Date: Thu, 23 Jul 2026 12:38:12 -0400 Subject: [PATCH 2/5] http: reuse HTTPHeaders to parse chunked trailer Chunked transfer trailers are just headers that are included at the end of the request. We can parse and validate them with code we already use to read headers. In a future commit we will also be able to use one MAX_HEADERS_SIZE limit to cover both sections. Even though we parse and validate, we ignore these data. --- src/httpserver.cpp | 28 ++++++++++------------------ src/httpserver.h | 4 +++- src/test/httpserver_tests.cpp | 3 ++- 3 files changed, 15 insertions(+), 20 deletions(-) diff --git a/src/httpserver.cpp b/src/httpserver.cpp index dd17754bfac..dfae7d10a66 100644 --- a/src/httpserver.cpp +++ b/src/httpserver.cpp @@ -298,12 +298,13 @@ void HTTPHeaders::RemoveAll(std::string_view key) m_headers.erase(moved.begin(), moved.end()); } -bool HTTPHeaders::Read(util::LineReader& reader) +bool HTTPHeaders::Read(util::LineReader& reader, bool write) { // Headers https://httpwg.org/specs/rfc9110.html#rfc.section.6.3 // A sequence of Field Lines https://httpwg.org/specs/rfc9110.html#rfc.section.5.2 + size_t start{reader.Consumed()}; while (auto maybe_line = reader.ReadLine()) { - if (reader.Consumed() > MAX_HEADERS_SIZE) throw std::runtime_error("HTTP headers exceed size limit"); + if (reader.Consumed() - start > MAX_HEADERS_SIZE) throw std::runtime_error("HTTP headers exceed size limit"); const std::string_view& line = *maybe_line; @@ -336,7 +337,9 @@ bool HTTPHeaders::Read(util::LineReader& reader) // that can not be empty. if (key.empty()) throw std::runtime_error("Empty HTTP header name"); - Write(std::string(key), std::move(value)); + if (write) { + Write(std::string(key), std::move(value)); + } } return false; @@ -447,22 +450,11 @@ bool HTTPRequest::LoadBody(LineReader& reader) // Last chunk has size 0 if (*chunk_size == 0) { - // Allow (but ignore) Chunked Trailer section, by - // reading CRLF-terminated lines until we read an empty line, - // which indicates the end of this request. + // Validate Chunked Trailer section, which is used for + // additional headers sent at the end of the message. + // At this time we ignore and drop these data after validating. // See https://httpwg.org/specs/rfc9112.html#rfc.section.7.1.2 - const size_t trailer_start{reader.Consumed()}; - while (true) { - auto maybe_trailer = reader.ReadLine(); - if (reader.Consumed() - trailer_start > MAX_HEADERS_SIZE) { - throw std::runtime_error("HTTP chunked trailer exceeds size limit"); - } - if (!maybe_trailer) return false; - if (maybe_trailer->empty()) break; - } - // Complete request has been parsed, reader is now pointing - // to beginning of next request or end of the buffer. - return true; + return m_headers.Read(reader, /*write=*/false); } // We are still expecting more data for this chunk diff --git a/src/httpserver.h b/src/httpserver.h index 1ae4a5e5ee5..d92b2054442 100644 --- a/src/httpserver.h +++ b/src/httpserver.h @@ -104,13 +104,15 @@ public: */ void RemoveAll(std::string_view key); /** + * @param[in] reader A LineReader instance initialized with the client's receive buffer. + * @param[in] write Whether or not to write the parsed data to the object after validation. * @returns false if LineReader hits the end of the buffer before reading an * \n, meaning that we are still waiting on more data from the client. * true after reading an entire HTTP headers section, terminated * by an empty line and \n. * @throws on exceeded read limit and on bad headers syntax (e.g. no ":" in a line) */ - bool Read(util::LineReader& reader); + bool Read(util::LineReader& reader, bool write = true); std::string Stringify() const; private: diff --git a/src/test/httpserver_tests.cpp b/src/test/httpserver_tests.cpp index cd020fc60c1..c03b3e41f78 100644 --- a/src/test/httpserver_tests.cpp +++ b/src/test/httpserver_tests.cpp @@ -446,8 +446,9 @@ BOOST_AUTO_TEST_CASE(http_request_tests) BOOST_CHECK(req.LoadHeaders(reader)); BOOST_CHECK(req.LoadBody(reader)); BOOST_CHECK_EQUAL(req.m_body, R"({"method":"getblockcount"})"); - // Chunk Trailer was cleared + // Chunk Trailer was parsed, but ignored BOOST_CHECK_EQUAL(reader.Remaining(), 0); + BOOST_CHECK(!req.GetHeader("Expires").first); } { // Invalid "chunked" transfer, using roman numerals instead of hex for chunk length From 90676e24ad1aa44346aed586f19ba1c68c477394 Mon Sep 17 00:00:00 2001 From: Matthew Zipkin Date: Tue, 14 Jul 2026 15:52:37 -0400 Subject: [PATCH 3/5] Add state to HTTPRequest to avoid duplicate work over I/O cycles --- src/httpserver.cpp | 209 ++++++++++++++++++------------ src/httpserver.h | 29 ++++- src/test/httpserver_tests.cpp | 21 --- test/functional/interface_http.py | 98 +++++++++----- 4 files changed, 220 insertions(+), 137 deletions(-) diff --git a/src/httpserver.cpp b/src/httpserver.cpp index dfae7d10a66..16215170141 100644 --- a/src/httpserver.cpp +++ b/src/httpserver.cpp @@ -304,12 +304,16 @@ bool HTTPHeaders::Read(util::LineReader& reader, bool write) // A sequence of Field Lines https://httpwg.org/specs/rfc9110.html#rfc.section.5.2 size_t start{reader.Consumed()}; while (auto maybe_line = reader.ReadLine()) { - if (reader.Consumed() - start > MAX_HEADERS_SIZE) throw std::runtime_error("HTTP headers exceed size limit"); + if (reader.Consumed() - start + m_consumed > MAX_HEADERS_SIZE) throw std::runtime_error("HTTP headers exceed size limit"); const std::string_view& line = *maybe_line; // An empty line indicates end of the headers section https://www.rfc-editor.org/rfc/rfc2616#section-4 - if (line.empty()) return true; + if (line.empty()) { + // Ensure all headers are accounted for in case there is a chunked trailer + m_consumed += reader.Consumed() - start; + return true; + } // "Field values containing CR, LF, or NUL characters are invalid and dangerous" // https://httpwg.org/specs/rfc9110.html#rfc.section.5.5 @@ -342,6 +346,11 @@ bool HTTPHeaders::Read(util::LineReader& reader, bool write) } } + // We have not received all the request headers yet. + // Keep track of how much data we have already consumed to enforce + // the total limit over multiple read operations. + m_consumed += reader.Consumed() - start; + return false; } @@ -430,51 +439,68 @@ bool HTTPRequest::LoadBody(LineReader& reader) // Chunked Transfer Coding: https://datatracker.ietf.org/doc/html/rfc7230.html#section-4.1 // see evhttp_handle_chunked_read() in libevent http.c while (reader.Remaining() > 0) { - auto maybe_chunk_size = reader.ReadLine(); - if (!maybe_chunk_size) return false; + if (!m_chunk_size) { + auto maybe_chunk_size = reader.ReadLine(); + if (!maybe_chunk_size) return false; - // Allow (but ignore) Chunk Extensions - // See https://www.rfc-editor.org/rfc/rfc9112.html#name-chunk-extensions - std::string_view chunk_size_noext{maybe_chunk_size.value()}; - const auto semicolon_pos = chunk_size_noext.find(';'); - if (semicolon_pos != chunk_size_noext.npos) { - chunk_size_noext.remove_suffix(chunk_size_noext.size() - semicolon_pos); + // Allow (but ignore) Chunk Extensions + // See https://www.rfc-editor.org/rfc/rfc9112.html#name-chunk-extensions + std::string_view chunk_size_noext{maybe_chunk_size.value()}; + const auto semicolon_pos = chunk_size_noext.find(';'); + if (semicolon_pos != chunk_size_noext.npos) { + chunk_size_noext.remove_suffix(chunk_size_noext.size() - semicolon_pos); + } + + m_chunk_size = ToIntegral(util::TrimStringView(chunk_size_noext), /*base=*/16); + if (!m_chunk_size) throw std::runtime_error("Cannot parse chunk length value"); + + if ((m_body.size() > MAX_BODY_SIZE) || + (*m_chunk_size > MAX_BODY_SIZE - m_body.size())) + throw ContentTooLargeError("Chunk will exceed max body size"); } - const auto chunk_size{ToIntegral(util::TrimStringView(chunk_size_noext), /*base=*/16)}; - if (!chunk_size) throw std::runtime_error("Cannot parse chunk length value"); - - if ((m_body.size() > MAX_BODY_SIZE) || - (*chunk_size > MAX_BODY_SIZE - m_body.size())) - throw ContentTooLargeError("Chunk will exceed max body size"); + // We either just read the chunk size, or we have it saved + // from a prior I/O loop iteration + Assume(m_chunk_size); // Last chunk has size 0 - if (*chunk_size == 0) { + if (*m_chunk_size == 0) { // Validate Chunked Trailer section, which is used for // additional headers sent at the end of the message. + // Data consumed here is counted towards MAX_HEADERS_SIZE + // along with the headers we read in the beginning of the request. // At this time we ignore and drop these data after validating. // See https://httpwg.org/specs/rfc9112.html#rfc.section.7.1.2 return m_headers.Read(reader, /*write=*/false); } - // We are still expecting more data for this chunk - if (reader.Remaining() < *chunk_size) { - return false; - } + // We have not read the entire chunk from the buffer yet + if (m_chunk_read < *m_chunk_size) { + // Get what we can from the buffer + const uint64_t chunk_need{*m_chunk_size - m_chunk_read}; + const uint64_t buffer_has{std::min(chunk_need, static_cast(reader.Remaining()))}; - // Pack chunk onto body - m_body += reader.ReadLength(*chunk_size); + // Pack [partial] chunk onto body and update state + m_body += reader.ReadLength(buffer_has); + m_chunk_read += buffer_has; + } // Even though every chunk size is explicitly declared, // they are still terminated by a CRLF we don't need, // just consume it here. - auto crlf = reader.ReadLine(); - if (!crlf) { - // CRLF not found before end of buffer: it has not been received by our socket yet. - return false; + if (m_chunk_read == *m_chunk_size) { + auto crlf = reader.ReadLine(); + if (!crlf) { + // CRLF not found before end of buffer: it has not been received by our socket yet. + return false; + } + // CRLF was found but there was unexpected data after the chunk_sized chunk + if (!crlf.value().empty()) throw std::runtime_error("Improperly terminated chunk"); + + // Clear state for next chunk + m_chunk_size.reset(); + m_chunk_read = 0; } - // CRLF was found but there was unexpected data after the chunk_sized chunk - if (!crlf.value().empty()) throw std::runtime_error("Improperly terminated chunk"); } // We read all the chunks but never got the last chunk, wait for client to send more @@ -496,12 +522,15 @@ bool HTTPRequest::LoadBody(LineReader& reader) if (*content_length > MAX_BODY_SIZE) throw ContentTooLargeError("Max body size exceeded"); - // Not enough data in buffer for expected body - if (reader.Remaining() < *content_length) return false; + // A large body may arrive over multiple I/O loop iterations. Copy + // whatever the buffer has now; m_body's size tracks our progress. + const uint64_t body_need{*content_length - m_body.size()}; + const uint64_t buffer_has{std::min(body_need, static_cast(reader.Remaining()))}; - m_body = reader.ReadLength(*content_length); + // Pack [partial] body on and update state + m_body += reader.ReadLength(buffer_has); - return true; + return m_body.size() == *content_length; } } @@ -987,49 +1016,36 @@ void HTTPServer::ThreadSocketHandler() void HTTPServer::MaybeDispatchRequestsFromClient(const std::shared_ptr& client) const { - // Try reading the next HTTP request from the buffer if (!client->m_req) { - // Create a new request object and try to fill it with data from the receive buffer - auto req = std::make_unique(client); - try { - // Stop reading if we need more data from the client to parse a complete request - if (!client->ReadRequest(*req)) return; - } catch (const ContentTooLargeError& e) { - LogDebug( - BCLog::HTTP, - "HTTP request body too large from client %s (id=%llu): %s", - client->m_origin, - client->m_id, - e.what()); + client->m_req = std::make_unique(client); + } - WriteNoStoreErrorReply(*req, HTTP_CONTENT_TOO_LARGE); - client->m_disconnect = true; - return; - } catch (const std::runtime_error& e) { - LogDebug( - BCLog::HTTP, - "Error reading HTTP request from client %s (id=%llu): %s", - client->m_origin, - client->m_id, - e.what()); - - // We failed to read a complete request from the buffer - WriteNoStoreErrorReply(*req, HTTP_BAD_REQUEST); - client->m_disconnect = true; - return; - } - - // We read a complete request from the buffer into the queue + try { + // Read data from the buffer into the current request + client->ReadRequest(*client->m_req); + } catch (const ContentTooLargeError& e) { LogDebug( BCLog::HTTP, - "Received a %s request for %s from %s (id=%llu)", - RequestMethodString(req->m_method), - req->m_target, + "HTTP request body too large from client %s (id=%llu): %s", client->m_origin, - client->m_id); + client->m_id, + e.what()); - // Move request to client - client->m_req = std::move(req); + WriteNoStoreErrorReply(*client->m_req, HTTP_CONTENT_TOO_LARGE); + client->m_disconnect = true; + return; + } catch (const std::runtime_error& e) { + LogDebug( + BCLog::HTTP, + "Error reading HTTP request from client %s (id=%llu): %s", + client->m_origin, + client->m_id, + e.what()); + + // We failed to read a complete request from the buffer + WriteNoStoreErrorReply(*client->m_req, HTTP_BAD_REQUEST); + client->m_disconnect = true; + return; } // If we are already handling a request from @@ -1037,8 +1053,16 @@ void HTTPServer::MaybeDispatchRequestsFromClient(const std::shared_ptrm_req_busy) return; - // Otherwise, if there is a request ready to go, handle it. - if (client->m_req) { + // Otherwise, if the request is ready, hand it to a worker. + if (client->m_req->GetState() == HTTPRequest::State::Complete) { + LogDebug( + BCLog::HTTP, + "Received a %s request for %s from %s (id=%llu)", + RequestMethodString(client->m_req->m_method), + client->m_req->m_target, + client->m_origin, + client->m_id); + LOCK(m_request_dispatcher_mutex); client->m_req_busy = true; m_request_dispatcher(std::move(client->m_req)); @@ -1113,22 +1137,47 @@ void HTTPServer::ClearConnectedClients() m_connected.clear(); } -bool HTTPRemoteClient::ReadRequest(HTTPRequest& req) +void HTTPRemoteClient::ReadRequest(HTTPRequest& req) { + if (m_recv_buffer.empty()) return; + LineReader reader(m_recv_buffer, MAX_HEADERS_SIZE); - if (!req.LoadControlData(reader)) return false; - if (!req.LoadHeaders(reader)) return false; - if (!req.LoadBody(reader)) return false; + try { + switch (req.GetState()) { + case HTTPRequest::State::Init: + if (!req.LoadControlData(reader)) break; + req.SetState(HTTPRequest::State::NeedsHeaders); + [[fallthrough]]; + + case HTTPRequest::State::NeedsHeaders: + if (!req.LoadHeaders(reader)) break; + req.SetState(HTTPRequest::State::NeedsBody); + [[fallthrough]]; + + case HTTPRequest::State::NeedsBody: + if (!req.LoadBody(reader)) break; + req.SetState(HTTPRequest::State::Complete); + [[fallthrough]]; + + case HTTPRequest::State::Complete: + break; + + case HTTPRequest::State::Error: + break; + } + } catch (...) { + // Don't try to read any more data for this request + req.SetState(HTTPRequest::State::Error); + // Clear the memory allocated to this client, caller must disconnect + m_recv_buffer.clear(); + throw; + } // Remove the bytes read out of the buffer. - // If one of the above calls throws an error, the caller must - // catch it and disconnect the client. m_recv_buffer.erase( m_recv_buffer.begin(), m_recv_buffer.begin() + reader.Consumed()); - - return true; } bool HTTPRemoteClient::MaybeSendBytesFromBuffer() diff --git a/src/httpserver.h b/src/httpserver.h index d92b2054442..d8f07c127d2 100644 --- a/src/httpserver.h +++ b/src/httpserver.h @@ -121,6 +121,9 @@ private: * https://httpwg.org/specs/rfc9110.html#rfc.section.5.2 */ std::vector> m_headers; + + //! Track total bytes consumed in Read() for limit checks + size_t m_consumed{0}; }; struct HTTPVersion { @@ -196,6 +199,27 @@ public: std::pair GetHeader(std::string_view hdr) const; std::string ReadBody() const { return m_body; } void WriteHeader(std::string&& hdr, std::string&& value); + + enum class State { + Init, + NeedsHeaders, + NeedsBody, + Complete, + Error + }; + State GetState() const { return m_state; } + void SetState(State state) { m_state = state; } + + // If a large request is sent with "Transfer-encoding: chunked" we may + // read the chunk size in a separate I/O loop iteration than the chunk + // of data itself. Store the chunk size value here until the chunk is read. + std::optional m_chunk_size; + // We may also read a large chunk over multiple loop iterations. + // Track the progress of the chunk here. + uint64_t m_chunk_read{0}; + +private: + State m_state = State::Init; }; class HTTPServer @@ -563,10 +587,11 @@ public: /** * Try to read an HTTP request from the receive buffer. + * Updates HTTPRequest.m_state and drains buffer on error. * @param[in] req A HTTPRequest to read into - * @returns true upon reading a complete request, otherwise false (may throw). + * @throws std::runtime_error if request is unreadable or violates protocol */ - bool ReadRequest(HTTPRequest& req); + void ReadRequest(HTTPRequest& req); /** * Push data (if there is any) from client's m_send_buffer to the connected socket. diff --git a/src/test/httpserver_tests.cpp b/src/test/httpserver_tests.cpp index c03b3e41f78..9916e84ad7a 100644 --- a/src/test/httpserver_tests.cpp +++ b/src/test/httpserver_tests.cpp @@ -484,27 +484,6 @@ BOOST_AUTO_TEST_CASE(http_request_tests) BOOST_CHECK(req.LoadHeaders(reader)); BOOST_CHECK_EXCEPTION(req.LoadBody(reader), std::runtime_error, HasReason{"Improperly terminated chunk"}); } - { - // End of buffer reached without chunk termination, caller must wait for more data to arrive - HTTPRequest req; - std::string delayed_chunked = "GET / HTTP/1.0\n" - "Transfer-Encoding: chunked\n" - "\n" - "10\n" - R"({"method":"getbl)""\n" - "a\n" - R"(ockcount"})"; - LineReader reader1(delayed_chunked, MAX_HEADERS_SIZE); - BOOST_CHECK(req.LoadControlData(reader1)); - BOOST_CHECK(req.LoadHeaders(reader1)); - BOOST_CHECK(!req.LoadBody(reader1)); - // more data arrives! - delayed_chunked += "\n0\n\n"; - LineReader reader2(delayed_chunked, MAX_HEADERS_SIZE); - BOOST_CHECK(req.LoadControlData(reader2)); - BOOST_CHECK(req.LoadHeaders(reader2)); - BOOST_CHECK(req.LoadBody(reader2)); - } } BOOST_AUTO_TEST_CASE(http_server_socket_tests) diff --git a/test/functional/interface_http.py b/test/functional/interface_http.py index b0c5243f9a8..13500152588 100755 --- a/test/functional/interface_http.py +++ b/test/functional/interface_http.py @@ -9,6 +9,8 @@ from test_framework.netutil import NETWORK_ERRORS from test_framework.util import assert_equal, str_to_b64str import http.client +import socket +import threading import time import urllib.parse @@ -222,23 +224,37 @@ class HTTPBasicsTest (BitcoinTestFramework): assert_equal(response4.status, http.client.OK) conn = BitcoinHTTPConnection(self.node) - try: - # Excessive body size is invalid - conn.post_raw('/', f'{{"jsonrpc": "2.0", "id": "0", "method": "submitblock", "params": ["{"0" * bytes_above_limit}"]}}') - self.log.info("Client finished sending request before connection was terminated") - except NETWORK_ERRORS: - self.log.info("Client did not finish sending request before connection was terminated") - # The server will send a 413 response and disconnect but due to a race - # condition, the python client may or may not read the response before - # detecting the broken socket (which it may still be trying to write to). + # Split off the send into a background thread. When the server detects + # the excessive size it will stop reading from the socket, but the client + # will continue trying to write until the backpressure eventually + # drops the TCP window size to 0. While the send operation is blocking until + # it times out, we can still receive the server's response in the foreground. + + def send_excessive_body(self, conn): + try: + # Excessive body size is invalid + conn.post_raw('/', f'{{"jsonrpc": "2.0", "id": "0", "method": "submitblock", "params": ["{"0" * bytes_above_limit}"]}}') + # On some platforms (e.g. Windows) the whole request may be + # accepted into the OS send buffer before the server disconnects. + # It's ok to allow that, the server-side behavior is asserted in + # the foreground thread via the 413 response. + self.log.info("Client finished sending request before connection was terminated") + except NETWORK_ERRORS: + self.log.info("Client did not finish sending request before connection was terminated") + + send_thread = threading.Thread(target=send_excessive_body, args=(self, conn)) + send_thread.start() + + response5 = conn.recv_raw().decode() + assert "413 Content too large" in response5 + try: - response5 = conn.conn.getresponse() - assert_equal(response5.status, http.client.REQUEST_ENTITY_TOO_LARGE) - self.log.info(f"Client got expected response status {response5.status}") - assert conn.sock_closed() - except NETWORK_ERRORS: - self.log.info("Client did not read response before disconnecting") + conn.conn.sock.shutdown(socket.SHUT_RDWR) + self.log.info("Send thread force-closed by test framework") + except OSError: + self.log.info("Send thread was already closed by RST from server") + send_thread.join() def check_pipelining(self): @@ -314,27 +330,41 @@ class HTTPBasicsTest (BitcoinTestFramework): b'3' * 10000000, b'"]}' ] - try: - conn.conn.request( - method='POST', - url='/', - body=iter(body_chunked), - headers=headers_chunked, - encode_chunked=True) - self.log.info("Client finished sending request before connection was terminated") - except NETWORK_ERRORS: - self.log.info("Client did not finish sending request before connection was terminated") - # The server will send a 413 response and disconnect but due to a race - # condition, the python client may or may not read the response before - # detecting the broken socket (which it may still be trying to write to). + # Split off the send into a background thread. When the server detects + # the excessive size it will stop reading from the socket, but the client + # will continue trying to write until the backpressure eventually + # drops the TCP window size to 0. While the send operation is blocking until + # it times out, we can still receive the server's response in the foreground. + + def send_excessive_chunked(self, conn): + try: + conn.conn.request( + method='POST', + url='/', + body=iter(body_chunked), + headers=headers_chunked, + encode_chunked=True) + # On some platforms (e.g. Windows) the whole request may be + # accepted into the OS send buffer before the server disconnects. + # It's ok to allow that, the server-side behavior is asserted in + # the foreground thread via the 413 response. + self.log.info("Client finished sending request before connection was terminated") + except NETWORK_ERRORS: + self.log.info("Client did not finish sending request before connection was terminated") + + send_thread = threading.Thread(target=send_excessive_chunked, args=(self, conn)) + send_thread.start() + + response2 = conn.recv_raw().decode() + assert "413 Content too large" in response2 + try: - response2 = conn.conn.getresponse() - assert_equal(response2.status, http.client.REQUEST_ENTITY_TOO_LARGE) - self.log.info(f"Client got expected response status {response2.status}") - assert conn.sock_closed() - except NETWORK_ERRORS: - self.log.info("Client did not read response before disconnecting") + conn.conn.sock.shutdown(socket.SHUT_RDWR) + self.log.info("Send thread force-closed by test framework") + except OSError: + self.log.info("Send thread was already closed by RST from server") + send_thread.join() def check_idle_timeout(self): From c7db3ae1f907a10748e608cd69899c7586bfc210 Mon Sep 17 00:00:00 2001 From: Matthew Zipkin Date: Tue, 21 Jul 2026 14:57:44 -0400 Subject: [PATCH 4/5] test: cover HTTPRequest state machine --- src/test/httpserver_tests.cpp | 337 ++++++++++++++++++++++++++++++++++ 1 file changed, 337 insertions(+) diff --git a/src/test/httpserver_tests.cpp b/src/test/httpserver_tests.cpp index 9916e84ad7a..15a7872f799 100644 --- a/src/test/httpserver_tests.cpp +++ b/src/test/httpserver_tests.cpp @@ -486,6 +486,343 @@ BOOST_AUTO_TEST_CASE(http_request_tests) } } +BOOST_AUTO_TEST_CASE(http_request_state_tests) +{ + // For these tests we just need a receive buffer for the requests to read from. + class DummyClient : public HTTPRemoteClient + { + public: + DummyClient() : HTTPRemoteClient{/*id=*/0, /*addr=*/CService(), /*socket=*/CreateSock(0, 0, 0)} {} + + void receive(std::string_view s) + { + m_recv_buffer.insert( + m_recv_buffer.end(), + s.begin(), + s.end()); + } + }; + + { + // Step through state machine + std::shared_ptr client{std::make_shared()}; + client->m_req = std::make_unique(client); + BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::Init); + + client->receive("POST / HTTP/1.0\n"); + client->ReadRequest(*client->m_req); + BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::NeedsHeaders); + + client->receive("Host: 127.0.0.1\n" + "Content-Length: 10\n\n"); + client->ReadRequest(*client->m_req); + BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::NeedsBody); + + client->receive("I miss you\n"); + client->ReadRequest(*client->m_req); + BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::Complete); + + // m_req holds a shared_ptr back to the client, so break the cycle + // before the client goes out of scope (as the server does on disconnect). + client->ReleaseRequest(); + } + { + // Read body over multiple data pushes, multiple requests in same push + std::shared_ptr client{std::make_shared()}; + client->m_req = std::make_unique(client); + BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::Init); + + client->receive("POST / HTTP/1.0\n" + "Host: 127.0.0.1\n" + "Content-Length: 10\n\n" + "I miss"); + client->ReadRequest(*client->m_req); + // Because of the Content-Length header we know the body is not complete + BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::NeedsBody); + + // Finish sending first request and include second request in the same buffer + client->receive(" you" + "GET /endpoint HTTP/1.0\n\n"); + client->ReadRequest(*client->m_req); + BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::Complete); + BOOST_CHECK_EQUAL(client->m_req->m_body, "I miss you"); + // Next request sitting in buffer + BOOST_CHECK_EQUAL(client->m_recv_buffer.size(), 24); + // Complete first request hasn't been moved yet, expect no-op + client->ReadRequest(*client->m_req); + BOOST_CHECK_EQUAL(client->m_recv_buffer.size(), 24); + + // Reset m_req + client->m_req = std::make_unique(client); + BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::Init); + + // Read second request + client->ReadRequest(*client->m_req); + BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::Complete); + BOOST_CHECK_EQUAL(client->m_req->m_target, "/endpoint"); + BOOST_CHECK_EQUAL(client->m_req->m_body.size(), 0); + // Buffer is cleared + BOOST_CHECK_EQUAL(client->m_recv_buffer.size(), 0); + + client->ReleaseRequest(); + } + { + // A Content-Length body is drained out of the receive buffer as it + // arrives, instead of accumulating there until the request is complete. + + std::shared_ptr client{std::make_shared()}; + client->m_req = std::make_unique(client); + BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::Init); + + client->receive("POST / HTTP/1.0\n" + "Content-Length: 30000\n\n"); + client->ReadRequest(*client->m_req); + BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::NeedsBody); + + // Body arrives in 10kB pieces. Each one is copied onto m_body and + // erased from the receive buffer, which never holds more than one piece. + for (int i = 1; i <= 3; ++i) { + client->receive(std::string(10000, 'x')); + BOOST_CHECK_EQUAL(client->m_recv_buffer.size(), 10000); + client->ReadRequest(*client->m_req); + BOOST_CHECK_EQUAL(client->m_req->m_body.size(), 10000 * i); + BOOST_CHECK_EQUAL(client->m_recv_buffer.size(), 0); + } + BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::Complete); + + client->ReleaseRequest(); + } + { + // A body sent in the same push as the next request is split correctly + std::shared_ptr client{std::make_shared()}; + client->m_req = std::make_unique(client); + BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::Init); + + client->receive("POST / HTTP/1.0\n" + "Content-Length: 4\n\n" + "body" + "GET /next HTTP/1.0\n\n"); + client->ReadRequest(*client->m_req); + BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::Complete); + BOOST_CHECK_EQUAL(client->m_req->m_body, "body"); + // Only the second request is left over + BOOST_CHECK_EQUAL(client->m_recv_buffer.size(), 20); + + client->ReleaseRequest(); + } + { + // Chunked transfer with state + std::shared_ptr client{std::make_shared()}; + client->m_req = std::make_unique(client); + + BOOST_CHECK(!client->m_req->m_chunk_size); + BOOST_CHECK_EQUAL(client->m_req->m_chunk_read, 0); + BOOST_CHECK_EQUAL(client->m_req->m_body.size(), 0); + BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::Init); + + // First chunk is incomplete + client->receive("GET / HTTP/1.0\n" + "Transfer-Encoding: chunked\n" + "\n" + "10\n" + R"({"method)"); + client->ReadRequest(*client->m_req); + BOOST_CHECK(client->m_req->m_chunk_size); + BOOST_CHECK_EQUAL(*client->m_req->m_chunk_size, 16); + BOOST_CHECK_EQUAL(client->m_req->m_chunk_read, 8); + BOOST_CHECK_EQUAL(client->m_req->m_body.size(), 8); + BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::NeedsBody); + + // More data arrives, chunk is completed. + client->receive(R"(":"getbl)""\n"); + client->ReadRequest(*client->m_req); + // State is reset + BOOST_CHECK(!client->m_req->m_chunk_size); + BOOST_CHECK_EQUAL(client->m_req->m_chunk_read, 0); + // New data is added to body but body is still incomplete + BOOST_CHECK_EQUAL(client->m_req->m_body.size(), 16); + BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::NeedsBody); + + // Next chunk arrives without terminal CRLF + client->receive("a\n" + R"(ockcount"})"); + client->ReadRequest(*client->m_req); + BOOST_CHECK(client->m_req->m_chunk_size); + BOOST_CHECK_EQUAL(*client->m_req->m_chunk_size, 10); + BOOST_CHECK_EQUAL(client->m_req->m_chunk_read, 10); + BOOST_CHECK_EQUAL(client->m_req->m_body.size(), 26); + BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::NeedsBody); + + // Chunk terminal CRLF arrives with final (size 0) chunk + client->receive("\n0\n\n"); + client->ReadRequest(*client->m_req); + // Body size hasn't changed + BOOST_CHECK_EQUAL(client->m_req->m_body.size(), 26); + // We're done + BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::Complete); + BOOST_CHECK_EQUAL(client->m_req->m_body, R"({"method":"getblockcount"})"); + + client->ReleaseRequest(); + } + { + // Invalid headers: error state stops reading + std::shared_ptr client{std::make_shared()}; + client->m_req = std::make_unique(client); + + // Request is in the buffer + client->receive("POST / HTTP/1.0\n" + "Host: 127.0.0.1\n" + "Invalid header with no colon\n" + "\n" + "body is not read"); + BOOST_CHECK(!client->m_recv_buffer.empty()); + + // Reading throws an error, sets state + BOOST_CHECK_EXCEPTION(client->ReadRequest(*client->m_req), + std::runtime_error, + HasReason{"HTTP header missing colon (:)"}); + BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::Error); + + // We read up to the invalid line + BOOST_CHECK_EQUAL(*client->m_req->m_headers.FindFirst("Host"), "127.0.0.1"); + // Buffer was cleared, client should just be disconnected now + BOOST_CHECK(client->m_recv_buffer.empty()); + + // Even if more data comes in, trying to read again in error state is a no-op + client->receive("Content-Length: 2\n\nok"); + BOOST_CHECK_EQUAL(client->m_recv_buffer.size(), 21); + client->ReadRequest(*client->m_req); + BOOST_CHECK_EQUAL(client->m_recv_buffer.size(), 21); + + client->ReleaseRequest(); + } + { + // Headers sent in batches that are below MAX_HEADERS_SIZE but the total is excessive + std::shared_ptr client{std::make_shared()}; + client->m_req = std::make_unique(client); + client->ReadRequest(*client->m_req); + BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::Init); + + client->receive("POST /huge HTTP/1.0\n"); + client->ReadRequest(*client->m_req); + BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::NeedsHeaders); + + for (int i = 0; i < 410; ++i) { + client->receive("key:value\n"); + } + client->ReadRequest(*client->m_req); + BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::NeedsHeaders); + + for (int i = 0; i < 409; ++i) { + client->receive("key:value\n"); + } + client->ReadRequest(*client->m_req); + BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::NeedsHeaders); + + // We're at 819 x 10-byte headers + // The limit is 8192, three more bytes should throw. + client->receive("k:\n"); + BOOST_CHECK_EXCEPTION(client->ReadRequest(*client->m_req), + std::runtime_error, + HasReason{"HTTP headers exceed size limit"}); + BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::Error); + + client->ReleaseRequest(); + } + { + // Client sends chunks that are below the limit but the total is excessive + std::shared_ptr client{std::make_shared()}; + client->m_req = std::make_unique(client); + client->ReadRequest(*client->m_req); + BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::Init); + + client->receive("POST /huge HTTP/1.0\n"); + client->ReadRequest(*client->m_req); + BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::NeedsHeaders); + + client->receive("Transfer-Encoding: chunked\n\n"); + client->ReadRequest(*client->m_req); + BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::NeedsBody); + + // Send 16-byte chunk + client->receive("10\nno auto updates!\n"); + client->ReadRequest(*client->m_req); + BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::NeedsBody); + + // The next chunk will be of size 32MiB - 16 + 1, below the limit + // on its own but not if it were added to the total cumulative body so far. + // We don't need to actually send or prepare this amount of data. + client->receive("1fffff1\n"); + BOOST_CHECK_EXCEPTION(client->ReadRequest(*client->m_req), + http_bitcoin::ContentTooLargeError, + HasReason{"Chunk will exceed max body size"}); + BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::Error); + + client->ReleaseRequest(); + } + { + // Ensure chunk trailer is parsed over state lines + std::shared_ptr client{std::make_shared()}; + client->m_req = std::make_unique(client); + + // Send a 1-byte chunk then send the 0-chunk with a trailer but no terminal CRLF + client->receive("GET / HTTP/1.0\n" + "Transfer-Encoding: chunked\n" + "\n" + "1\n" + "x\n" + "0\n" + "Digest: sha-4=deadbeef\n"); + client->ReadRequest(*client->m_req); + BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::NeedsBody); + + // Send first part of another trailer line + client->receive("Expires:"); + client->ReadRequest(*client->m_req); + BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::NeedsBody); + + // Finish the trailer line + client->receive("never\n"); + client->ReadRequest(*client->m_req); + BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::NeedsBody); + + // Terminate + client->receive("\n"); + client->ReadRequest(*client->m_req); + BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::Complete); + BOOST_CHECK_EQUAL(client->m_req->m_body, "x"); + + client->ReleaseRequest(); + } + { + // Ensure chunk trailer counts towards the headers size limit + std::shared_ptr client{std::make_shared()}; + client->m_req = std::make_unique(client); + + client->receive("POST /huge HTTP/1.0\n" + "Transfer-Encoding: chunked\n"); // 27 bytes + for (int i = 0; i < 816; ++i) { + client->receive("key:value\n"); // 8160 + } + client->receive("\n" // 1 + "1\n" + "x\n" + "0\n"); + client->ReadRequest(*client->m_req); + BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::NeedsBody); + + // We're in the trailer section with a total of 8188 bytes of headers. + // The limit is 8192, five more bytes should throw. + client->receive("k:vv\n"); + BOOST_CHECK_EXCEPTION(client->ReadRequest(*client->m_req), + std::runtime_error, + HasReason{"HTTP headers exceed size limit"}); + BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::Error); + + client->ReleaseRequest(); + } +} + BOOST_AUTO_TEST_CASE(http_server_socket_tests) { // Hard code the timestamp for the Date header in the HTTP response From 9954aa77280ecd67816e784815c6478a973f6635 Mon Sep 17 00:00:00 2001 From: Matthew Zipkin Date: Thu, 6 Aug 2026 17:23:44 -0400 Subject: [PATCH 5/5] http: don't parse any new requests from a client if m_req_busy = true --- src/httpserver.cpp | 12 ++++++------ test/functional/interface_http.py | 32 ++++++++++++++++++++++++------- 2 files changed, 31 insertions(+), 13 deletions(-) diff --git a/src/httpserver.cpp b/src/httpserver.cpp index 16215170141..d72e9d9fd7e 100644 --- a/src/httpserver.cpp +++ b/src/httpserver.cpp @@ -1016,6 +1016,11 @@ void HTTPServer::ThreadSocketHandler() void HTTPServer::MaybeDispatchRequestsFromClient(const std::shared_ptr& client) const { + // If we are already handling a request from + // this client, do nothing. We'll check again on the next I/O + // loop iteration. + if (client->m_req_busy) return; + if (!client->m_req) { client->m_req = std::make_unique(client); } @@ -1048,12 +1053,7 @@ void HTTPServer::MaybeDispatchRequestsFromClient(const std::shared_ptrm_req_busy) return; - - // Otherwise, if the request is ready, hand it to a worker. + // If the request is ready, hand it to a worker. if (client->m_req->GetState() == HTTPRequest::State::Complete) { LogDebug( BCLog::HTTP, diff --git a/test/functional/interface_http.py b/test/functional/interface_http.py index 13500152588..3e5d1832fb0 100755 --- a/test/functional/interface_http.py +++ b/test/functional/interface_http.py @@ -116,7 +116,8 @@ class HTTPBasicsTest (BitcoinTestFramework): self.check_keepalive_connection() self.check_close_connection() self.check_excessive_request_size() - self.check_pipelining() + self.check_pipelining(with_invalid_second_request=False) + self.check_pipelining(with_invalid_second_request=True) self.check_chunked_transfer() self.check_idle_timeout() self.check_server_busy_idle_timeout() @@ -257,12 +258,12 @@ class HTTPBasicsTest (BitcoinTestFramework): send_thread.join() - def check_pipelining(self): + def check_pipelining(self, with_invalid_second_request): """ Requests are responded to in the order in which they were received See https://www.rfc-editor.org/rfc/rfc7230#section-6.3.2 """ - self.log.info("Check pipelining") + self.log.info("Check pipelining" + (" with invalid second request" if with_invalid_second_request else "")) tip_height = self.node.getblockcount() conn = BitcoinHTTPConnection(self.node) conn.set_timeout(5) @@ -270,7 +271,10 @@ class HTTPBasicsTest (BitcoinTestFramework): # Send two requests in a row. # The first request will block the second indefinitely conn.post_raw('/', f'{{"method": "waitforblockheight", "params": [{tip_height + 1}]}}') - conn.post_raw('/', '{"method": "getblockcount"}') + if with_invalid_second_request: + conn.post_raw(f'/{"x" * MAX_HEADERS_SIZE * 2}', '{"method": "getblockcount"}') + else: + conn.post_raw('/', '{"method": "getblockcount"}') try: # The server should not respond to the second request until the first @@ -284,16 +288,30 @@ class HTTPBasicsTest (BitcoinTestFramework): # Use a separate http connection to generate a block self.generate(self.node, 1, sync_fun=self.no_op) - # Wait for two responses to be received + # Wait for responses to be received + if with_invalid_second_request: + OK = 1 + BAD = 1 + else: + OK = 2 + BAD = 0 res = b"" - while res.count(b"result") != 2: + while True: res += conn.recv_raw() + if res.count(b"HTTP/1.1 200") == OK and res.count(b"HTTP/1.1 400") == BAD: + break # waitforblockheight was responded to first, and then getblockcount # which includes the block added after the request was made chunks = res.split(b'"result":') assert chunks[1].startswith(b'{"hash":') - assert chunks[2].startswith(bytes(f'{tip_height + 1}', 'utf8')) + if with_invalid_second_request: + # The response to the in-flight first request is sent before the + # error generated by parsing the second one, even though the second + # request could have been rejected much earlier. + assert res.index(b"HTTP/1.1 200") < res.index(b"HTTP/1.1 400") + else: + assert chunks[2].startswith(bytes(f'{tip_height + 1}', 'utf8')) def check_chunked_transfer(self):