http: throttle per-connection reads while a request is in flight #36123

pinheadmz commented at 9:00 PM on August 30, 2026: member

This patches a memory exhaustion scenario found while auditing the new http server with kimi-k3. A shallow version of this scenario was addressed in #35735 (See #35735 (review) and #35735 (comment)) but a OOM vector still remained.

On master when the sever is busy handling a request from a client, it will still read data from that client and "queue up" the next request. In #35735 we handled the scenario where that additional incoming data was an invalid HTTP request by not attempting to parse the data. However, we didn't add a size limit.

A misbehaving client could block its request queue with something like waitforblock and then flood the server with nonsense data without any limit.

The solution in this patch is to not even read from the socket at all if we are busy with a request. Similar to the intent of #35735, the kernel will buffer incoming data until backpressure kicks in and the TCP window drops to 0.

If unaddressed, the attack vector is still limited to authenticated clients: unauthenticated REST requests don't block for very long, so the server should be able to drain the receive buffer.

DrahtBot added the label RPC/REST/ZMQ on Aug 30, 2026

DrahtBot commented at 9:00 PM on August 30, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/36123.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
ACK	frankomosh, hodlinator, jeanpablojp, winterrdog, sedited

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

LLM Linter (✨ experimental)

Possible places where comparison-specific test macros should replace generic comparisons:

test/functional/interface_http.py assert sent <= len(flood) * 10, (...) -> prefer assert_greater_than_or_equal(len(flood) * 10, sent) for a comparison-specific helper.

pinheadmz commented at 9:01 PM on August 30, 2026: member

Requesting review from @frankomosh and @winterrdog who helped address the initial issue

DrahtBot added the label CI failed on Aug 30, 2026

pinheadmz force-pushed on Aug 30, 2026

pinheadmz commented at 11:37 PM on August 30, 2026: member

Trying another approach to de-flake-ify the test

in test/functional/interface_http.py:723 in 412e0c6e29

 718 | +        # one full request.
 719 | +        stuck_since = None
 720 | +        sent = 0
 721 | +        while True:
 722 | +            try:
 723 | +                sent += conn.conn.sock.send(flood)

hodlinator commented at 9:00 AM on August 31, 2026:

Since we are going through the trouble of actually creating a well formatted request, we might as well not garble the bytes:

                sent += conn.conn.sock.send(flood[sent % len(flood):])

in test/functional/interface_http.py:727 in 412e0c6e29

 722 | +            try:
 723 | +                sent += conn.conn.sock.send(flood)
 724 | +                # The server is still reading
 725 | +                stuck_since = None
 726 | +                self.log.debug(f"sent: {sent}")
 727 | +                assert sent <= len(flood), (

hodlinator commented at 10:26 AM on August 31, 2026:

Given the CI failures on Windows (https://github.com/bitcoin/bitcoin/actions/runs/33342301034/job/99340016146?pr=36123#step:14:1050), I think Windows has a differently sized TCP window (or possibly still buffers data on the client end?). Doubling the threshold fixed it for me during local testing. Maybe safer to go 10x.

                assert sent <= len(flood) * 2, (

in test/functional/interface_http.py:721 in 412e0c6e29 outdated

 716 | +        # nothing, so sends keep stalling; an unpatched server drains 64KB per
 717 | +        # I/O tick even while busy, so progress resumes and `sent` climbs past
 718 | +        # one full request.
 719 | +        stuck_since = None
 720 | +        sent = 0
 721 | +        while True:

hodlinator commented at 10:41 AM on August 31, 2026:

Would be good to set a bound on this loop, just in case it prevents the test from completing like in this Mac run: https://github.com/bitcoin/bitcoin/actions/runs/33342301034/job/99340016279?pr=36123

Something like:

--- a/test/functional/interface_http.py
+++ b/test/functional/interface_http.py
@@ -718,6 +718,7 @@ class HTTPBasicsTest (BitcoinTestFramework):
         # one full request.
         stuck_since = None
         sent = 0
+        start = time.time()
         while True:
             try:
                 sent += conn.conn.sock.send(flood)
@@ -740,6 +741,9 @@ class HTTPBasicsTest (BitcoinTestFramework):
                     # After 5 seconds of no progress we assume the server is
                     # behaving appropriately.
                     break
+            duration = time.time() - start
+            assert duration < 60 * self.options.timeout_factor, \
+                f"Failed to prove appropriate behavior after {duration:.0f} seconds."

         self.log.info(f"Pipelined flood stalled after {sent} bytes; no progress for 5s")

pinheadmz force-pushed on Aug 31, 2026

pinheadmz commented at 12:28 PM on August 31, 2026: member

push to 3e259fcc7d074212887a82c52b59250ee4ee5f58:

Took all suggestions from @hodlinator review, improving test and hopefully passing CI on windows. I reproduced the failed test and the fix on my own native windows machine. Still confused about the runaway macos test though since I wrote and tested the patch originally on macos!

winterrdog commented at 3:18 PM on August 31, 2026: contributor

Concept ACK

I like the idea of moving this kind of hard work away from the application layer, and then delegating it to the OS at the transport layer (usually the OS has handled a lot of strange edge cases over the years)

Will provide more review

pinheadmz force-pushed on Aug 31, 2026

pinheadmz commented at 7:30 PM on August 31, 2026: member

push to 782d195fc370456d0f54bb9178a1c9ba9829ed08

Try again to pass the test on all platforms. It now exits in three possible places during the blocked-flood:

~3 GB read into memory by the server (FAIL)
The server is reading/draining data from the buffer for 60 seconds (FAIL)
Neither of the above occur AND 5 seconds pass with the server blocking (PASS)

jeanpablojp commented at 12:50 AM on September 1, 2026: contributor

Concept ACK

With small pipelined requests m_recv_buffer takes in more each tick than it clears, since Receive() reads up to 64 KiB while TryReadRequest consumes at most one request. With -rest on, 16 connections with no credentials at all took the node from 46 MB to about 3 GB in a minute at the default -rpcmaxconnections, same as without this patch, so this patch didn't introduce it. Since REST takes no auth, the vector isn't limited to authenticated clients.

Does it make sense to cover that here, or is it better as a follow-up? Happy to open one.

http: throttle per-connection reads while a request is in flight

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.

3d1004cb9b

in src/httpserver.cpp:1015 in 782d195fc3

1011 | +            // While the client has a request being processed by a worker, don't
1012 | +            // read any more from the socket. Excess (pipelined) data then backs up
1013 | +            // in the kernel socket buffer, which applies TCP backpressure to the
1014 | +            // sender, instead of accumulating without bound in m_recv_buffer.
1015 | +            // Only one request per connection is ever in flight.
1016 | +            continue;

jeanpablojp commented at 12:50 AM on September 1, 2026:

One side effect of the skip is that the socket is no longer in the set the loop waits on, so nothing wakes it for that connection. On a keep-alive connection the median per request went from 0.14 ms without the patch to 53 ms here. The peer's FIN also stops showing up, so a client that closes mid-request ends up holding its connection slot for as long as the request runs.

net.cpp gates the same skip on a size threshold instead. Not a drop-in here, since TryReadRequest only runs for sockets left in the wait set, but worth a look in that direction?

pinheadmz commented at 3:11 PM on September 1, 2026:

Interesting, could you share any code you used to measure the connection? I'll look into gating it by size

jeanpablojp commented at 4:24 PM on September 1, 2026:

Sure, this is the latency one.

import base64, http.client, statistics, time
auth = base64.b64encode(b"user:pass").decode()
hdr = {"Authorization": "Basic " + auth}
c = http.client.HTTPConnection("127.0.0.1", 18443)
lat = []
for _ in range(20):
    t0 = time.perf_counter()
    c.request("POST", "/", '{"method":"getblockcount"}', hdr)
    r = c.getresponse()
    assert r.status == 200, r.status
    r.read()
    lat.append((time.perf_counter() - t0) * 1000)
print(statistics.median(lat))

Median comes out at 0.5 ms on the merge base and 51 ms here. The 0.14 above was the same measurement over a raw socket, without http.client's overhead. The FIN and slot side takes a bit more setup, happy to paste that one too if you want.

On the macOS job, I don't think the test is what hangs it. rpc_blockchain.py goes from 39 s to 108 s here and rpc_rawtransaction.py from 8 s to 18 s, which is the 50 ms landing on every call, since AuthServiceProxy holds one connection open. Your fork run had --filter=interface, so it would not have shown that.

winterrdog commented at 4:45 PM on September 1, 2026:

look into gating it by size

i've always wondered why we don't cap the size of the application receive buffer (m_recv_buffer) and then let the rest of the data that doesn’t fit yet stay in the kernel buffers in the meantime. @pinheadmz any tradeoffs in doing this?

in test/functional/interface_http.py:747 in 782d195fc3 outdated

 742 | +                # feeling backpressure from the server or the client-side buffer.
 743 | +                if stuck_since is None:
 744 | +                    stuck_since = time.monotonic()
 745 | +                elif time.monotonic() - stuck_since > STALL_TIMEOUT:
 746 | +                    # No progress: the server has stopped reading.
 747 | +                    break

jeanpablojp commented at 12:50 AM on September 1, 2026:

On EAGAIN the loop spends the five seconds calling send() in a tight spin. Waiting for writability does exactly the same job and drops the cost from 5.8 s to 0.9 s of CPU here, with the same wall time. Needs import select at the top.

            except BlockingIOError:
                # The kernel send buffer is full (EAGAIN).
                # That's good, but we still need to determine if we are
                # feeling backpressure from the server or the client-side buffer.
                if stuck_since is None:
                    stuck_since = time.monotonic()
                remaining = STALL_TIMEOUT - (time.monotonic() - stuck_since)
                if remaining <= 0:
                    # No progress: the server has stopped reading.
                    break
                # Sleep until the socket is writable again instead of spinning.
                select.select([], [conn.conn.sock], [], remaining)

winterrdog commented at 12:40 PM on September 1, 2026:

same wall-clock time either way, right ?

just wondering if it is worth adding the extra machinery to save some CPU on a test that only runs occasionally. feels like keeping this one simple might be preferable here.

🤔

hodlinator commented at 1:22 PM on September 1, 2026:

Some kind of sleep()/yield() could maybe have a similar same effect but require less domain knowledge? Probably best to avoid a hot loop either way IMO.

jeanpablojp commented at 2:32 PM on September 1, 2026:

Yes, same wall time here, about 7.5 s in all of them. CPU is 5.9 s as is, 1.3 s with a sleep(0.05) and 0.9 s with the select, and the sleep version still fails without the patch.

So @hodlinator is right, that gets most of the way with less machinery. @winterrdog Fair point that it's a small thing, though a one-line sleep is cheap. Sounds good to me.

pinheadmz commented at 3:13 PM on September 1, 2026:

I played with a short sleep here (50 ms makes sense since thats SELECT_TIMEOUT) I am also hoping that helps with the macos ci job hanging for hours -- I wasn't able to reproduce that locally or even on a narrowed-down CI job on my fork

pinheadmz force-pushed on Sep 1, 2026

pinheadmz commented at 8:02 PM on September 1, 2026: member

push to 3d1004cb9b8f27bd328d95b4c7524e878c296d61

Rewrote the throttling mechanism, using m_req as the major key: if it exists, there is an incomplete request in progress and we should continue to read bytes to finish it. If it's missing, that means we have moved the previous request to a worker and have not read any new bytes yet. In this case we will read from the socket only if m_recv_buffer is currently empty.

In addition, replaced the continue with a 0 event -- so even a busy client still gets passed to TryReadRequest() and what that does is try to parse a request already waiting in m_recv_buffer thus closing the 50ms gap noticed by @jeanpablojp

I haven't explicitly addressed the REST issue yet it's possible this last update rolled it up but I still want to test it out.

DrahtBot removed the label CI failed on Sep 1, 2026

jeanpablojp commented at 9:10 PM on September 1, 2026: contributor

it's possible this last update rolled it up but I still want to test it out

It did, in my tests. Peak m_recv_buffer is 0.1 MB at every request size from 200 B to 1 MiB, where the previous head was 24 MB at 200 B. Sixteen unauthenticated REST connections move RSS by 3 MB over 90 s, against 3.2 GB before.

The 50 ms and the held connection slot are both gone here too.

in src/httpserver.cpp:1009 in 3d1004cb9b

1002 | @@ -1003,7 +1003,20 @@ HTTPServer::IOReadiness HTTPServer::GenerateWaitSockets() const
1003 |          // never hold m_sock_mutex and m_send_mutex at the same time here.
1004 |          // MaybeSendBytesFromBuffer() locks m_send_mutex then m_sock_mutex, so nesting
1005 |          // them in the opposite order here would risk a lock-order inversion deadlock.
1006 | -        Sock::Event event = (http_client->ReadyToSend() ? Sock::SendEvent : Sock::RecvEvent);
1007 | +        Sock::Event event{0};
1008 | +        if (http_client->ReadyToSend()) {
1009 | +            event = Sock::SendEvent;
1010 | +        } else if (http_client->GetRequest() != nullptr || http_client->ReceiveBufferEmpty()) {

frankomosh commented at 9:38 AM on September 3, 2026:

nit: should we have more unit tests, especially covering this half of the condition?

frankomosh commented at 9:40 AM on September 3, 2026: contributor

ACK 3d1004cb9b8f27bd328d95b4c7524e878c296d61

Built a small, deterministic test to check the bug this PR fixes, and ran it on both master and this branch.

On master: dispatched one request to a worker and kept it busy, then sent 5 MiB of garbage on the same connection. All 5 MiB ended up in memory (m_recv_buffer), as there is no limit.
On this branch, same exact steps as above: the flood stopped growing at exactly 65,536 bytes, which I believe is one Receive() call's worth, and never went past that, no matter how long the worker stayed busy.

DrahtBot requested review from winterrdog on Sep 3, 2026

DrahtBot requested review from jeanpablojp on Sep 3, 2026

in test/functional/interface_http.py:725 in 3d1004cb9b

 720 | +        # If send() is blocked for this many seconds, we assume the server
 721 | +        # is behaving correctly.
 722 | +        STALL_TIMEOUT = 5
 723 | +        # If send() continues to progress for this many seconds, we assume
 724 | +        # the server is vulnerable to memory exhaustion.
 725 | +        PROGRESS_TIMEOUT = 10

hodlinator commented at 11:27 AM on September 3, 2026:

nit: Might make the tests more robust to scale at least the latter of these two constants by self.options.timeout_factor as suggested in #36123 (review). Maybe with some upper bound so they don't go fully 40x as on CI.

hodlinator approved

hodlinator commented at 12:13 PM on September 3, 2026: contributor

ACK 3d1004cb9b8f27bd328d95b4c7524e878c296d61

Makes sense to only read from the remote HTTP client's socket:

While we are parsing a request which may need more data (or is waiting to be dispatched to a worker), or
When our receive buffer is empty. If we get any new data it will cause a new request to begin being parsed - unless we hit the m_req_busy guard in HTTPRemoteClient::TryReadRequest() because the client's worker is busy with an earlier request.

This effectively has the reading from the socket pause with the beginning of the next request in the buffer while we are waiting for a client's worker thread to finish processing the previous request.

Reading more eagerly than that may lead to the HTTP server RAM consumption ballooning as pointed out by previous reviewers.

Tested by reverting the httpserver.cpp change and observing the new functional test case failing as a consequence.

Would be nice to include in v32, though it's not critical as the HTTP interface shouldn't be exposed publicly.

jeanpablojp commented at 12:58 PM on September 3, 2026: contributor

tACK 3d1004cb9b8f27bd328d95b4c7524e878c296d61

Retested after the rewrite.

nit: if you push again, the commit message still carries this line.

Stop selecting RecvEvent for clients whose request is being processed

A busy client with an empty buffer now falls into m_recv_buffer.empty() and does get RecvEvent, which is what makes the peer's FIN visible again, so that line reads like the earlier version.

fanquake added this to the milestone 32.0 on Sep 3, 2026

winterrdog commented at 1:34 PM on September 3, 2026: contributor

tACK 3d1004cb9b8f27bd328d95b4c7524e878c296d61

successfully built and tested on NetBSD 11.0/g++ 12.5.0/x86_64

pinheadmz force-pushed on Sep 4, 2026

pinheadmz commented at 5:47 PM on September 4, 2026: member

Pushed draft work to this branch by mistake, but reverted so current reviews are still on HEAD. sorry!

DrahtBot added the label CI failed on Sep 4, 2026

DrahtBot removed the label CI failed on Sep 4, 2026

sedited approved

sedited commented at 3:08 PM on September 5, 2026: contributor

ACK 3d1004cb9b8f27bd328d95b4c7524e878c296d61

Did not spend too much time with the introduced functional test, but tried this out with a manual test.

sedited merged this on Sep 5, 2026

sedited closed this on Sep 5, 2026

janb84 commented at 6:21 PM on September 5, 2026: contributor

Post merge, ACK 3d1004cb9b8f27bd328d95b4c7524e878c296d61

The PR fixes a memory exhaustion scenario in a way that is comparable to what NGINX undertakes.

One point of note, this PR does reduce streaming pipelined throughput on an idle node (from 383 to 18.9 requests per second). I could not find any client that would hit this slow path, so I do not think it matters.
In order for the slowdown to happen, the pipelining connection needs to be the node's only traffic. One (non-streaming pipelined ) client brings it back to 600 to 965 requests per second.

As far as I can tell this is a side-effect and not a designed defence. If it is, it needs some more documentation.

pinheadmz commented at 12:47 PM on September 6, 2026: member

@janb84

Not sure if I'd call that a side effect, or the intended effect. You're pushing hundreds of requests per second into the server and the new behavior is to only read one buffer-load (65kB) per 50ms-I/O loop tick. All the requests in that load can be handled within one tick, and the next requests have to wait in the socket.

Then my guess is, when you add a second client it wakes up the I/O loop before the 50ms SELECT_TIMEOUT, and the queue gets processed more quickly.

I'd expect that during your 300 request/s trial on master the memory consumption was much higher than on the branch, and that's the exact tradeoff I want to make with this PR.

Kino1994 referenced this in commit 8ac3615c46 on Sep 6, 2026

janb84 commented at 8:40 AM on September 7, 2026: contributor

@pinheadmz Not to make this a big back-and-forward, that is not my intent.

Not sure if I'd call that a side effect, or the intended effect. You're pushing hundreds of requests per second into the server and the new behavior is to only read one buffer-load (65kB) per 50ms-I/O loop tick. All the requests in that load can be handled within one tick, and the next requests have to wait in the socket.

I think this is a bit more nuanced, and in that lies what I wanted to convey. It's not "one buffer-load per tick with all requests in the load handled in that tick", it's one request per tick, each paying a full poll() timeout. This matters because thats the part that matters for, if the per-50ms ceiling is a side-effect (imho) or needed for the solution. (I think you can lift the ceiling without touching the memory bound, e.g. wake the poll when the worker clears m_req_busy )

To be clear, I'm not arguing against the bound. This PR clearly fixes the memory exhaustion.

pinheadmz commented at 11:07 AM on September 7, 2026: member

wake the poll when the worker clears m_req_busy

Gotcha now, yeah I think you're right and @theuni proposed a strategy for this during #35182 and the implementation of "optimistic send" which we can still do in the future: We add our own internal socket pair to the poll list, and whenever we need to wake up before the full 50 ms timeout, you push one byte across the pair so it reports an event and triggers the rest of SocketHandlerConnected()