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.
Replace the HTTP server's WorkQueue implementation and single threads
handling code with ThreadPool for processing HTTP requests. The
ThreadPool class encapsulates all this functionality on a reusable
class, properly unit and fuzz tested (the previous code was not
unit nor fuzz tested at all).
This cleanly separates responsibilities:
The HTTP server now focuses solely on receiving and dispatching requests,
while ThreadPool handles concurrency, queuing, and execution.
It simplifies init, shutdown and requests tracking.
This also allows us to experiment with further performance improvements at
the task queuing and execution level, such as a lock-free structure, task
prioritization or any other performance improvement in the future, without
having to deal with HTTP code that lives on a different layer.
3333bae9b2 tidy: modernize-use-equals-default (MarcoFalke)
Pull request description:
Prior to C++20, `modernize-use-equals-default` could have been problematic because it could turn a non-aggregate into an aggregate. The risk would be that aggregate initialization would be enabled where the author did not intend to enable it.
With C++20, aggregate for those is forbidden either way. (https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p1008r1.pdf)
So enabled it for code clarity, consistency, and possibly unlocking compiler optimizations. See https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-equals-default.html
ACKs for top commit:
stickies-v:
ACK 3333bae9b2
Tree-SHA512: ab42ff01be7ca7e7d8b4c6a485e68426f59627d83dd827cf292304829562348dc17a52ee009f5f6f3c1c2081d7166ffac4baef23197ebeba8de7767c6ddfe255
Pass HTTP server an interrupt object instead of having it depend on shutdown.h
and global shutdown state.
There is no change in behavior in this commit.
The removed code was intended to catch issues with event_enable_debug_logging which was not available prior to libevent 2.1.1. This is not necessary since the minimum libevent version was bumped to 2.1.8.
Adds the following util units and adds them to libbitcoin_util:
- `util/url.cpp` takes `urlDecode` from `httpserver.cpp`
- `util/error.cpp` takes `TransactionErrorString` from
`node/transaction.cpp` and `AmountHighWarn` and `AmountErrMsg` from
`ui_interface.cpp`
- `util/fees.cpp` takes `StringForFeeReason` and `FeeModeFromString` from `policy/fees.cpp`
- `util/rbf.cpp` takes `SignalsOptInRBF` from `policy/rbf.cpp`
- 'util/validation.cpp` takes `FormatStateMessage` and `strMessageMagic` from 'validation.cpp`
