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):