fix: handle invalid `-rpcbind` port earlier #30679

pull tdb3 wants to merge 4 commits into bitcoin:master from tdb3:handle_invalid_rpcbind_port changing 3 files +74 −45
  1. tdb3 commented at 2:12 AM on August 20, 2024: contributor

    Previously, when an invalid port was specified in -rpcbind, the SplitHostPort() return value in HTTPBindAddresses() was ignored and attempt would be made to bind to the default rpcbind port (with the host/port treated as a host).

    This rearranges port checking code in AppInitMain() to handle the invalid port before reaching HTTPBindAddresses(). Also adds a check in HTTPBindAddresses() as a defensive measure for future changes.

    Adds then updates associated functional tests as well.

  2. DrahtBot commented at 2:12 AM on August 20, 2024: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--006a51241073e994b41acfe9ec718e94-->

    Code Coverage

    For detailed information about the code coverage, see the test coverage report.

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK ryanofsky, zaidmstrr, achow101

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #30157 (Fee Estimation via Fee rate Forecasters by ismaelsadeeq)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  3. tdb3 commented at 2:14 AM on August 20, 2024: contributor

    Existing behavior:

    src/bitcoind -rpcallowip=127.0.0.1 -rpcbind=127.0.0.1:18000 -rpcbind=[::1]:18001 -rpcbind=127.0.0.1:notaport -rpcbind=127.0.0.1:-18002 -rpcbind=[::1]:-18003
...
2024-08-21T02:44:13Z Command-line arg: rpcallowip="127.0.0.1"
2024-08-21T02:44:13Z Command-line arg: rpcbind="127.0.0.1:18000"
2024-08-21T02:44:13Z Command-line arg: rpcbind="[::1]:18001"
2024-08-21T02:44:13Z Command-line arg: rpcbind="127.0.0.1:notaport"
2024-08-21T02:44:13Z Command-line arg: rpcbind="127.0.0.1:-18002"
2024-08-21T02:44:13Z Command-line arg: rpcbind="[::1]:-18003"
2024-08-21T02:44:13Z Using at most 125 automatic connections (1024 file descriptors available)
2024-08-21T02:44:13Z scheduler thread start
2024-08-21T02:44:13Z Binding RPC on address 127.0.0.1 port 18000
2024-08-21T02:44:13Z Binding RPC on address ::1 port 18001
2024-08-21T02:44:13Z Binding RPC on address 127.0.0.1:notaport port 18443
2024-08-21T02:44:13Z [libevent:warning] getaddrinfo: nodename nor servname provided, or not known
2024-08-21T02:44:13Z Binding RPC on address 127.0.0.1:notaport port 18443 failed.
2024-08-21T02:44:13Z Binding RPC on address 127.0.0.1:-18002 port 18443
2024-08-21T02:44:13Z [libevent:warning] getaddrinfo: nodename nor servname provided, or not known
2024-08-21T02:44:13Z Binding RPC on address 127.0.0.1:-18002 port 18443 failed.
2024-08-21T02:44:13Z Binding RPC on address [::1]:-18003 port 18443
2024-08-21T02:44:13Z [libevent:warning] getaddrinfo: nodename nor servname provided, or not known
2024-08-21T02:44:13Z Binding RPC on address [::1]:-18003 port 18443 failed.
2024-08-21T02:44:13Z Using random cookie authentication.
2024-08-21T02:44:13Z Generated RPC authentication cookie /home/dev/.bitcoin/regtest/.cookie
2024-08-21T02:44:13Z Permissions used for cookie: rw-------
2024-08-21T02:44:13Z Starting HTTP server with 4 worker threads
2024-08-21T02:44:13Z Using wallet directory /home/dev/.bitcoin/regtest/wallets
2024-08-21T02:44:13Z init message: Verifying wallet(s)…
2024-08-21T02:44:13Z Using /16 prefix for IP bucketing
2024-08-21T02:44:13Z init message: Loading P2P addresses…
2024-08-21T02:44:13Z Loaded 0 addresses from peers.dat  0ms
2024-08-21T02:44:13Z init message: Loading banlist…
2024-08-21T02:44:13Z SetNetworkActive: true
2024-08-21T02:44:13Z [error] Invalid port specified in -rpcbind: '127.0.0.1:notaport'
Error: Invalid port specified in -rpcbind: '127.0.0.1:notaport'
2024-08-21T02:44:13Z Shutdown: In progress...
2024-08-21T02:44:13Z scheduler thread exit
2024-08-21T02:44:13Z Flushed fee estimates to fee_estimates.dat.
2024-08-21T02:44:13Z Shutdown: done

    New behavior:

    src/bitcoind -rpcallowip=127.0.0.1 -rpcbind=127.0.0.1:18000 -rpcbind=[::1]:18001 -rpcbind=127.0.0.1:notaport -rpcbind=127.0.0.1:-18002 -rpcbind=[::1]:-18003
...
2024-08-21T02:40:24Z Bitcoin Core version v27.99.0-fbb03ba76f72-dirty (release build)
2024-08-21T02:40:24Z Script verification uses 15 additional threads
2024-08-21T02:40:24Z Using the 'standard' SHA256 implementation
2024-08-21T02:40:24Z Default data directory /home/dev/.bitcoin
2024-08-21T02:40:24Z Using data directory /home/dev/.bitcoin/regtest
2024-08-21T02:40:24Z Config file: /home/dev/.bitcoin/bitcoin.conf
2024-08-21T02:40:24Z Config file arg: coinstatsindex="1"
2024-08-21T02:40:24Z Config file arg: regtest="1"
2024-08-21T02:40:24Z Config file arg: txindex="1"
2024-08-21T02:40:24Z Command-line arg: rpcallowip="127.0.0.1"
2024-08-21T02:40:24Z Command-line arg: rpcbind="127.0.0.1:18000"
2024-08-21T02:40:24Z Command-line arg: rpcbind="[::1]:18001"
2024-08-21T02:40:24Z Command-line arg: rpcbind="127.0.0.1:notaport"
2024-08-21T02:40:24Z Command-line arg: rpcbind="127.0.0.1:-18002"
2024-08-21T02:40:24Z Command-line arg: rpcbind="[::1]:-18003"
2024-08-21T02:40:24Z Using at most 125 automatic connections (1024 file descriptors available)
2024-08-21T02:40:24Z scheduler thread start
2024-08-21T02:40:24Z [error] Invalid port specified in -rpcbind: '127.0.0.1:notaport'
Error: Invalid port specified in -rpcbind: '127.0.0.1:notaport'
2024-08-21T02:40:24Z Shutdown: In progress...
2024-08-21T02:40:24Z scheduler thread exit
2024-08-21T02:40:24Z Shutdown: done
  4. DrahtBot added the label CI failed on Aug 20, 2024
  5. DrahtBot commented at 2:19 AM on August 20, 2024: contributor

    <!--85328a0da195eb286784d51f73fa0af9-->

    🚧 At least one of the CI tasks failed. <sub>Debug: https://github.com/bitcoin/bitcoin/runs/28977110960</sub>

    <details><summary>Hints</summary>

    Make sure to run all tests locally, according to the documentation.

    The failure may happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

    </details>

  6. tdb3 renamed this:
    fix: handle invalid rpcbind port earlier
    fix: handle invalid `-rpcbind` port earlier
    on Aug 20, 2024
  7. tdb3 force-pushed on Aug 20, 2024
  8. DrahtBot removed the label CI failed on Aug 20, 2024
  9. ryanofsky commented at 7:47 PM on August 20, 2024: contributor

    Code review 9f19d10922d615c1e313eb7348fcc536642ad2e4.

    I'm not sure this fix is ideal. For example if the GUI is used I believe this will change the error message from:

    • Error: Invalid port specified in -rpcbind: '127.0.0.1:notaport'

    to just:

    • Unable to start HTTP server. See debug log for details.

    I think it would be good to keep the new return false and error log in HTTPBindAddresses. But a more direct fix for this issue might be to just move the code that checks port numbers on line 1304 above the code that starts the http server on line 1219 in AppInitMain:

    https://github.com/bitcoin/bitcoin/blob/d79ea809d28197b1b4e3748aa1715272b53601d0/src/init.cpp#L1219-L1223

    https://github.com/bitcoin/bitcoin/blob/d79ea809d28197b1b4e3748aa1715272b53601d0/src/init.cpp#L1304-L1346

  10. tdb3 force-pushed on Aug 21, 2024
  11. tdb3 commented at 2:48 AM on August 21, 2024: contributor

    I think it would be good to keep the new return false and error log in HTTPBindAddresses. But a more direct fix for this issue might be to just move the code that checks port numbers on line 1304 above the code that starts the http server on line 1219 in AppInitMain:

    Thank you for taking a look. Agreed, rearranging in AppInitMain seems to be the better approach. Updated.

    Also rearranged the commits such that the tests are introduced in the first commit and adjusted with the introduction of the fix.

  12. in src/init.cpp:1282 in 8892150c2d outdated 
    1210 | @@ -1211,6 +1211,50 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1211 |      RegisterZMQRPCCommands(tableRPC);
1212 |  #endif
1213 |  
1214 | +    // Check port numbers
    ryanofsky commented at 3:03 PM on August 21, 2024:

    In commit "fix: handle invalid rpcbind port earlier" (8892150c2d8ea9e09046e288cecd4c35472c0267)

    Not important, but to make AppInitMain function shorter and make the bugfix commit fix more readable you could consider adding an earlier refactoring commit the moves these for loops into a CheckHostPortOptions function, and have AppinitMain call it like if (!CheckHostPortOptions(args)) return false;

    tdb3 commented at 12:28 AM on August 22, 2024:

    Thanks, good suggestion. Makes things cleaner. Added. Was also thinking about updating ParseUint16() to ToIntegral(), but that could be left for a follow-up, since we would also want to ensure adequate tests for -port and -rpcport with that update.

  13. ryanofsky approved
  14. ryanofsky commented at 3:06 PM on August 21, 2024: contributor

    Code review ACK 8892150c2d8ea9e09046e288cecd4c35472c0267. Looks good! Nice to have earlier feedback and clearer checking for these options. I left a suggestion, but it's not important, so feel free to ignore

  15. tdb3 force-pushed on Aug 22, 2024
  16. in src/httpserver.cpp:380 in 51ba0e6127 outdated 
     376 | @@ -374,7 +377,11 @@ static bool HTTPBindAddresses(struct evhttp* http)
 377 |          for (const std::string& strRPCBind : gArgs.GetArgs("-rpcbind")) {
 378 |              uint16_t port{http_port};
 379 |              std::string host;
 380 | -            SplitHostPort(strRPCBind, port, host);
 381 | +            if (!SplitHostPort(strRPCBind, port, host)) {
    ryanofsky commented at 3:49 PM on August 22, 2024:

    In commit "fix: handle invalid rpcbind port earlier" (51ba0e61273bec02833329ee6ac8fa219679cefa)

    (Not important, but another idea to make this commit smaller and clearer: I think it would be good to move the change in src/httpserver.cpp to a separate followup commit, because while it's a good change that improves code quality and error reporting, it is not really a part of the bugfix, and is more tangentially related to it.)

    tdb3 commented at 4:22 PM on August 22, 2024:

    Thanks. Yes, that's correct (movement of CheckHostPortOptions() handles the bug before HTTPBindAddresses()). It's cleaner to break this out into its own commit. Updated.

  17. ryanofsky approved
  18. ryanofsky commented at 3:50 PM on August 22, 2024: contributor

    Code review ACK 51ba0e61273bec02833329ee6ac8fa219679cefa. Since last review, just split up the change and moved some code so this should be a little easier to review now.

    I had another idea and left a suggestion but it is not important so please feel free to ignore.

  19. tdb3 force-pushed on Aug 22, 2024
  20. ryanofsky approved
  21. ryanofsky commented at 6:18 PM on August 22, 2024: contributor

    Code review ACK 590d2fb0d8548d6d55fd1c738c8bcbe71a57aaf2

    Just split up the last commit since the previous review to separate bugfix from cleanup

  22. zaidmstrr commented at 2:55 PM on September 6, 2024: contributor

    Hey, I'm reviewing this PR, and I saw that the test run_invalid_bind_test you implemented in rpc_bind.py is not invoking without explicitly providing the --ipv4 or --ipv6 flags, so is there any particular reason for that?

  23. tdb3 commented at 12:19 AM on September 7, 2024: contributor

    Hey, I'm reviewing this PR, and I saw that the test run_invalid_bind_test you implemented in rpc_bind.py is not invoking without explicitly providing the --ipv4 or --ipv6 flags, so is there any particular reason for that?

    Thank you for the review! In test/functional/test_runner.py, rpc_bind.py is run with three variations (--ipv4, --ipv6, and --nonloopback), so there is coverage for parsing rpcbind with both IPv4 and IPv6 addresses/port binds (loopback).

  24. zaidmstrr commented at 4:06 PM on September 7, 2024: contributor

    ACK 590d2fb I have reviewed and tested by manual and dry running the code to detect any edge cases and coding related erros. I also performed manual fuzzing by passing arguments in the CLI using bitcoind to check for any unexpected behaviours and to check whether the new code giving the updated results without any bypass or failure. 

  25. DrahtBot added the label Needs rebase on Sep 16, 2024
  26. test: add tests for invalid rpcbind ports 73c243965a
  27. tdb3 force-pushed on Sep 18, 2024
  28. tdb3 commented at 1:15 AM on September 18, 2024: contributor

    Rebased to address conflict from #30868

  29. refactor: move host/port checking
    Reduces the size of AppInitMain() by moving
checks to CheckHostPortOptions()

Co-Authored-By: Ryan Ofsky <ryan@ofsky.org>
    83b67f2e6d
  30. fix: handle invalid rpcbind port earlier
    Previously, when an invalid port was specified
in `-rpcbind`, the `SplitHostPort()` return value
in `HTTPBindAddresses()` was ignored and attempt
would be made to bind to the default rpcbind port
(with the host/port treated as a host).

This rearranges port checking code in
`AppInitMain()` to handle the invalid
port before reaching `HTTPBindAddresses()`.

Also adjusts associated functional tests.
    d38e3aed89
  31. fix: increase rpcbind check robustness
    Adds invalid rpcbind port checking to
`HTTPBindAddresses()`. While movement of
`CheckHostPortOptions()` in the previous
commit handles rcpbind port errors, updating
`HTTPBindAddresses()` port checking adds
a defensive measure for potential future
changes.
    e6994efe08
  32. tdb3 force-pushed on Sep 18, 2024
  33. DrahtBot added the label CI failed on Sep 18, 2024
  34. DrahtBot commented at 2:00 AM on September 18, 2024: contributor

    <!--85328a0da195eb286784d51f73fa0af9-->

    🚧 At least one of the CI tasks failed. <sub>Debug: https://github.com/bitcoin/bitcoin/runs/30290727636</sub>

    <details><summary>Hints</summary>

    Make sure to run all tests locally, according to the documentation.

    The failure may happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

    </details>

  35. DrahtBot removed the label Needs rebase on Sep 18, 2024
  36. DrahtBot removed the label CI failed on Sep 18, 2024
  37. ryanofsky approved
  38. ryanofsky commented at 11:18 AM on September 19, 2024: contributor

    Code review ACK e6994efe08b282dd9e46602bcbad69567fe91dcd

    No changes since last review, just rebased due to conflict

  39. DrahtBot requested review from zaidmstrr on Sep 19, 2024
  40. zaidmstrr approved
  41. zaidmstrr commented at 7:23 AM on September 20, 2024: contributor

    Code review ACK e6994ef All the things seem correct as before.

  42. achow101 commented at 5:26 PM on September 20, 2024: member

    ACK e6994efe08b282dd9e46602bcbad69567fe91dcd

  43. achow101 merged this on Sep 20, 2024
  44. achow101 closed this on Sep 20, 2024
  45. TheCharlatan referenced this in commit 8bb47d4c2c on Nov 2, 2024
  46. bitcoin locked this on Sep 20, 2025
Contributors
tdb3DrahtBotryanofskyzaidmstrrachow101
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-05-01 06:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me