← Watch feed
Merge bitcoin/bitcoin#36299: cli: Improve empty-response and fix -rpcclienttimeout regression
Public commit record
100/100 · Strong
What the developer wrote
Authored by Ava Chow
Merge bitcoin/bitcoin#36299: cli: Improve empty-response and fix -rpcclienttimeout regression
1805716354b49ecb96e49b949a61e2c72ad2c1a3 cli: Apply -rpcclienttimeout per socket wait instead of per phase (Fabian Jahr)
b841ab694215699cba53c4de8a7cc08b9fce259a cli: Treat a response with Content-Length: 0 as complete (Fabian Jahr)
Pull request description:
Two follow-ups to #34342
First commit: An empty body was not treated as a complete response. We currently check if `Content-Length` is greater than zero instead of whether the header was sent, so a response with a `Content-Length` of 0 falls into the branch for responses that carry no length and confinues to read until the peer disconnects. Our server sends an empty body with several types of errors such as a wrong RPC password but it does close the connection as well, which mitigates this from causing any serious issue. However, it would still be good to handle this correctly on the client side that we don't have to rely on the server to save us from hanging.
b-l-u-e found this in post-merge review in https://github.com/bitcoin/bitcoin/pull/34342#discussion_r3358700705 but I didn't manage to look into it until now.
Second commit: `-rpcclienttimeout` no longer measures real idle time. Before the libevent removal, it used to mean give up if really nothing arrives for this long, and any newly incoming data did reset the counter. With the new code the countdown ignores progress, so a large/slow response could be cut off while data still arrives. Revert this to the old behavior.
The second commit does not have a test because I didn't manage to construct one that didn't turn out to be flaky. It may be possible but I couldn't come up with something within a scope of complexity that seems reasonable for this.
ACKs for top commit:
achow101:
ACK 1805716354b49ecb96e49b949a61e2c72ad2c1a3
0tuedon:
tACK 1805716354b49ecb96e49b949a61e2c72ad2c1a3
winterrdog:
tACK 1805716354b49ecb96e49b949a61e2c72ad2c1a3
hodlinator:
ACK 1805716354b49ecb96e49b949a61e2c72ad2c1a3
Tree-SHA512: 6139328c84480af7c7ccd5beec3361a28bbd639459e36b3f033ffc5946840ac0e589d987d4ce2414e58110b73ce48e34b449f9dd5ca577d07e334589a280a136
✓ Specific, descriptive subject✓ Names a concrete action or component✓ Provides detailed explanatory context✓ Explains rationale or failure mode✓ Mentions testing or verification✓ Links an issue, advisory, or supporting reference
What changed, and why it matters
This update fixes two bugs in bitcoin-cli, the command-line tool used to talk to a Bitcoin node. First, when a server replied with an empty body but said it was intentionally empty (Content-Length: 0), the client would keep waiting instead of accepting the reply. Second, the client timeout stopped measuring actual idle time, so a slow but ongoing download could be cut off. The patch makes the timeout reset whenever new data arrives and treats a declared empty body as complete. The hang-on-empty-body case is the more security-relevant one, because a malicious or misbehaving server that keeps the connection open could make bitcoin-cli hang until the user kills it.
Recommended action
No urgent action beyond applying the patch. Users and downstream packagers should include this fix to avoid bitcoin-cli hanging against servers that return Content-Length: 0 while keeping the connection open. Operators relying on -rpcclienttimeout for large RPC calls should verify slow responses are no longer prematurely aborted.
Security signals we found
01
Client-side hang on empty HTTP body (denial-of-service against bitcoin-cli user)
02
Timeout regression could abort legitimate slow RPC responses
03
Fix distinguishes Content-Length: 0 from absent Content-Length
04
Timeout behavior reverted to reset on each received data chunk
Evidence from the diff
The patch changes src/bitcoin-cli.cpp in two ways. (1) Content-Length is now stored as std::optional
Changed components
src/bitcoin-cli.cpp HTTPClient::ReadResponsesrc/bitcoin-cli.cpp HTTPClient::SendRequestsrc/bitcoin-cli.cpp HTTPClient::Recvbitcoin-cli RPC client timeout handlingInspect captured patch +41 / −23
### src/bitcoin-cli.cpp
@@ -854,7 +854,7 @@ class HTTPClient
: m_socket(std::move(socket)), m_host(host), m_timeout(timeout) {}
bool SendRequest(std::string_view request);
HTTPResponse ReadResponse();
- std::optional<std::string> Recv(std::chrono::time_point<std::chrono::steady_clock> deadline);
+ std::optional<std::string> Recv();
};
HTTPClient HTTPClient::Connect(const std::string& host, uint16_t port, std::chrono::seconds timeout)
@@ -906,13 +906,9 @@ HTTPResponse HTTPClient::Post(const std::string& endpoint,
bool HTTPClient::SendRequest(std::string_view request)
{
- const auto deadline{std::chrono::steady_clock::now() + m_timeout};
-
while (!request.empty()) {
Sock::Event event{0};
- auto time_left = std::chrono::duration_cast<std::chrono::milliseconds>(
- deadline - std::chrono::steady_clock::now());
- if (time_left.count() <= 0 || !m_socket->Wait(time_left, Sock::SendEvent, &event)) {
+ if (!m_socket->Wait(m_timeout, Sock::SendEvent, &event)) {
return false;
}
@@ -938,13 +934,12 @@ HTTPResponse HTTPClient::ReadResponse()
{
HTTPResponse response;
std::string buffer;
- const auto deadline{std::chrono::steady_clock::now() + m_timeout};
// Read data until we have complete headers
size_t headers_end = 0;
while (headers_end == 0) {
- if (auto result{Recv(deadline)}) {
+ if (auto result{Recv()}) {
buffer.append(*result);
} else {
std::this_thread::yield();
@@ -987,7 +982,7 @@ HTTPResponse HTTPClient::ReadResponse()
headers.Read(reader);
// Determine body length
- size_t content_length = 0;
+ std::optional<size_t> content_length;
bool chunked = false;
// RFC 9112 §6.3 says responses with both Transfer-Encoding and Content-Length
@@ -999,11 +994,10 @@ HTTPResponse HTTPClient::ReadResponse()
} else {
auto content_length_header = headers.FindFirst("content-length");
if (content_length_header) {
- auto maybe_len = ToIntegral<size_t>(*content_length_header);
- if (!maybe_len) {
+ content_length = ToIntegral<size_t>(*content_length_header);
+ if (!content_length) {
throw HTTPError{"Invalid Content-Length"};
}
- content_length = *maybe_len;
}
}
@@ -1044,7 +1038,7 @@ HTTPResponse HTTPClient::ReadResponse()
size_t crlf_pos = buffer.find("\r\n");
if (crlf_pos == std::string::npos) {
// Need more data
- if (auto result{Recv(deadline)}) {
+ if (auto result{Recv()}) {
buffer.append(*result);
} else {
std::this_thread::yield();
@@ -1076,7 +1070,7 @@ HTTPResponse HTTPClient::ReadResponse()
// Need more data
while (true) {
- if (auto result{Recv(deadline)}) {
+ if (auto result{Recv()}) {
buffer.append(*result);
break;
} else {
@@ -1086,10 +1080,10 @@ HTTPResponse HTTPClient::ReadResponse()
}
response.body = std::move(body);
- } else if (content_length > 0) {
+ } else if (content_length) {
// Fixed content length
- while (buffer.size() < content_length) {
- if (auto result{Recv(deadline)}) {
+ while (buffer.size() < *content_length) {
+ if (auto result{Recv()}) {
buffer.append(*result);
} else {
std::this_thread::yield();
@@ -1098,14 +1092,14 @@ HTTPResponse HTTPClient::ReadResponse()
// Possibly shrink buffer in case we got a larger response than
// originally specified.
- buffer.resize(content_length);
+ buffer.resize(*content_length);
response.body = std::move(buffer);
} else {
// No Content-Length and not chunked: read until the peer closes the
// connection (RFC 9112 §6.3, HTTP/1.0 fallback).
try {
while (true) {
- if (auto result{Recv(deadline)}) {
+ if (auto result{Recv()}) {
buffer.append(*result);
} else {
std::this_thread::yield();
@@ -1118,7 +1112,7 @@ HTTPResponse HTTPClient::ReadResponse()
return response;
}
-std::optional<std::string> HTTPClient::Recv(const std::chrono::time_point<std::chrono::steady_clock> deadline)
+std::optional<std::string> HTTPClient::Recv()
{
auto wait_for_readable{[this](std::chrono::milliseconds timeout) -> bool {
Sock::Event event{0};
@@ -1128,9 +1122,7 @@ std::optional<std::string> HTTPClient::Recv(const std::chrono::time_point<std::c
return (event & Sock::RecvEvent) != 0;
}};
- auto time_left = std::chrono::duration_cast<std::chrono::milliseconds>(
- deadline - std::chrono::steady_clock::now());
- if (time_left.count() <= 0 || !wait_for_readable(time_left)) {
+ if (!wait_for_readable(m_timeout)) {
throw CConnectionFailed{"timeout"};
}
### test/functional/interface_bitcoin_cli.py
@@ -6,7 +6,9 @@
from decimal import Decimal
import re
+import socket
import subprocess
+import threading
from test_framework.blocktools import COINBASE_MATURITY
from test_framework.netutil import test_ipv6_local
@@ -115,6 +117,28 @@ def test_echojson_positional_equals(self):
expected = [["data=test"], 42]
assert_equal(result, expected)
+ def test_empty_response_body(self):
+ self.log.info("Test that a response with Content-Length: 0 does not wait for the peer to close")
+
+ # A real node closes the connection, so only a raw socket can keep it open
+ listener = socket.socket()
+ listener.bind(('127.0.0.1', 0))
+ listener.listen(1)
+
+ def serve():
+ conn, _ = listener.accept()
+ with conn:
+ conn.sendall(b'HTTP/1.1 401 Unauthorized\r\nContent-Length: 0\r\n\r\n')
+ # Keep the connection open so the client cannot rely on EOF
+ while conn.recv(4096):
+ pass
+
+ threading.Thread(target=serve, daemon=True).start()
+ with listener:
+ assert_raises_process_error(
+ 1, 'Authorization failed',
+ self.nodes[0].cli(f'-rpcport={listener.getsockname()[1]}', '-rpcclienttimeout=10').echo)
+
def run_test(self):
"""Main test logic"""
self.test_echojson_positional_equals()
@@ -148,6 +172,8 @@ def run_test(self):
self.log.info("Test connecting to a non-existing server")
assert_raises_process_error(1, "Could not connect to the server", self.nodes[0].cli('-rpcport=1').echo)
+ self.test_empty_response_body()
+
self.log.info("Test handling of invalid ports in rpcconnect")
assert_raises_process_error(1, "Invalid port provided in -rpcconnect: 127.0.0.1:notaport", self.nodes[0].cli("-rpcconnect=127.0.0.1:notaport").echo)
assert_raises_process_error(1, "Invalid port provided in -rpcconnect: 127.0.0.1:-1", self.nodes[0].cli("-rpcconnect=127.0.0.1:-1").echo)Risk score
Our methodology →Why this scored 38/100
Human-validated context
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
No validated notes yet.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.