test: tolerate race condition in interface_http.py #36118

pull pinheadmz wants to merge 1 commits into bitcoin:master from pinheadmz:interface-http-consistency changing 1 files +24 −8
  1. pinheadmz commented at 6:44 PM on August 28, 2026: member

    Fixes #35632 by allowing both outcomes of a race condition. The server behavior is unchanged: in response to a malformed request we send an error code and disconnect. The issue is that sometimes on Windows the RST is caught by the platform and the receive buffer is discarded before the Python client can process it with recv().

    We can also be much more polite to misbehaving clients by implementing a lingering close using SO_LINGER as suggested in #35780 but that will require more review.

    The exact error in #35632 is hard to produce reliably but there are a few close options for reviewers. I tested this on windows native building with MSVC. In both of these cases the patch from this PR caught the error and passed the test.

    RemoteDisconnected: Remote end closed connection without response

    diff --git a/src/httpserver.cpp b/src/httpserver.cpp
    index 9bb89863af..62324d3fea 100644
    --- a/src/httpserver.cpp
    +++ b/src/httpserver.cpp
    @@ -1072,7 +1072,7 @@ std::unique_ptr<HTTPRequest> HTTPRemoteClient::TryReadRequest(const std::shared_
                 e.what());
    
             // We failed to read a complete request from the buffer
    -        WriteNoStoreErrorReply(*client->m_req, HTTP_BAD_REQUEST);
    +        // WriteNoStoreErrorReply(*client->m_req, HTTP_BAD_REQUEST);
             client->m_disconnect = true;
             return nullptr;
         }
    

    ConnectionResetError: [WinError 10054] An existing connection was forcibly closed by the remote host

    diff --git a/src/httpserver.cpp b/src/httpserver.cpp
    index 9bb89863af..be52acb874 100644
    --- a/src/httpserver.cpp
    +++ b/src/httpserver.cpp
    @@ -1154,6 +1154,11 @@ bool HTTPRemoteClient::MaybeDisconnect(std::chrono::time_point<SteadyClock> now,
                  "Disconnecting HTTP client %s (id=%llu)",
                  m_origin,
                  m_id);
    +    auto sock{GetSock()};
    +    linger opt{};
    +    opt.l_onoff  = 1;  // enable SO_LINGER
    +    opt.l_linger = 0;  // zero timeout
    +    sock->SetSockOpt(SOL_SOCKET, SO_LINGER, &opt, sizeof(opt));
         return true;
     }
    
    
  2. DrahtBot added the label Tests on Aug 28, 2026
  3. DrahtBot commented at 6:44 PM on August 28, 2026: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--006a51241073e994b41acfe9ec718e94-->

    Code Coverage & Benchmarks

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

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

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

    If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

    <!--5faf32d7da4f0f540f40219e4f7537a3-->

  4. pinheadmz force-pushed on Aug 28, 2026
  5. pinheadmz commented at 11:18 AM on August 30, 2026: member

    Review requests: @b-l-u-e @winterrdog have been looking at the issue with the longer term approach of linger close. @janb84 would be nice to confirm as far as http behavior for handling invalid requests, that this new flexibility isn't reducing coverage over the server.

  6. fanquake added this to the milestone 32.0 on Aug 30, 2026
  7. in test/functional/interface_http.py:107 in 72c7412c99
     103 | @@ -104,6 +104,20 @@ def setup_network(self):
     104 |          self.setup_nodes()
     105 |          self.node = self.nodes[0]
     106 |  
     107 | +    def send_and_tolerate_disconnect(self, predicate, expected_response_status):
    


    hodlinator commented at 11:31 AM on August 31, 2026:

    nit: When initially glancing at this I got suspicious by the configurable expected status. "Should we be tolerating disconnects even for non-error statuses?" was roughly what ran through my head.

    I'd prefer it be somewhat more locked down for now until we need to support other statuses:

    --- a/test/functional/interface_http.py
    +++ b/test/functional/interface_http.py
    @@ -104,7 +104,7 @@ class HTTPBasicsTest (BitcoinTestFramework):
             self.setup_nodes()
             self.node = self.nodes[0]
    
    -    def send_and_tolerate_disconnect(self, predicate, expected_response_status):
    +    def send_bad_and_tolerate_disconnect(self, predicate):
             '''
             Tolerate a race condition when sending a malformed request that should result
             in the server disconnecting the client. The server *should* be sending an error
    @@ -113,10 +113,10 @@ class HTTPBasicsTest (BitcoinTestFramework):
             '''
             try:
                 response = predicate()
    -            assert_equal(response.status, expected_response_status)
    -            self.log.info("Client received expected response before connection was terminated")
    +            assert_equal(response.status, http.client.BAD_REQUEST)
    +            self.log.info(f"Client received expected {http.client.BAD_REQUEST} response before connection was terminated")
             except NETWORK_ERRORS:
    -            self.log.info("Client did not receive expected response before connection was terminated")
    +            self.log.info(f"Client did not receive expected {http.client.BAD_REQUEST} response before connection was terminated")
    
         def run_test(self):
             # The test framework typically reuses a single persistent HTTP connection
    @@ -206,7 +206,7 @@ class HTTPBasicsTest (BitcoinTestFramework):
    
             # Excessive URI size plus default headers breaks the limit.
             conn = BitcoinHTTPConnection(self.node)
    -        self.send_and_tolerate_disconnect(lambda: conn.get(f'/{"x" * MAX_HEADERS_SIZE}'), http.client.BAD_REQUEST)
    +        self.send_bad_and_tolerate_disconnect(lambda: conn.get(f'/{"x" * MAX_HEADERS_SIZE}'))
    
             # Compute how many short header lines need to be added to http.client
             # default headers to make / break the total limit in a single request.
    @@ -225,7 +225,7 @@ class HTTPBasicsTest (BitcoinTestFramework):
             conn = BitcoinHTTPConnection(self.node)
             for i in range(headers_above_limit):
                 conn.add_header(f"header_{i:04}", "foo")
    -        self.send_and_tolerate_disconnect(lambda: conn.get('/x'), http.client.BAD_REQUEST)
    +        self.send_bad_and_tolerate_disconnect(lambda: conn.get('/x'))
    
             # Compute how much data we can add to a request message body
             # to make / break the limit.
    @@ -605,7 +605,7 @@ class HTTPBasicsTest (BitcoinTestFramework):
             # Extra whitespace before colon in header.
             conn = BitcoinHTTPConnection(self.node)
             conn.headers = {"Authorization ": f"Basic {str_to_b64str(conn.authpair)}"}
    -        self.send_and_tolerate_disconnect(lambda: conn.post('/', '{"method": "getbestblockhash"}'), http.client.BAD_REQUEST)
    +        self.send_bad_and_tolerate_disconnect(lambda: conn.post('/', '{"method": "getbestblockhash"}'))
    
             # Extra whitespace at start of new line.
             # "line folding" as defined in
    @@ -614,7 +614,7 @@ class HTTPBasicsTest (BitcoinTestFramework):
             # https://www.rfc-editor.org/rfc/rfc7230#section-3.2.4
             conn = BitcoinHTTPConnection(self.node)
             conn.headers = {"Authorization": f"Basic \n {str_to_b64str(conn.authpair)}"}
    -        self.send_and_tolerate_disconnect(lambda: conn.post('/', '{"method": "getbestblockhash"}'), http.client.BAD_REQUEST)
    +        self.send_bad_and_tolerate_disconnect(lambda: conn.post('/', '{"method": "getbestblockhash"}'))
    
    
         def check_connection_limit(self):
    

    janb84 commented at 2:11 PM on August 31, 2026:

    NIT: is predicate the correct parameter name ? predicate returns a truth value e.a boolean testable. Given that the function returns a object , maybe request_fn is better suited.

        def send_and_tolerate_disconnect(self, request_fn, expected_response_status):
    

    pinheadmz commented at 3:18 PM on September 2, 2026:

    Taking this change, I think originally I wanted to use the helper for 413 TOO LARGE as well, which is the other error-then-disconnect behavior, but it's handled in a different way after all.


    pinheadmz commented at 3:18 PM on September 2, 2026:

    using predicate_fn here


    hodlinator commented at 10:54 AM on September 3, 2026:

    re #36118 (review): nanonit: Would prefer the more informative but shorter send_fn.

  8. hodlinator approved
  9. hodlinator commented at 11:47 AM on August 31, 2026: contributor

    tACK 72c7412c99136b16192c53dda9c183953a415cc0

    My initial impression was that this was loosening the functional test too much. However, upon further consideration it seems fine to hang-up quickly on clients once we've detected malicious behavior and stop wasting HTTP server cycles on them.

    One could limit the tolerance to only be allowed on Windows, but in theory it should be fine on other platforms too.

    Reproduced the expected "Client did not receive expected response"... messages (on Windows) through applying the diff of omitting WriteNoStoreErrorReply(*client->m_req, HTTP_BAD_REQUEST);.

  10. winterrdog commented at 1:23 PM on August 31, 2026: contributor

    approach ACK

    we already do sth similar to this in check_excessive_request_size() and check_chunked_transfer() i.e. inside their send_excessive_body & send_excessive_chunked local-scope helpers respectively


    just 1 question though - shouldn't we also apply send_and_tolerate_disconnect() (or a similar try/except) to the receive side in check_excessive_request_size() and check_chunked_transfer()?

    the background threads (send_excessive_body & send_excessive_chunked in those 2 aforementioned functions) already handle the send-side race with try/except, but the main thread still does bare recv_raw() calls that could (presumably, because it is similar to what we are trying to fix here) hit the same Windows RST race when receiving the 413 response:

    in check_excessive_request_size(): https://github.com/bitcoin/bitcoin/blob/d2e24e951de45e7e8d328ef36b80c055c90a6fdd/test/functional/interface_http.py#L231-L254

    in check_chunked_transfer(): https://github.com/bitcoin/bitcoin/blob/d2e24e951de45e7e8d328ef36b80c055c90a6fdd/test/functional/interface_http.py#L350-L377

    e.g.

    send_thread.start()
    response5 = conn.recv_raw().decode()  # <----- an unprotected recv
    assert "413 Content too large" in response5
    

    since 413 responses trigger the same "send error + disconnect" code path as 400 responses in TryReadRequest(), they should hit the same race condition on Windows, right ?

    https://github.com/bitcoin/bitcoin/blob/d2e24e951de45e7e8d328ef36b80c055c90a6fdd/src/httpserver.cpp#L1052-L1078

    just curious to hear what others (besides me, of course) think - maybe i am missing something about why the 413 cases are any different ?

  11. b-l-u-e commented at 3:44 AM on September 1, 2026: contributor

    I will review the code shortly

  12. janb84 commented at 6:59 PM on September 1, 2026: contributor

    Concept ACK 72c7412c99136b16192c53dda9c183953a415cc0

    I have some trouble re-creating the errors on a win 11 vm box as per PR description. The code looks oke and the tests run fine (locally and windows). Don't think we have lost any coverage, ran some coverage tooling on master and this PR (sometimes 1-2 branch more of coverage due to non-deter.).

  13. jeanpablojp commented at 3:14 PM on September 2, 2026: contributor

    Approach ACK

    I suppressed the 400 for the three header errors covered only by these call sites, and interface_http.py still passes. The same mutation fails on the base. The fourth remains covered by check_pipelining() through the same max_line_length error.

    Could we keep a server-side assertion for the response? Wrapping the request with assert_debug_log() for the expected status keeps the reset tolerance while proving the server queued the reply. It passes on the head and fails under the mutation.

    Separately, as a pre-existing non-blocking gap, removing client->m_disconnect = true from the std::runtime_error path leaves the test passing on both base and head. Since these calls have no background send thread, could the helper consume the response and then assert conn.sock_closed() with a short timeout? Consuming first matters because ResponseNotReady otherwise gives a false positive.

  14. test: tolerate race condition in interface_http.py
    Fixes #35632 by allowing both outcomes of a race condition.
    The server behavior is unchanged: in response to a malformed request
    we send an error code and disconnect. The issue is that sometimes
    on Windows the RST is caught by the platform and the receive buffer
    is discarded before the Python client can process it with recv().
    
    We can also be much more polite to misbehaving clients by
    implementing SO_LINGER as suggested in #35780 but that will require
    more review.
    a51df9b0ec
  15. pinheadmz force-pushed on Sep 2, 2026
  16. pinheadmz commented at 5:46 PM on September 2, 2026: member

    push to a51df9b0ecf6ecab1a9eb7728a0b475be6eec3fd

    Rename some things from review feedback by @janb84 and @hodlinator

    Add extra assertions sugested by @jeanpablojp:

    • assert HTTP server logged the BAD_REQUEST response
    • assert the server did disconnect @winterrdog

    check_excessive_request_size() and check_chunked_transfer() Those tests handle the same behavior but in a different way, because they send so much data that the server will disconnect while the client is still trying to send. The cases covered in this PR send smaller amounts of data so the platform could handle the RST a lot sooner.

  17. winterrdog commented at 7:06 AM on September 3, 2026: contributor

    tACK a51df9b0ecf6ecab1a9eb7728a0b475be6eec3fd

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


    later on, once we implement a lingering close, i reckon this kind of test workaround will not be needed anymore, since the server will give the error response a chance to reach the client before disconnecting can go ahead

  18. DrahtBot requested review from janb84 on Sep 3, 2026
  19. DrahtBot requested review from hodlinator on Sep 3, 2026
  20. DrahtBot requested review from jeanpablojp on Sep 3, 2026
  21. janb84 commented at 9:09 AM on September 3, 2026: contributor

    re ACK a51df9b0ecf6ecab1a9eb7728a0b475be6eec3fd

    changes since last ack:

    • NITS of holdernator applied,
    • NIT of me applied (thanks)

    LGTM! (for now)

  22. hodlinator approved
  23. hodlinator commented at 11:00 AM on September 3, 2026: contributor

    re-ACK a51df9b0ecf6ecab1a9eb7728a0b475be6eec3fd

    Nice addition to assert the server log message regardless of whether the client gets the status code in time or not.

  24. jeanpablojp commented at 11:10 AM on September 3, 2026: contributor

    re-ACK a51df9b0ecf6ecab1a9eb7728a0b475be6eec3fd

    Tested again just in case, all good.

  25. sedited approved
  26. sedited commented at 11:23 AM on September 3, 2026: contributor

    ACK a51df9b0ecf6ecab1a9eb7728a0b475be6eec3fd

  27. sedited merged this on Sep 3, 2026
  28. sedited closed this on Sep 3, 2026


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-09-09 07:56 UTC