A client streaming pipelined requests into a busy connection
(or any connection whose replies are slower than the sender) could grow
server memory without limit, up to remote OOM.
Stop selecting RecvEvent for clients whose request is being processed;
pipelined data then backs up in the kernel socket buffer, applying TCP
backpressure to the sender. One request per connection is in flight
at a time.
Functional test streams pipelined submitblock requests into a connection
blocked on waitforblockheight. Unpatched builds continue draining the
socket buffer indefinitely, patched builds will stall.
5e0d7a286a refactor: Drastically narrow scope of http_bitcoin namespace and rename it to bitcoin_http (Hodlinator)
8f9fd8698a refactor: Make HTTPRemoteClient fields private (Hodlinator)
d72f67fd6c refactor: Expose additional HTTPRemoteClient fields through accessors (Hodlinator)
10bbae302f refactor: Expose HTTPRemoteClient fields to tests through methods (Hodlinator)
5b06d90831 refactor: Replace HTTPServer::MaybeDispatchRequestsFromClient() with HTTPRemoteClient::TryReadRequest() (Hodlinator)
a1183c02aa refactor: Extract Send() and Receive() into HTTPRemoteClient from HTTPServer (Hodlinator)
6d9b61d4f8 refactor: Extract HTTPRemoteClient::MaybeDisconnect() from HTTPServer::DisconnectClients() (Hodlinator)
6fec8d6914 refactor: Make HTTPRequest fields private (Hodlinator)
b8cd77237b refactor: Make HTTPRequest::GetHeader() return saner optional type (Hodlinator)
e5be0dc35e refactor: Make HTTPResponse a struct since all fields are public (Hodlinator)
Pull request description:
The new HTTP server implementation in v32 has `HTTPServer` reaching into and modifying fields of `HTTPRemoteClient` and `HTTPRequest`. This PR encapsulates field data of the latter 2 types which enforces invariants and reduces cognitive load[^1]. Exposing data through accessor methods also implies adding lock annotations.
Commits:
* Makes `HTTPResponse` a struct since it is used that way. (https://github.com/bitcoin/bitcoin/pull/35182#discussion_r3336757663) [^2]
* `HTTPRequest`:
* Saner return type for `GetHeader()` (old type was mirroring the now removed libevent-wrapper and made later commits ugly).
* Make fields private.
* Simplifies boolean logic in `HTTPServer::DisconnectClients()`. (https://github.com/bitcoin/bitcoin/pull/35182#discussion_r3336757663)
* Extraction of `HTTPServer` functions into `HTTPRemoteClient`:
Refactors `HTTPRemoteClient` to be more self-contained rather than having `HTTPServer` reach into the fields of other objects. (https://github.com/bitcoin/bitcoin/pull/35182#discussion_r3339543447, https://github.com/bitcoin/bitcoin/pull/35182#discussion_r3339543447)
* Severely narrows `http_bitcoin` namespace and renames it to `bitcoin_http` (https://github.com/bitcoin/bitcoin/pull/35182#discussion_r3264510816)
Follow-up to #35182.
[^1]: Core Guidelines: C.9: Minimize exposure of members - https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c9-minimize-exposure-of-members
[^2]: Core Guidelines: C.2: Use class if the class has an invariant; use struct if the data members can vary independently - https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c2-use-class-if-the-class-has-an-invariant-use-struct-if-the-data-members-can-vary-independently
ACKs for top commit:
achow101:
ACK 5e0d7a286a
janb84:
ACK 5e0d7a286a
winterrdog:
tACK 5e0d7a286a
Tree-SHA512: e1c5aa067538e31247ca74923e451038c90750ccc941ae16711dd976c8cd750bd1afaee6e4378aeee91440f7727955d9bfb32aa25a0a745613d0d771a674ebc8
http_bitcoin was mostly used during #35182 to distinguish from http_libevent counterpart:
- The http_libevent namespace was introduced around the legacy code in 89c54ae4cb.
- The http_bitcoin namespace was introduced in 68b5d289d1 and extended in subsequent commits.
- The http_libevent namespace together with code it contained was removed in 8c1eea0777.
bitcoin_http is a better name as it is Bitcoin Core's implementation of the HTTP protocol, not HTTP protocol's implementation of bitcoin 402 payment required codes or anything like that.
The namespace only remains for a few constants and a type which don't have HTTP in their names.
bd4b1524ea init: do not count file descriptors for HTTPServer if -server=0 (Matthew Zipkin)
b08662060d init: account for maximum file descriptors needed by HTTP (Matthew Zipkin)
cc2acebefb http: configure simultaneous connection limit with -rpcmaxconnections (Matthew Zipkin)
b3d6d2d1a7 http: limit connected clients to 16 (Matthew Zipkin)
86651d8197 scripted-diff: Rename nUserBind, nBind, nMaxConnections to snake_case (Matthew Zipkin)
Pull request description:
Introduces a new configuration option `-rpcmaxconnections` with default value `16`. This is used to limit the number of simultaneous `HTTPClient` connected to the `HTTPServer`. When the limit is reached, new pending connections remain queued in the kernel's socket buffer. Those connections have complete TCP handshakes with the kernel but do not occupy any application memory.
The previous libevent-based HTTP server had no limit on connections but it did have a limit on the kernel socket queue:
e7ff4ef2b4/http.c (L3510)
```c
if (listen(fd, 128) == -1) {
```
The current HTTP server, like the p2p server, uses a platform constant here:
b6becf3534/src/httpserver.cpp (L743)
(on my macOS `SOMAXCONN` is `128` but on my Debian machine it's `4096`)
The default of 16 was chosen as a reasonable upper bound for single-user RPC use cases. Systems designed to handle more simultaneous HTTP connections than this (previously relying on the absence of a limit) can adjust the setting.
## File descriptors
Because of the connection limit, we can now account for the maximum number of file descriptors needed by the HTTP server. This addresses several issues (#11368#11322 maybe #27732) that could have been fixed by a PR waiting in vain for a libevent release (#27731).
## Bonus performance improvement
The new limit is managed in a loop that drains the kernel's socket queue with `accept()`. All pending connections from the queue (up to the limit) are processed in one single call to `SocketHandlerListening()`. The previous code would only accept one connection from the queue on each I/O loop tick, with a `SELECT_TIMEOUT` (50ms) sleep between each.
ACKs for top commit:
fjahr:
tACK bd4b1524ea
janb84:
ACK bd4b1524ea
winterrdog:
tested ACK bd4b1524ea
hodlinator:
Concept ACK bd4b1524ea
willcl-ark:
ACK bd4b1524ea
Tree-SHA512: 2ef7a96da4d7037c7343ec0ea03fda5bb55d10c2a071fce4929141297515923b203d3d338dbcb6599849768f52aa3c9da509fb5d1d6f7c574a1d2034ea2a9e74
This removes the need for HTTPClient::ReleaseRequest() as the client<->request cycle is broken. Not having to remember to call ReleaseRequest() reduces cognitive load.
9954aa7728 http: don't parse any new requests from a client if m_req_busy = true (Matthew Zipkin)
c7db3ae1f9 test: cover HTTPRequest state machine (Matthew Zipkin)
90676e24ad Add state to HTTPRequest to avoid duplicate work over I/O cycles (Matthew Zipkin)
507e528e84 http: reuse HTTPHeaders to parse chunked trailer (Matthew Zipkin)
902d8908c9 http: only read one HTTPRequest at a time per client (Matthew Zipkin)
Pull request description:
This PR reduces the memory consumption of the HTTP Server when reading data from connected clients, and improves performance especially when requests are large (i.e. requiring multiple TCP packets).
In https://github.com/bitcoin/bitcoin/pull/35182 the server copies as much data as it can from the socket into application memory, and then tries to parse as many complete HTTP requests as possible from that data. If a request is discovered to be incomplete, the in-progress request is abandoned. The server tries again on the next I/O cycle to read the same data from the buffer, duplicating work as many times as it takes before the client finishes sending the request (or times out).
This PR implements two improvements to this:
1. Only parse one request at a time from the receive buffer. The server processes requests from each client in series anyway.
2. Add state to `HTTPRequest` so it can be filled with data from the receive buffer over multiple I/O loop iterations without losing progress.
If a client sends large or multiple requests, that data will sit in the kernel's socket buffer instead of the application memory. Eventually the socket buffer will fill up and TCP backpressure will kick in, dropping the TCP window to 0 and blocking the client from sending any more.
A state machine for `HTTPRemoteClient` was [discussed previously](https://github.com/bitcoin/bitcoin/pull/35182#pullrequestreview-4322490068) to control resource consumption. Another nice benefit of this model (for a follow-up PR) will be to insert the RPC authentication check after reading 8kB-limited headers but before the 32MB-limited request body.
ACKs for top commit:
winterrdog:
re-ACK 9954aa7728
janb84:
re ACK 9954aa7728
frankomosh:
ACK 9954aa7728.
fjahr:
ACK 9954aa7728
Tree-SHA512: b7c913114283fbf1f360b40f6c65a01390a26731bf3b166f460ec260f9206f25d738b3a06887bfa839911c1c6aaf634448181da47a752a9a881aebd907e44868
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.
Both are identical since C++17 and this refactor shouldn't change any
behavior. The benefits are consistency and to be explicit, to avoid
confusion with the C++11/14 constexpr.
Co-Authored-By: l0rinc <pap.lorinc@gmail.com>
-BEGIN VERIFY SCRIPT-
sed -i --regexp-extended 's/^constexpr \S+ \w+(\[\])? ?[={]/inline &/' $( \
git grep -l '^constexpr ' -- \
'*.h' \
':(exclude)src/minisketch' \
)
-END VERIFY SCRIPT-
Both are fine and this refactor shouldn't change any behavior.
However, inline constexpr will ensure each symbol has a single address
across all TU, making the release binary smaller.
Note, a follow-up commit will deal with string literals (const char*)
and other static const, which can not be constexpr (e.g. std::vector).
-BEGIN VERIFY SCRIPT-
# Limit to types that can be constexpr
type='bool|CAmount|size_t|((signed|unsigned) )?int|u?int[0-9]+_t|std::array|DatabaseFormat|CFeeRate|std::streamsize'
sed -i --regexp-extended "s/^(static )?const (${type})\>/inline constexpr \2/" $( \
git grep -l --extended-regexp "^(static )?const " -- \
'*.h' \
':(exclude)src/leveldb' \
':(exclude)src/secp256k1' \
)
-END VERIFY SCRIPT-
55d3cd51a4 doc: add release note describing change for forbidden clients (Matthew Zipkin)
d1ed2a6e25 http: check rpcallowip immediately after accepting connection (Matthew Zipkin)
Pull request description:
This is a follow-up to #35182 addressing a review comment from that PR: https://github.com/bitcoin/bitcoin/pull/35182#pullrequestreview-4322490068
This update to HTTPServer checks the IP subnet allowlist as soon as possible (immediately after receiving a connection from a client) before any data is received. This does not entirely protect the server from the "slow loris" attack or [CWE-400](https://cwe.mitre.org/data/definitions/400.html) but does restrict the attack surface to localhost and clients explicitly allowed by the user.
If a client is not allowed by the list, we disconnect as soon as possible. This is a behavior change from master branch (and previous release with libevent) where `403 Forbidden` was returned (after a potentially large amount request data was written to memory by the server).
To facilitate existing unit tests, this commit includes a refactor that moves the subnet allow list and relevant methods into the HTTPServer class instead of static file scope. This is needed because otherwise the allow list would be empty when the unit tests run.
There is still plenty of refactoring to do in order to modernize `HTTPServer` and de-globalize it, but since this specific issue has a resource allocation guard, I wanted to open it quickly on its own.
ACKs for top commit:
janb84:
ACK 55d3cd51a4
winterrdog:
ACK 55d3cd51a4
w0xlt:
ACK 55d3cd51a4
fjahr:
Code review ACK 55d3cd51a4
Tree-SHA512: 545911f2e4d2f97ab8bc854e9e57c39eb896428f8c349d34c8e8025a1f6bfb8cfd436f381e36af8b87592c07df3e16210819f3eef7943e23c6626030e615fdf5
Instead of sending 403 Forbidden, disconnect as soon as possible.
To facilitate unit testing, this commit includes a refactor
that moves the subnet allow list and relevant methods
into the HTTPServer class instead of file-scope static scope.
This prevents a losing race condition that could prevent the server
from reading requests from an HTTP client.
A connected socket can either be written to or read from based on the
result of GenerateWaitSockets(). That method checks the HTTPRemoteClient
flag m_send_ready. If it's `true` the implication is that there is
data in the client's send buffer ready to go. Once that data is sent
and the buffer is empty, MaybeSendBytesFromBuffer() sets it `false` again.
The sad case was when a worker thread calling WriteReply() adds
data to the send buffer, but before it sets m_send_ready to `true`,
the I/O thread sends that data and empties the buffer. With the
buffer unexpectedly empty, WriteReply() sets m_send_ready to `true`.
The effect of this is that the socket will stay in "write" mode
with nothing to write. With nothing to write, MaybeSendBytesFromBuffer()
never sets it back to `false` and the socket is stuck forever.
This is a refactor to prepare for matching the API of HTTPRequest
definitions in both namespaces http_bitcoin and http_libevent. In
particular, to provide a consistent return type for GetRequestMethod()
in both classes.
See https://www.rfc-editor.org/rfc/rfc7230#section-6.3.2
> A server MAY process a sequence of pipelined requests in
parallel if they all have safe methods (Section 4.2.1 of [RFC7231]),
but it MUST send the corresponding responses in the same order that
the requests were received.
We choose NOT to process requests in parallel. They are executed in
the order recevied as well as responded to in the order received.
This prevents race conditions where old state may get sent in response
to requests that are very quick to process but were requested later on
in the queue.
Sockets-touching bits copied and adapted from `CConnman::SocketSendData()`
Testing this requires adding a new feature to the SocketTestingSetup,
returning the DynSock I/O pipes from the mock socket so the received
data can be checked.
Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
`SocketHandlerConnected()` adapted from CConnman
Testing this requires adding a new feature to the SocketTestingSetup,
inserting a "request" payload into the mock client that connects
to us.
This commit also moves IOErrorIsPermanent() from sock.cpp to sock.h
so it can be called from the socket handler in httpserver.cpp
Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
Socket handling methods are copied from CConnMan:
`CConnman::GenerateWaitSockets()`
`CConnman::SocketHandlerListening()`
`CConnman::ThreadSocketHandler()` and `CConnman::SocketHandler()` are combined into ThreadSocketHandler()`.
Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
Introduce a new low-level socket managing class `HTTPServer`.
BindAndStartListening() was copied from CConnMan's BindListenPort()
in net.cpp and modernized.
Unit-test it with a new class `SocketTestingSetup` which mocks
`CreateSock()` and will enable mock client I/O in future commits.
Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
This commit is a no-op to isolate HTTP methods and objects that
depend on libevent. Following commits will add replacement objects
and methods in a new namespace for testing and review before
switching over the server.
Add ExecuteHTTPRPC to provide a way to execute an HTTP request without relying
on HTTPRequest and libevent types.
Behavior is not changing in any way, this is just moving code. This commit may
be easiest to review using git's --color-moved option.
