Thanks for pointing out that HTTPRemoteClient is the only one which needs that modifier!
Hesitant to give full friend access though. Maybe some kind of passkey pattern could be explored (https://github.com/bitcoin/bitcoin/pull/35301#discussion_r3820529374 https://chromium.googlesource.com/chromium/src.git/+/master/docs/patterns/passkey.md).
Alternatively one could friend the specific method friend void HTTPRemoteClient::ReadRequest(HTTPRequest&);, but that involves solving problems with changing declaration-order of classes.
Or one could remove HTTPRequest::SetState() in favor of extracting most of HTTPRemoteClient::ReadRequest() into a new HTTPRequest::Load(util::LineReader& reader).
<details><summary>draft diff</summary>
diff --git a/src/httpserver.cpp b/src/httpserver.cpp
index 0d93fd34cb..bb8a382f2a 100644
--- a/src/httpserver.cpp
+++ b/src/httpserver.cpp
@@ -1163,42 +1163,48 @@ void HTTPRemoteClient::ReadRequest(HTTPRequest& req)
if (m_recv_buffer.empty()) return;
LineReader reader(m_recv_buffer, MAX_HEADERS_SIZE);
+ try {
+ req.Load(reader);
+ } catch (...) {
+ // Clear the memory allocated to this client, caller must disconnect
+ m_recv_buffer.clear();
+ throw;
+ }
+
+ // Remove the bytes read out of the buffer.
+ m_recv_buffer.erase(
+ m_recv_buffer.begin(),
+ m_recv_buffer.begin() + reader.Consumed());
+}
+bool HTTPRequest::Load(util::LineReader& reader)
+{
try {
- switch (req.GetState()) {
+ switch (m_state) {
case HTTPRequest::State::Init:
- if (!req.LoadControlData(reader)) break;
- req.SetState(HTTPRequest::State::NeedsHeaders);
+ if (!LoadControlData(reader)) return false;
+ m_state = HTTPRequest::State::NeedsHeaders;
[[fallthrough]];
case HTTPRequest::State::NeedsHeaders:
- if (!req.LoadHeaders(reader)) break;
- req.SetState(HTTPRequest::State::NeedsBody);
+ if (!LoadHeaders(reader)) return false;
+ m_state = HTTPRequest::State::NeedsBody;
[[fallthrough]];
case HTTPRequest::State::NeedsBody:
- if (!req.LoadBody(reader)) break;
- req.SetState(HTTPRequest::State::Complete);
+ if (!LoadBody(reader)) return false;
+ m_state = HTTPRequest::State::Complete;
[[fallthrough]];
case HTTPRequest::State::Complete:
- break;
-
case HTTPRequest::State::Error:
- break;
+ return true;
}
} 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();
+ m_state = HTTPRequest::State::Error;
throw;
}
-
- // Remove the bytes read out of the buffer.
- m_recv_buffer.erase(
- m_recv_buffer.begin(),
- m_recv_buffer.begin() + reader.Consumed());
}
bool HTTPRemoteClient::MaybeSendBytesFromBuffer()
diff --git a/src/httpserver.h b/src/httpserver.h
index 9a41838102..8f19437627 100644
--- a/src/httpserver.h
+++ b/src/httpserver.h
@@ -153,18 +153,11 @@ public:
explicit HTTPRequest() : m_client{} {}
/**
- * Methods that attempt to parse HTTP request fields line-by-line
- * from a receive buffer.
- * [@param](/bitcoin-bitcoin/contributor/param/)[in] reader A LineReader object constructed over a span of data.
- * [@returns](/bitcoin-bitcoin/contributor/returns/) true If the request field was parsed.
- * false If there was not enough data in the buffer to complete the field.
+ * [@returns](/bitcoin-bitcoin/contributor/returns/) true If the request was fully parsed or in error.
+ * false If there was not enough data in the buffer.
* [@throws](/bitcoin-bitcoin/contributor/throws/) std::runtime_error if data is invalid.
*/
- /// @{
- bool LoadControlData(util::LineReader& reader);
- bool LoadHeaders(util::LineReader& reader);
- bool LoadBody(util::LineReader& reader);
- /// @}
+ bool Load(util::LineReader& reader);
void WriteReply(HTTPStatusCode status, std::span<const std::byte> reply_body = {});
void WriteReply(HTTPStatusCode status, std::string_view reply_body_view)
@@ -195,9 +188,22 @@ public:
Error
};
State GetState() const { return m_state; }
- void SetState(State state) { m_state = state; }
private:
+ /**
+ * Methods that attempt to parse HTTP request fields line-by-line
+ * from a receive buffer.
+ * [@param](/bitcoin-bitcoin/contributor/param/)[in] reader A LineReader object constructed over a span of data.
+ * [@returns](/bitcoin-bitcoin/contributor/returns/) true If the request field was parsed.
+ * false If there was not enough data in the buffer to complete the field.
+ * [@throws](/bitcoin-bitcoin/contributor/throws/) std::runtime_error if data is invalid.
+ */
+ /// @{
+ bool LoadControlData(util::LineReader& reader);
+ bool LoadHeaders(util::LineReader& reader);
+ bool LoadBody(util::LineReader& reader);
+ /// @}
+
HTTPRequestMethod m_method;
std::string m_target;
HTTPVersion m_version;
diff --git a/src/test/fuzz/http_request.cpp b/src/test/fuzz/http_request.cpp
index c24413add0..5314339a92 100644
--- a/src/test/fuzz/http_request.cpp
+++ b/src/test/fuzz/http_request.cpp
@@ -29,9 +29,7 @@ FUZZ_TARGET(http_request)
HTTPRequest http_request;
LineReader reader(http_buffer, MAX_HEADERS_SIZE);
try {
- if (!http_request.LoadControlData(reader)) return;
- if (!http_request.LoadHeaders(reader)) return;
- if (!http_request.LoadBody(reader)) return;
+ http_request.Load(reader);
} catch (const std::runtime_error&) {
return;
}
diff --git a/src/test/httpserver_tests.cpp b/src/test/httpserver_tests.cpp
index f853a2d01c..8364aa641e 100644
--- a/src/test/httpserver_tests.cpp
+++ b/src/test/httpserver_tests.cpp
@@ -196,9 +196,7 @@ BOOST_AUTO_TEST_CASE(http_request_tests)
{
HTTPRequest req;
LineReader reader(full_request, MAX_HEADERS_SIZE);
- BOOST_CHECK(req.LoadControlData(reader));
- BOOST_CHECK(req.LoadHeaders(reader));
- BOOST_CHECK(req.LoadBody(reader));
+ BOOST_CHECK(req.Load(reader));
BOOST_CHECK_EQUAL(req.GetRequestMethod(), HTTPRequestMethod::POST);
BOOST_CHECK_EQUAL(req.GetURI(), "/");
BOOST_CHECK_EQUAL(req.GetVersion().major, 1);
@@ -214,61 +212,61 @@ BOOST_AUTO_TEST_CASE(http_request_tests)
// Malformed: no spaces between data
HTTPRequest req;
LineReader reader("GET/HTTP/1.0\r\nHost: 127.0.0.1\r\n\r\n", MAX_HEADERS_SIZE);
- BOOST_CHECK_EXCEPTION(req.LoadControlData(reader), std::runtime_error, HasReason{"HTTP request line too short"});
+ BOOST_CHECK_EXCEPTION(req.Load(reader), std::runtime_error, HasReason{"HTTP request line too short"});
}
{
// Malformed: too many spaces
HTTPRequest req;
LineReader reader("GET / HTTP / 1.0\r\nHost: 127.0.0.1\r\n\r\n", MAX_HEADERS_SIZE);
- BOOST_CHECK_EXCEPTION(req.LoadControlData(reader), std::runtime_error, HasReason{"HTTP request line malformed"});
+ BOOST_CHECK_EXCEPTION(req.Load(reader), std::runtime_error, HasReason{"HTTP request line malformed"});
}
{
// Malformed: slash missing before version
HTTPRequest req;
LineReader reader("GET / HTTP1.0\r\nHost: 127.0.0.1\r\n\r\n", MAX_HEADERS_SIZE);
- BOOST_CHECK_EXCEPTION(req.LoadControlData(reader), std::runtime_error, HasReason{"HTTP request line too short"});
+ BOOST_CHECK_EXCEPTION(req.Load(reader), std::runtime_error, HasReason{"HTTP request line too short"});
}
{
// Malformed: no decimal in version
HTTPRequest req;
LineReader reader("GET / HTTP/11\r\nHost: 127.0.0.1\r\n\r\n", MAX_HEADERS_SIZE);
- BOOST_CHECK_EXCEPTION(req.LoadControlData(reader), std::runtime_error, HasReason{"HTTP request line too short"});
+ BOOST_CHECK_EXCEPTION(req.Load(reader), std::runtime_error, HasReason{"HTTP request line too short"});
}
{
// Malformed: version is not a number
HTTPRequest req;
LineReader reader("GET / HTTP/1.x\r\nHost: 127.0.0.1\r\n\r\n", MAX_HEADERS_SIZE);
- BOOST_CHECK_EXCEPTION(req.LoadControlData(reader), std::runtime_error, HasReason{"HTTP bad version"});
+ BOOST_CHECK_EXCEPTION(req.Load(reader), std::runtime_error, HasReason{"HTTP bad version"});
}
{
// Malformed: version is out of range
HTTPRequest req;
LineReader reader("GET / HTTP/2.0\r\nHost: 127.0.0.1\r\n\r\n", MAX_HEADERS_SIZE);
- BOOST_CHECK_EXCEPTION(req.LoadControlData(reader), std::runtime_error, HasReason{"HTTP bad version"});
+ BOOST_CHECK_EXCEPTION(req.Load(reader), std::runtime_error, HasReason{"HTTP bad version"});
}
{
// Malformed: version is out of range
HTTPRequest req;
LineReader reader("GET / HTTP/0.9\r\nHost: 127.0.0.1\r\n\r\n", MAX_HEADERS_SIZE);
- BOOST_CHECK_EXCEPTION(req.LoadControlData(reader), std::runtime_error, HasReason{"HTTP bad version"});
+ BOOST_CHECK_EXCEPTION(req.Load(reader), std::runtime_error, HasReason{"HTTP bad version"});
}
{
// Malformed: version is out of range
HTTPRequest req;
LineReader reader("GET / HTTP/-1.0\r\nHost: 127.0.0.1\r\n\r\n", MAX_HEADERS_SIZE);
- BOOST_CHECK_EXCEPTION(req.LoadControlData(reader), std::runtime_error, HasReason{"HTTP bad version"});
+ BOOST_CHECK_EXCEPTION(req.Load(reader), std::runtime_error, HasReason{"HTTP bad version"});
}
{
// Malformed: version is not exactly two integers and a dot
HTTPRequest req;
LineReader reader("GET / HTTP/1.00\r\nHost: 127.0.0.1\r\n\r\n", MAX_HEADERS_SIZE);
- BOOST_CHECK_EXCEPTION(req.LoadControlData(reader), std::runtime_error, HasReason{"HTTP bad version"});
+ BOOST_CHECK_EXCEPTION(req.Load(reader), std::runtime_error, HasReason{"HTTP bad version"});
}
{
// Malformed: contains NUL
HTTPRequest req;
LineReader reader{std::string_view{"GET /safe\0/etc/passwd HTTP/1.00\r\nHost: 127.0.0.1\r\n\r\n", 50}, MAX_HEADERS_SIZE};
- BOOST_CHECK_EXCEPTION(req.LoadControlData(reader), std::runtime_error, HasReason{"Invalid request line contains NUL"});
+ BOOST_CHECK_EXCEPTION(req.Load(reader), std::runtime_error, HasReason{"Invalid request line contains NUL"});
}
{
// Malformed: differing Content-Length values, case insensitive
@@ -279,9 +277,7 @@ BOOST_AUTO_TEST_CASE(http_request_tests)
"12345678";
HTTPRequest req;
util::LineReader reader{differing_length, /*max_line_length=*/MAX_HEADERS_SIZE};
- BOOST_CHECK(req.LoadControlData(reader));
- BOOST_CHECK(req.LoadHeaders(reader));
- BOOST_CHECK_EXCEPTION(req.LoadBody(reader), std::runtime_error, HasReason{"Differing Content-Length values"});
+ BOOST_CHECK_EXCEPTION(req.Load(reader), std::runtime_error, HasReason{"Differing Content-Length values"});
}
{
// Ok: multiple same Content-Length values
@@ -292,17 +288,13 @@ BOOST_AUTO_TEST_CASE(http_request_tests)
"12345678";
HTTPRequest req;
util::LineReader reader{differing_length, /*max_line_length=*/MAX_HEADERS_SIZE};
- BOOST_CHECK(req.LoadControlData(reader));
- BOOST_CHECK(req.LoadHeaders(reader));
- BOOST_CHECK(req.LoadBody(reader));
+ BOOST_CHECK(req.Load(reader));
}
{
// Ok
HTTPRequest req;
LineReader reader("GET / HTTP/1.0\r\nHost: 127.0.0.1\r\n\r\n", MAX_HEADERS_SIZE);
- BOOST_CHECK(req.LoadControlData(reader));
- BOOST_CHECK(req.LoadHeaders(reader));
- BOOST_CHECK(req.LoadBody(reader));
+ BOOST_CHECK(req.Load(reader));
BOOST_CHECK_EQUAL(req.GetRequestMethod(), HTTPRequestMethod::GET);
BOOST_CHECK_EQUAL(req.GetURI(), "/");
BOOST_CHECK_EQUAL(req.GetVersion().major, 1);
@@ -315,8 +307,7 @@ BOOST_AUTO_TEST_CASE(http_request_tests)
// Malformed: missing colon
HTTPRequest req;
LineReader reader("GET / HTTP/1.0\r\nHost=127.0.0.1\r\n\r\n", MAX_HEADERS_SIZE);
- BOOST_CHECK(req.LoadControlData(reader));
- BOOST_CHECK_EXCEPTION(req.LoadHeaders(reader), std::runtime_error, HasReason{"HTTP header missing colon (:)"});
+ BOOST_CHECK_EXCEPTION(req.Load(reader), std::runtime_error, HasReason{"HTTP header missing colon (:)"});
}
{
// We might not have received enough data from the client which is not
@@ -324,16 +315,13 @@ BOOST_AUTO_TEST_CASE(http_request_tests)
// buffer has more data.
HTTPRequest req;
LineReader reader("GET / HTTP/1.0\r\nHost: ", MAX_HEADERS_SIZE);
- BOOST_CHECK(req.LoadControlData(reader));
- BOOST_CHECK(!req.LoadHeaders(reader));
+ BOOST_CHECK(!req.Load(reader));
}
{
// No Content-Length: body is not read
HTTPRequest req;
LineReader reader("GET / HTTP/1.0\r\n\r\n" R"({"method":"getblockcount"})", MAX_HEADERS_SIZE);
- BOOST_CHECK(req.LoadControlData(reader));
- BOOST_CHECK(req.LoadHeaders(reader));
- BOOST_CHECK(req.LoadBody(reader));
+ BOOST_CHECK(req.Load(reader));
// Don't try to read request body if Content-Length is missing
BOOST_CHECK_EQUAL(req.ReadBody(), "");
}
@@ -341,17 +329,13 @@ BOOST_AUTO_TEST_CASE(http_request_tests)
// Malformed: Content-Length is not a number
HTTPRequest req;
LineReader reader("GET / HTTP/1.0\r\nContent-Length: eleven\r\n\r\n" R"({"method":"getblockcount"})", MAX_HEADERS_SIZE);
- BOOST_CHECK(req.LoadControlData(reader));
- BOOST_CHECK(req.LoadHeaders(reader));
- BOOST_CHECK_EXCEPTION(req.LoadBody(reader), std::runtime_error, HasReason{"Cannot parse Content-Length value"});
+ BOOST_CHECK_EXCEPTION(req.Load(reader), std::runtime_error, HasReason{"Cannot parse Content-Length value"});
}
{
// Malformed: Content-Length is negative
HTTPRequest req;
LineReader reader("GET / HTTP/1.0\r\nContent-Length: -8\r\n\r\n" R"({"method":"getblockcount"})", MAX_HEADERS_SIZE);
- BOOST_CHECK(req.LoadControlData(reader));
- BOOST_CHECK(req.LoadHeaders(reader));
- BOOST_CHECK_EXCEPTION(req.LoadBody(reader), std::runtime_error, HasReason{"Cannot parse Content-Length value"});
+ BOOST_CHECK_EXCEPTION(req.Load(reader), std::runtime_error, HasReason{"Cannot parse Content-Length value"});
}
{
// Content-Length exceeds limit
@@ -360,9 +344,7 @@ BOOST_AUTO_TEST_CASE(http_request_tests)
const std::string request{"GET / HTTP/1.0\r\nContent-Length: " + util::ToString(excessive_size) + "\r\n\r\n" + std::move(huge_body)};
HTTPRequest req;
LineReader reader(request, MAX_HEADERS_SIZE);
- BOOST_CHECK(req.LoadControlData(reader));
- BOOST_CHECK(req.LoadHeaders(reader));
- BOOST_CHECK_EXCEPTION(req.LoadBody(reader), ContentTooLargeError, HasReason{"Max body size exceeded"});
+ BOOST_CHECK_EXCEPTION(req.Load(reader), ContentTooLargeError, HasReason{"Max body size exceeded"});
}
{
// Content-Length exactly on the limit
@@ -370,18 +352,14 @@ BOOST_AUTO_TEST_CASE(http_request_tests)
const std::string request{"GET / HTTP/1.0\r\nContent-Length: " + util::ToString(MAX_BODY_SIZE) + "\r\n\r\n" + std::move(max_body)};
HTTPRequest req;
LineReader reader(request, MAX_HEADERS_SIZE);
- BOOST_CHECK(req.LoadControlData(reader));
- BOOST_CHECK(req.LoadHeaders(reader));
- BOOST_CHECK(req.LoadBody(reader));
+ BOOST_CHECK(req.Load(reader));
}
{
// Content-Length indicates more data than we have in the buffer.
// Not an error; we wait for more data before completing the body.
HTTPRequest req;
LineReader reader("GET / HTTP/1.0\r\nContent-Length: 1024\r\n\r\n" R"({"method":"getblockcount"})", MAX_HEADERS_SIZE);
- BOOST_CHECK(req.LoadControlData(reader));
- BOOST_CHECK(req.LoadHeaders(reader));
- BOOST_CHECK(!req.LoadBody(reader));
+ BOOST_CHECK(!req.Load(reader));
}
{
// Support "chunked" transfer. Chunk lengths are ascii-encoded hex integers, whitespace ignored
@@ -396,9 +374,7 @@ BOOST_AUTO_TEST_CASE(http_request_tests)
"0\n"
"\n";
LineReader reader(ok_chunked, MAX_HEADERS_SIZE);
- BOOST_CHECK(req.LoadControlData(reader));
- BOOST_CHECK(req.LoadHeaders(reader));
- BOOST_CHECK(req.LoadBody(reader));
+ BOOST_CHECK(req.Load(reader));
BOOST_CHECK_EQUAL(req.ReadBody(), R"({"method":"getblockcount"})");
}
{
@@ -414,9 +390,7 @@ BOOST_AUTO_TEST_CASE(http_request_tests)
"0\n"
"\n";
LineReader reader(excessive_chunk_size, MAX_HEADERS_SIZE);
- BOOST_CHECK(req.LoadControlData(reader));
- BOOST_CHECK(req.LoadHeaders(reader));
- BOOST_CHECK_EXCEPTION(req.LoadBody(reader), ContentTooLargeError, HasReason{"Chunk will exceed max body size"});
+ BOOST_CHECK_EXCEPTION(req.Load(reader), ContentTooLargeError, HasReason{"Chunk will exceed max body size"});
}
{
// Allow (but ignore) Chunk Extensions
@@ -432,9 +406,7 @@ BOOST_AUTO_TEST_CASE(http_request_tests)
"Expires: Wed, 21 Oct 2026 07:28:00 GMT\n"
"\n";
LineReader reader(ok_chunked, MAX_HEADERS_SIZE);
- BOOST_CHECK(req.LoadControlData(reader));
- BOOST_CHECK(req.LoadHeaders(reader));
- BOOST_CHECK(req.LoadBody(reader));
+ BOOST_CHECK(req.Load(reader));
BOOST_CHECK_EQUAL(req.ReadBody(), R"({"method":"getblockcount"})");
// Chunk Trailer was parsed, but ignored
BOOST_CHECK_EQUAL(reader.Remaining(), 0);
@@ -453,9 +425,7 @@ BOOST_AUTO_TEST_CASE(http_request_tests)
"0\n"
"\n";
LineReader reader(invalid_chunked, MAX_HEADERS_SIZE);
- BOOST_CHECK(req.LoadControlData(reader));
- BOOST_CHECK(req.LoadHeaders(reader));
- BOOST_CHECK_EXCEPTION(req.LoadBody(reader), std::runtime_error, HasReason{"Cannot parse chunk length value"});
+ BOOST_CHECK_EXCEPTION(req.Load(reader), std::runtime_error, HasReason{"Cannot parse chunk length value"});
}
{
// Invalid "chunked" transfer, missing chunk termination \n
@@ -470,9 +440,7 @@ BOOST_AUTO_TEST_CASE(http_request_tests)
"0\n"
"\n";
LineReader reader(invalid_chunked, MAX_HEADERS_SIZE);
- BOOST_CHECK(req.LoadControlData(reader));
- BOOST_CHECK(req.LoadHeaders(reader));
- BOOST_CHECK_EXCEPTION(req.LoadBody(reader), std::runtime_error, HasReason{"Improperly terminated chunk"});
+ BOOST_CHECK_EXCEPTION(req.Load(reader), std::runtime_error, HasReason{"Improperly terminated chunk"});
}
}
</details>
Deferring for now.
