Validate port-options #22087

pull amadeuszpawlik wants to merge 2 commits into bitcoin:master from amadeuszpawlik:sanitize_ports changing 8 files +117 −14
  1. amadeuszpawlik commented at 2:08 pm on May 27, 2021: contributor

    Validate port-options, so that invalid values are rejected early in the startup. Ports are uint16_ts, which effectively limits a port’s value to <=65535. As discussed in #24116 and #24344, port “0” is considered invalid too. Proposed in #21893 (comment)

    The SplitHostPort(std::string in, uint16_t& portOut, std::string& hostOut) now returns a bool that indicates whether the port value was set and within the allowed range. This is an improvement that can be used not only for port validation of options at startup, but also in rpc calls, etc,

  2. amadeuszpawlik renamed this:
    Sanitize ports
    Sanitize port numbers
    on May 27, 2021
  3. amadeuszpawlik commented at 2:14 pm on May 27, 2021: contributor
    Should this be done with -zmqpub... options as well?
  4. jonatack commented at 2:18 pm on May 27, 2021: contributor
    I’d suggest searching for and looking at a previous pull request proposal to do this. Consider also unit testing and Doxygen docs.
  5. jonatack commented at 2:18 pm on May 27, 2021: contributor
    Consider also the various places in the codebase where users can input port numbers.
  6. DrahtBot commented at 3:07 pm on May 27, 2021: contributor

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #25136 (Torcontrol opt check by amadeuszpawlik)

    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.

  7. DrahtBot added the label Utils/log/libs on May 27, 2021
  8. amadeuszpawlik renamed this:
    Sanitize port numbers
    WIP Sanitize port numbers
    on May 27, 2021
  9. amadeuszpawlik force-pushed on May 29, 2021
  10. amadeuszpawlik force-pushed on May 29, 2021
  11. amadeuszpawlik force-pushed on May 29, 2021
  12. amadeuszpawlik commented at 9:43 am on May 29, 2021: contributor

    Added doxygen descriptions Added/updated test for SplitHostPort @jonatack could you clarify

    Consider also the various places in the codebase where users can input port numbers.

    Have I missed something, like a config?

  13. amadeuszpawlik renamed this:
    WIP Sanitize port numbers
    Sanitize port numbers
    on May 31, 2021
  14. amadeuszpawlik marked this as ready for review on May 31, 2021
  15. klementtan commented at 0:47 am on June 3, 2021: contributor
    You might wanna add the validation at addpeeraddress too. Currently, the port number will overflow if it is invalid.
  16. amadeuszpawlik commented at 4:09 pm on June 3, 2021: contributor
    Thanks for your input, @klementtan ! I addressed your comment in a1c591453fbd3f28772a230720675f4e9ff74978
  17. in src/util/strencodings.cpp:136 in a1c591453f outdated 
    131+{
132+    std::string portStr;
133+    SplitHostPort(in, portStr, hostOut);
134+
135+    uint16_t n;
136+    if (!portStr.empty() && ParseUInt16(portStr, &n))
    klementtan commented at 6:01 pm on June 3, 2021:
    The value portOut will be undefined behavior if portStr = "-1" as ParseUInt16 will return false before assigning a value to n
    amadeuszpawlik commented at 11:05 am on June 4, 2021:
    I don’t know if I agree with you on this point: My intention here was not to change the behaviour of SplitHostPort(...) (rather to gain access to the port portion of a socket address as a string). If you for example take a look at src/httpserver.cpp in HTTPBindAddresses(...) this function is used as follows: uint16_t port{some_default_port}; SplitHostPort(..., port, ...); The same can be seen in src/bitcoin-cli.cpp and src/net.cpp: the callers have already set default port value before calling this function.
    klementtan commented at 0:46 am on June 5, 2021:
    That makes sense thanks! Additionally, I was wrong about portOut having an undefined behavior as portOut = n only if portStr is valid
  18. klementtan commented at 6:02 pm on June 3, 2021: contributor

    Thanks for updating it!

    I think you might have missed out the scenario where the user enters -1 as the port. On a1c591453fbd3f28772a230720675f4e9ff74978

    0$ ./src/bitcoin-cli -signet addpeeraddress "1.1.1.3" -1
1{
2  "success": true
3}

    You might wanna refactor the logic in strencodings.cpp so that the port validation code could be reused in other places(ie addpeeraddress)

  19. amadeuszpawlik force-pushed on Jun 4, 2021
  20. amadeuszpawlik commented at 1:08 pm on June 4, 2021: contributor

    @klementtan

    think you might have missed out the scenario where the user enters -1 as the port.

    You are absolutely right, fixed that. I also added a couple negative port numbers in the tests, thanks.

  21. laanwj commented at 12:08 pm on August 2, 2021: member
    Concept ACK. I would personally call this “validate” (as in input validation), not “santize”. In the source base we use “sanitize” only in the context of removing invalid characters from strings in logging.
  22. in src/util/strencodings.h:85 in 96942e8505 outdated 
    80+ *
81+ * @param[in] in        The socket address string to split.
82+ * @param[out] portOut  Port-portion of the input, if found and parsable.
83+ * @param[out] hostOut  Host-portion of the input, if found.
84+ */
85 void SplitHostPort(std::string in, uint16_t& portOut, std::string& hostOut);
    laanwj commented at 12:13 pm on August 2, 2021:

    I would prefer this to be more explicit about parse errors. For example:

    • Return a boolean true if valid false otherwise
    • Return std::optional<uint16_t> (or even std:::optional<std::pair<string, uint16_t>>)

    Either is a lot clearer than “we don’t assign anything if the port is invalid”, which is somewhat indirect behavior.

    amadeuszpawlik commented at 7:43 pm on December 1, 2021:
    Changed to return bool
  23. amadeuszpawlik force-pushed on Aug 27, 2021
  24. amadeuszpawlik renamed this:
    Sanitize port numbers
    Validate port numbers
    on Aug 27, 2021
  25. amadeuszpawlik commented at 1:58 pm on August 27, 2021: contributor

    @laanwj

    Concept ACK. I would personally call this “validate” (as in input validation), not “santize”. In the source base we use “sanitize” only in the context of removing invalid characters from strings in logging.

    I have changed the wording, thanks 👍

  26. DrahtBot added the label Needs rebase on Sep 21, 2021
  27. amadeuszpawlik force-pushed on Dec 1, 2021
  28. DrahtBot removed the label Needs rebase on Dec 1, 2021
  29. amadeuszpawlik requested review from laanwj on Dec 2, 2021
  30. unknown changes_requested
  31. unknown commented at 10:27 pm on December 2, 2021: none

    Concept ACK

    Tested by fuzzing addpeeraddress, did not try config params: -port -rpcport -bind -i2psam -onion-proxy -rpcbind -torcontrol -whitebind -zmq*

    Example of a response for invalid port number or you get parse error:

     0HTTP/1.1 500 Internal Server Error
 1
 2Content-Type: application/json
 3
 4Date: Thu, 02 Dec 2021 21:44:11 GMT
 5
 6Content-Length: 90
 7
 8Connection: close
 9
10
11
12{"result":null,"error":{"code":-8,"message":"Invalid port specified: 89614"},"id":"test"}

    :x: One invalid user input returned 200 response. Negative zero from this wordlist

    Request:

     0POST / HTTP/1.1
 1
 2Host: 127.0.0.1:18662
 3
 4Authorization: Basic dXNlcjY6cGFzczY=
 5
 6Content-Type: text/plain
 7
 8Content-Length: 92
 9
10Connection: close
11
12
13
14{"jsonrpc": "1.0", "id": "test", "method": "addpeeraddress", "params": ["4.4.4.4",-0, true]}

    Response:

     0HTTP/1.1 200 OK
 1
 2Content-Type: application/json
 3
 4Date: Thu, 02 Dec 2021 22:15:28 GMT
 5
 6Content-Length: 54
 7
 8Connection: close
 9
10
11
12{"result":{"success":true},"error":null,"id":"test"}
  32. amadeuszpawlik force-pushed on Dec 5, 2021
  33. jhay666 approved
  34. amadeuszpawlik force-pushed on Dec 5, 2021
  35. amadeuszpawlik renamed this:
    Validate port numbers
    Validate port-options
    on Dec 5, 2021
  36. amadeuszpawlik commented at 8:24 pm on December 5, 2021: contributor
    I reconsidered the addpeeraddress after @prayank23’s comment and decided to remove that case from this PR. Validating port values in RPCs does not contribute to “early rejection of invalid port values at startup”. Please correct me if I’m wrong.
  37. luke-jr changes_requested
  38. luke-jr commented at 0:19 am on February 15, 2022: member
    Please put the error string in one place, with a substitution for the option name. This helps translators only need to translate it once, and avoids accidental translation of the option name.
  39. in src/init.cpp:1283 in 9d97b0501e outdated 
    1275@@ -1276,6 +1276,68 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1276                                      chainman, *node.mempool, ignores_incoming_txs);
1277     RegisterValidationInterface(node.peerman.get());
1278 
1279+    // Check port numbers
1280+    if (args.IsArgSet("-port")) {
1281+        uint64_t portIn = args.GetIntArg("-port", 0);
1282+        if (portIn > std::numeric_limits<uint16_t>::max())
1283+            return InitError(strprintf(_("Invalid port specified in -port: '%d'"), portIn));
    thonkle commented at 0:34 am on February 15, 2022:

    This should also error for port 0.

    See discussions in #24116 and https://github.com/bitcoin/bitcoin/pull/24344

    luke-jr commented at 11:48 pm on February 18, 2022:
    And for less than 0… (change type above to int64_t)
    amadeuszpawlik commented at 4:42 pm on February 20, 2022:
    Thanks for the input, guys. I applied your suggestions + set bool SplitHostPort(...) to consider port 0 as invalid.
  40. amadeuszpawlik force-pushed on Feb 20, 2022
  41. amadeuszpawlik force-pushed on Feb 20, 2022
  42. in src/init.cpp:1280 in 508df36158 outdated 
    1275@@ -1276,6 +1276,69 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1276                                      chainman, *node.mempool, ignores_incoming_txs);
1277     RegisterValidationInterface(node.peerman.get());
1278 
1279+    // Check port numbers
1280+    const bilingual_str port_error = _("Invalid port specified in ");
    luke-jr commented at 11:24 pm on February 22, 2022: 
    0    const bilingual_str port_error = _("Invalid port specified in %s: %s");

    Other languages may need to change the ordering of parameters.

    amadeuszpawlik commented at 9:06 pm on February 23, 2022:
    Yeah, I did that initially but the linter complains, any tips?
    luke-jr commented at 9:41 pm on February 23, 2022:
    Check out the functions at the bottom of src/util/error.cpp (but don’t copy their prepending of '-' - that’s causing trouble in translations)
    amadeuszpawlik commented at 8:50 pm on February 24, 2022:
    Thanks for the review Luke, much appreciated
  43. in src/init.cpp:1294 in 508df36158 outdated 
    1289+            return InitError(strprintf(port_error + Untranslated("%s: %d"),"-rpcport", portIn));
1290+    }
1291+
1292+    std::string hostOut;
1293+    uint16_t portOut{0};
1294+    for (const std::string& socketAddr : args.GetArgs("-bind")) {
    luke-jr commented at 11:29 pm on February 22, 2022:
    A lot of repetition. Maybe an outer for loop over each option name?
    amadeuszpawlik commented at 8:32 pm on February 23, 2022:
    Done, good point
  44. amadeuszpawlik force-pushed on Feb 23, 2022
  45. in src/init.cpp:1282 in 64bb1ad30a outdated 
    1275@@ -1276,6 +1276,30 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1276                                      chainman, *node.mempool, ignores_incoming_txs);
1277     RegisterValidationInterface(node.peerman.get());
1278 
1279+    // Check port numbers
1280+    const bilingual_str port_error = _("Invalid port specified in %s: %s");
1281+    const std::string port_options_int[] = {"-port", "-rpcport"};
1282+    for (const std::string& port_option : port_options_int) {
    luke-jr commented at 9:22 pm on February 23, 2022:

    nit

    0    for (const std::string& port_option : {
1        "-port",
2         "-rpcport",
3    }) {
  46. in src/init.cpp:1286 in 64bb1ad30a outdated 
    1281+    const std::string port_options_int[] = {"-port", "-rpcport"};
1282+    for (const std::string& port_option : port_options_int) {
1283+        if (args.IsArgSet(port_option)) {
1284+            int64_t portIn = args.GetIntArg(port_option, 0);
1285+            if (portIn <= 0 || portIn > std::numeric_limits<uint16_t>::max())
1286+                return InitError(strprintf(port_error, port_option, ToString(portIn)));
    luke-jr commented at 9:24 pm on February 23, 2022:

    nit: I don’t think this needs ToString (strprintf should handle int64_t as %s just fine)

    0                return InitError(strprintf(port_error, port_option, portIn));
  47. in src/init.cpp:1291 in 64bb1ad30a outdated 
    1286+                return InitError(strprintf(port_error, port_option, ToString(portIn)));
1287+        }
1288+    }
1289+
1290+    std::string hostOut;
1291+    uint16_t portOut{0};
    luke-jr commented at 9:24 pm on February 23, 2022:
    nit: Prefer scoping these just before SplitHostPort call
  48. in src/init.cpp:1294 in 64bb1ad30a outdated 
    1289+
1290+    std::string hostOut;
1291+    uint16_t portOut{0};
1292+    const std::string port_options_str[] = {"-bind", "-i2psam", "-onion", "-proxy", "-rpcbind",
1293+      "-torcontrol", "-whitebind", "-zmqpubhashblock","-zmqpubhashtx","-zmqpubrawblock", "-zmqpubsequence"};
1294+    for (const std::string& port_option : port_options_str) {
    luke-jr commented at 9:25 pm on February 23, 2022:

    nit

     0    for (const std::string& port_option : {
 1        "-bind",
 2        "-i2psam",
 3        "-onion",
 4        "-proxy",
 5        "-rpcbind",
 6        "-torcontrol",
 7        "-whitebind",
 8        "-zmqpubhashblock",
 9        "-zmqpubhashtx",
10        "-zmqpubrawblock",
11        "-zmqpubsequence",
12    }) {
  49. in src/init.cpp:1308 in 64bb1ad30a outdated 
    1292+    const std::string port_options_str[] = {"-bind", "-i2psam", "-onion", "-proxy", "-rpcbind",
1293+      "-torcontrol", "-whitebind", "-zmqpubhashblock","-zmqpubhashtx","-zmqpubrawblock", "-zmqpubsequence"};
1294+    for (const std::string& port_option : port_options_str) {
1295+        for (std::string socketAddr : args.GetArgs(port_option)) {
1296+            if (port_option == "-bind")
1297+                socketAddr = socketAddr.substr(0, socketAddr.rfind('='));
    luke-jr commented at 9:29 pm on February 23, 2022:

    Include braces, even if it’s just a one line body.

    0            if (port_option == "-bind") {
1                socketAddr = socketAddr.substr(0, socketAddr.rfind('='));
2            }
  50. in src/init.cpp:1299 in 64bb1ad30a outdated 
    1294+    for (const std::string& port_option : port_options_str) {
1295+        for (std::string socketAddr : args.GetArgs(port_option)) {
1296+            if (port_option == "-bind")
1297+                socketAddr = socketAddr.substr(0, socketAddr.rfind('='));
1298+            if (!SplitHostPort(socketAddr, portOut, hostOut))
1299+                return InitError(strprintf(port_error, port_option, socketAddr));
    luke-jr commented at 9:29 pm on February 23, 2022:

    braces again

    0            if (!SplitHostPort(socketAddr, portOut, hostOut)) {
1                return InitError(strprintf(port_error, port_option, socketAddr));
2            }
  51. in src/util/strencodings.cpp:141 in 64bb1ad30a outdated 
    136+    uint16_t n;
137+    if (portStr.empty()) {
138+      valid = true;
139+    } else if (ParseUInt16(portStr, &n)) {
140+      portOut = n;
141+      valid = portOut == 0 ? false : true;
    luke-jr commented at 9:32 pm on February 23, 2022: 
    0      valid = (portOut != 0);
  52. in src/util/strencodings.cpp:110 in 64bb1ad30a outdated 
    106@@ -107,18 +107,17 @@ std::vector<unsigned char> ParseHex(const std::string& str)
107     return ParseHex(str.c_str());
108 }
109 
110-void SplitHostPort(std::string in, uint16_t& portOut, std::string& hostOut)
111+void SplitHostPort(std::string in, std::string& portOut, std::string& hostOut)
    luke-jr commented at 9:33 pm on February 23, 2022:

    I don’t think we gain anything by having a string-only parser?

    Doing this means SplitHostPort will only conditionally validate port range.

  53. luke-jr changes_requested
  54. amadeuszpawlik force-pushed on Feb 24, 2022
  55. in src/util/strencodings.cpp:127 in c64877da34 outdated 
    126+              valid = (portOut != 0);
127+            }
128             in = in.substr(0, colon);
129-            portOut = n;
130+        } else {
131+          valid = true;
    luke-jr commented at 1:27 am on February 28, 2022:
    This is weird. Ending a host/port string with a bare colon should be treated as invalid IMO.
  56. in src/util/strencodings.h:96 in c64877da34 outdated 
    92+ * Validates port value.
93+ *
94+ * @param[in] in        The socket address string to split.
95+ * @param[out] portOut  Port-portion of the input, if found and parsable.
96+ * @param[out] hostOut  Host-portion of the input, if found.
97+ * @return              true if port-portion is found and within its allowed range, otherwise false
    luke-jr commented at 1:31 am on February 28, 2022: 
    0 * [@return](/bitcoin-bitcoin/contributor/return/)              true if port-portion is absent or within its allowed range, otherwise false
  57. in src/util/strencodings.cpp:114 in c64877da34 outdated 
    129-            portOut = n;
130+        } else {
131+          valid = true;
132         }
133+    } else {
134+      valid = true;
    luke-jr commented at 1:32 am on February 28, 2022:
    IMO we should be setting portOut to 0 here explicitly. Maybe make it a bugfix commit before your others.
    amadeuszpawlik commented at 3:16 pm on February 28, 2022:
    The way this has been used up to now makes for neat code: https://github.com/bitcoin/bitcoin/blob/25290071c434638e2719a99784572deef44542ad/src/netbase.cpp#L203-L205 Don’t you think it’s a bit neater this way?
    luke-jr commented at 6:13 pm on February 28, 2022:
    Yes, I agree.
  58. in src/init.cpp:1309 in c64877da34 outdated 
    1304+        for (std::string socketAddr : args.GetArgs(port_option)) {
1305+            std::string hostOut;
1306+            uint16_t portOut{0};
1307+            if (port_option == "-bind") {
1308+                socketAddr = socketAddr.substr(0, socketAddr.rfind('='));
1309+            }
    luke-jr commented at 1:33 am on February 28, 2022:
    Hmm, maybe better to keep -bind separate for this.
  59. in src/util/error.cpp:48 in c64877da34 outdated 
    44@@ -45,6 +45,11 @@ bilingual_str ResolveErrMsg(const std::string& optname, const std::string& strBi
45     return strprintf(_("Cannot resolve -%s address: '%s'"), optname, strBind);
46 }
47 
48+bilingual_str InvalidPortErrMsg(const std::string& optname, const std::string& strPort)
    luke-jr commented at 1:35 am on February 28, 2022:
    nit: Just call it port, or maybe port_str; we don’t use Hungarian variable names in new code.
    luke-jr commented at 1:42 am on February 28, 2022:
    Actually, something like invalid_value might be better, since this isn’t just a bare port number.
    amadeuszpawlik commented at 3:29 pm on February 28, 2022:
    Thanks again for your input, Luke
  60. luke-jr changes_requested
  61. amadeuszpawlik force-pushed on Feb 28, 2022
  62. in src/init.cpp:1303 in c9d4f114c6 outdated 
    1298+        "-zmqpubhashblock",
1299+        "-zmqpubhashtx",
1300+        "-zmqpubrawblock",
1301+        "-zmqpubsequence",
1302+    }) {
1303+        for (std::string& socket_addr : args.GetArgs(port_option)) {
    luke-jr commented at 6:41 pm on February 28, 2022:

    nit

    0        for (const std::string& socket_addr : args.GetArgs(port_option)) {
  63. in src/init.cpp:1312 in c9d4f114c6 outdated 
    1307+                return InitError(InvalidPortErrMsg(port_option, socket_addr));
1308+            }
1309+        }
1310+    }
1311+
1312+    for (std::string& socket_addr : args.GetArgs("-bind")) {
    luke-jr commented at 6:41 pm on February 28, 2022:

    nit

    0    for (const std::string& socket_addr : args.GetArgs("-bind")) {
  64. in src/init.cpp:1317 in c9d4f114c6 outdated 
    1312+    for (std::string& socket_addr : args.GetArgs("-bind")) {
1313+        std::string host_out;
1314+        uint16_t port_out{0};
1315+        std::string bind_socket_addr = socket_addr.substr(0, socket_addr.rfind('='));
1316+        if (!SplitHostPort(bind_socket_addr, port_out, host_out)) {
1317+            return InitError(InvalidPortErrMsg("-bind", bind_socket_addr));
    luke-jr commented at 6:41 pm on February 28, 2022:

    Might be better to include the full value in the error? idk

    0            return InitError(InvalidPortErrMsg("-bind", socket_addr));
  65. luke-jr approved
  66. luke-jr commented at 6:42 pm on February 28, 2022: member
    utACK as-is, a few nits
  67. amadeuszpawlik force-pushed on Mar 2, 2022
  68. ghost commented at 2:52 pm on March 2, 2022: none

    I was fuzzing -port with different wordlists and noticed something weird for 2 values in big-list-of-naughty-strings.txt:

    It uses port as 1 when -port=1/2 or -port=1E2. Is this expected?

    Can directly try those values or go through each using:

    0wget https://raw.githubusercontent.com/danielmiessler/SecLists/master/Fuzzing/big-list-of-naughty-strings.txt
1while read line;do bitcoind -port="$line";done<big-list-of-naughty-strings.txt

    The error is incorrect in lot of cases where it just says zero:

    02022-03-02T15:01:14Z Command-line arg: port="999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999"
1
22022-03-02T15:01:14Z Error: Invalid port specified in -port: '0'
3Error: Invalid port specified in -port: '0'
  69. amadeuszpawlik force-pushed on Mar 2, 2022
  70. amadeuszpawlik commented at 5:20 pm on March 2, 2022: contributor
    Thanks for catching this @prayank23 , changed the way -port and -rpcport are parsed. -port=1/2 and the like are caught and not defaulting to 1 anymore, and the error returns the exact string that has been passed as argument.
  71. unknown approved
  72. unknown commented at 5:28 pm on March 2, 2022: none

    tACK https://github.com/bitcoin/bitcoin/pull/22087/commits/51155b512a7acdc6950b71284c7711386c6f4a0c

    I could not find anymore issues however I am not confident and think this PR needs more fuzzing and review.

  73. luke-jr commented at 0:14 am on March 9, 2022: member 
     0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA256
 2
 3re-utACK 51155b512a7acdc6950b71284c7711386c6f4a0c
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQQzBAEBCAAdFiEE5GOpP18xF+7ebHMWvQKUJCH0iJ8FAmIn8TkACgkQvQKUJCH0
 7iJ9taCAAoTF45MLzSW5rL15SNoJcy+/H1z2QLKrpW6NzG5ktgY5aJNCJu+BR0bVQ
 8ZIit/5eKECGf2mXf+ZBpknGU/O4QZZHHe4qKC8A7AhWFVU0pgyDRE3U9Yir23Cev
 98oRb8fji21e5p2JuL/bU1WTUMla0B3YBmPt8DyzFe35k3DyOGiriDkh+dut8eWUn
10LkCe/JcVf8VgjpLAL2ZDGp9Drhu+V6y/JPMiFQ7IYdo5H70JErKjd9I6rCfaYg37
11jaMVNgK0yZ8G5SDBdhjITb4d7QK700/ZbHfqBRU3yXabGE4fJoRRkdnmbcMG4MV0
12iC/ZGb9yhhSFEK/rkJFqYca2png3In2gl/NsDklUG9+mYX3SwEyKat4oXc5n3d6L
13fjYezT9rXH6mjKWtnBvRXVfkE0X3bcbv70jI8fQKwCpHPbl9Y8mTlQPl7iZS+93c
1450zUUSWV4fgI5FTVbm9lAiCrXNEa8vQJzi5ZnSzKS5BDcQXdGFq3/sgfpDkTAg9f
15kA5JZtoB11t7nNObFZJVc5oN2QR4e6Hjs12ZguqAQQfZidXvLb7GPzU4ttjaod2y
16B+/PInkac+HiDfZ7EkeYTShB3WO/CxjISXGWDun/vUi0v2LfI8pI8oCOzMhNoXw+
17yPq5Sb3xxXJXM3k0TSyv1HWtLCnEWvm9GdEJe+Zrqr7ebcH/r1nRurPpXP8oA3et
18PqWeikv9r/s9FTAXjU766wmomivlKDqV2RAn9TT0unrKgMMQlC1UXp02jL9FoABX
196hsy/JY9HUQ41jY4fR2Oo4R5uuPfpOAkhffFDe30RZWoRIq5+zsStARsy9SSoeVt
20fxmzOV//pRELFY3VsQ6x3EFAj5yT2paXDadst2i0Aa40obI//iA42Qz7up7MAid6
21rB3ukbme8NnjDZ0Njc4aNGB6xdH/Cm71jvgA4bpDXWt5XQtLsyKk/EvzjIgnf6/q
22D0Iete9WYYGefYf3K+Nd/O7xyYwY64eeAveeSHJrTiwt2ht0DcLIasRiOpvYJzCu
23QfS/vzVMoZ0FyYBi7+ZRuckBLVBhmN86HuNb6zZy2BZIOJKcTv7pqTL7H77evygx
24R+DPgYWUBWOQfYnS7FyOqp/RQ5mQjiylxhtQdhDmhe00ZWthWcflwp9zNSNsnUxS
25jVjlMhZvBx/ehiOZzBpsOX3K9pY9zqAOLmMHaLrG0r0krMtwxBZ3otI9DrwNAvhw
26XxcR6Cu65b/8GUBB74/R864zhIi8xreUirkwjxXbA1byjgpppajyw/basoShsdlw
27fATGwITm4DjaURrvms4/ox4wFsO2IzwMEXTIL9Qb76eEI4sZzgbO+h3/IlxTbdSu
28sl2aM0INVY7FQ/2GHBcXKALHdglOrA==
29=EKhU
30-----END PGP SIGNATURE-----
  74. luke-jr changes_requested
  75. luke-jr commented at 9:22 pm on March 22, 2022: member

    Sorry, GCC is whining about the loops in init. Probably should make it happy.

    0init.cpp: In function ‘bool AppInitMain(NodeContext&, interfaces::BlockAndHeaderTipInfo*)’:
1init.cpp:1280:29: warning: loop variable ‘port_option’ of type ‘const string&’ {aka ‘const std::__cxx11::basic_string<char>&’} binds to a temporary constructed from type ‘const char* const’ [-Wrange-loop-construct]
2 1280 |     for (const std::string& port_option : {
3      |                             ^~~~~~~~~~~
4init.cpp:1280:29: note: use non-reference type ‘const string’ {aka ‘const std::__cxx11::basic_string<char>’} to make the copy explicit or ‘const char* const&’ to prevent copying
5init.cpp:1293:29: warning: loop variable ‘port_option’ of type ‘const string&’ {aka ‘const std::__cxx11::basic_string<char>&’} binds to a temporary constructed from type ‘const char* const’ [-Wrange-loop-construct]
6 1293 |     for (const std::string& port_option : {
7      |                             ^~~~~~~~~~~
8init.cpp:1293:29: note: use non-reference type ‘const string’ {aka ‘const std::__cxx11::basic_string<char>’} to make the copy explicit or ‘const char* const&’ to prevent copying
  76. luke-jr commented at 11:26 pm on March 22, 2022: member

    Looking at the options available, I think it’d be best to just drop the & in the loop for now (option A).

    0    for (const std::string port_option : {
1        "-port",
2        "-rpcport",
3    }) {

    Option B: Explicitly declare the type of the list. But I think this is slower than option A for no benefit.

    0    for (const std::string& port_option : std::vector<std::string>{
1        "-port",
2        "-rpcport",
3    }) {

    Option C: Use std::string_literals

    0    using namespace std::string_literals;
1    for (const std::string& port_option : {
2        "-port"s,
3        "-rpcport"s,
4    }) {

    Option D: Use std::array<std::string>? Kinda ugly to have to specify the number of items though IMO.

    0    for (const std::string& port_option : (const std::array<const std::string, 2>){
1        "-port",
2        "-rpcport",
3    }) {

    Option E: C++20 supposedly makes std::vector more flexible, so it could be a compile-time constexpr. But hopefully this gets merged before C++20 is required :)

  77. amadeuszpawlik force-pushed on Mar 25, 2022
  78. amadeuszpawlik force-pushed on Mar 27, 2022
  79. amadeuszpawlik force-pushed on Mar 30, 2022
  80. amadeuszpawlik commented at 3:15 pm on March 30, 2022: contributor
    Thanks @luke-jr , done! Rebased & fixed failing feature_proxy.py-tests
  81. in test/functional/feature_proxy.py:324 in c9ce2ccc3b outdated 
    319@@ -320,19 +320,34 @@ def networks_dict(d):
320 
321         self.stop_node(1)
322 
323-        self.log.info("Test passing invalid -proxy raises expected init error")
324-        self.nodes[1].extra_args = ["-proxy=abc:def"]
325-        msg = "Error: Invalid -proxy address or hostname: 'abc:def'"
326+        self.log.info("Test passing invalid -proxy hostname raises expected init error")
327+        self.nodes[1].extra_args = ["-proxy=abc:23456"]
    luke-jr commented at 11:44 pm on March 30, 2022:
    This now performs and depends on the outcome of a DNS lookup, I think. Need to ensure it always fails.
    amadeuszpawlik commented at 2:57 pm on March 31, 2022:

    Good catch. As per the RFC 2181:

    The DNS itself places only one restriction on the particular labels that can be used to identify resource records […] The length of any one label is limited to between 1 and 63 octets.

    I therefore propose to set the hostname with an invalid 0-length label (e.g. abc..abc), which should always fail.

  82. amadeuszpawlik force-pushed on Mar 31, 2022
  83. luke-jr approved
  84. luke-jr commented at 6:07 pm on April 13, 2022: member 
     0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA256
 2
 3re-utACK 1b6f8d3ea0876ede1fff97d725d2094f9e1dac68
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQQzBAEBCAAdFiEE5GOpP18xF+7ebHMWvQKUJCH0iJ8FAmJXETQACgkQvQKUJCH0
 7iJ8dfh/+JdJ73VBn/G/wmten/hIhXMMKZFGTAdC2cr/xVFq0cdst6KAfEf25J5YM
 86q0iadBHV8My/3BRTQPxpPxQMAD9pAwaol17yGxWpwQxD+Y9DVSfYjTYRwdJOTsw
 9ClKKcNu2nbBr6iNuE5XI0i9Y/D2ycx86cH9hDgKUwRh2BMFivh89awZ9I2zAP8UE
10+uWM3Kst5EC5cegkxIWj03idbrqHHPixq51Ium4Fr+XXokSbz9QSTNj6snwUujHr
11WROCqiNDB1x1BhZ4F48GRTh5IGTBMhEuTWPucKRxGS89064YxdV4UsQna7H8eJ5o
12kqmMeBcOTib70nsEirPPrFVeJUfrb6/XvD4TN5+8E8PBn9O3WkFLwnjzLbSscMgl
13uELth1n1vNFMu9UvKWwgxJJXBULvN+uFAaQzkbr0+L3GjiRKaCuYJNJBCojN15V4
14lRdALdZX6pgyvbYYwlQhKtbl69fGXAoO1ZSI3CRzrejfmqDOxI7E7w6rP/TN2LIC
15ifHb8q6u3Jza0B334bbZMBHwbFHIgC3TfOPjf1JJBgZNIq2l//KuqzN4xu8ZcTyJ
16A4UAH4JaWQoB1krU1HF112bIMPwon5ZrMiKZU4P0a3mOuSlwL294/z1rEjg8wUhe
17mHg9RdOVxdFGyQ/2ALc7SgpBOcQilaheQ2ARUw9osjSDqpCJYpHEhJ60L05TWDvp
187mn61pdiOc0Hq3YUKNPWzRkdslo5Lx/x0pjHZwMm2wRsz202SUquWosC0NhwMRX7
19vkINH/UFzKsG+k/mTUCx0OJSzmJGLSXRtE+ByozJGK3eEzvUx+sapJpsIHvBT98D
20S66neNcVZiS4pqR08MwtddcSs47pyNkWn6YXHAP7LOrkkD1gu2XRntiLLLETh4k8
21/8HrQrS3teIVVpCazcDCYIucH3Bd5n9LU8VWMX7a1Lw5ts6+mhQ06Qt/zKPApQ6E
22DrB5lQoeXzOIH/bUvXDF79WFU6yVyww28u8U4N65KUyVdh42fddRQiFGvvvcY9m4
23/Vq1hMUDOZHAkBhKMQ5rpoRa05jFQPHbaae70+6cvEGrMwDO6V7ON4avVtATOvpa
24S/DIFpDOpd0jKf/5l9TM6pm8FZAWK7vf2Qr8QkCL9gfFSnqQZiY2Bh8IYDpNCv0p
25c88Kfp6IQDJOK8XmtQdHPPYRC74QST+fIXp0QIwK2Qrf8lq/lbxxGY9YqR3lnz7f
26M6rKMbD2SxBs6J5H/EvYHb8PnKIalfCfnTQa4zYo7ogFirYxJc1IEe4gfTN2GoNm
27BwAwiVD0I5VLXXhGPuQcyWE6wlvl/RG4t7Xuin/9aAheel5+KbWB7Hk78FGLNmC/
28Lv0trYwADpAr2JFsrqaSXYQ/dydTQA==
29=oWrb
30-----END PGP SIGNATURE-----
  85. DrahtBot added the label Needs rebase on Apr 27, 2022
  86. amadeuszpawlik force-pushed on May 5, 2022
  87. DrahtBot removed the label Needs rebase on May 5, 2022
  88. fanquake referenced this in commit d5d40d59f8 on May 17, 2022
  89. luke-jr referenced this in commit 0a9c31fb55 on May 21, 2022
  90. luke-jr referenced this in commit 31be3a9e02 on May 21, 2022
  91. sidhujag referenced this in commit ec374e7e02 on May 28, 2022
  92. in src/init.cpp:1281 in ce4652aaa0 outdated 
    1311+        "-rpcbind",
1312+        "-torcontrol",
1313+        "-whitebind",
1314+        "-zmqpubhashblock",
1315+        "-zmqpubhashtx",
1316+        "-zmqpubrawblock",
    luke-jr commented at 9:56 pm on June 12, 2022:
    "-zmqpubrawtx", is missing ?
    amadeuszpawlik commented at 2:58 pm on June 13, 2022:
    resolved! thanks!
  93. amadeuszpawlik force-pushed on Jun 13, 2022
  94. in src/util/strencodings.cpp:107 in 1ad78b07c6 outdated 
    107     if (fHaveColon && (colon == 0 || fBracketed || !fMultiColon)) {
108-        uint16_t n;
109-        if (ParseUInt16(in.substr(colon + 1), &n)) {
110-            in = in.substr(0, colon);
111-            portOut = n;
112+        if (!in.substr(colon + 1).empty()) {
    ryanofsky commented at 4:18 pm on September 27, 2022:

    In commit “Validate port value in SplitHostPort” (1ad78b07c68f9564c583bc414e26a4323e483bbb)

    I’m not seeing why this new check for an empty port struct is necessary since it looks like ParseUInt16 should already return false for an empty string.

    If this check is not necessary, I think previous code without it is simpler and clearer. If it is necessary, it would be good to add a comment explaining why it is here.

    amadeuszpawlik commented at 6:21 pm on October 3, 2022:
    Agreed
  95. in src/util/strencodings.cpp:112 in 1ad78b07c6 outdated 
    114+            if (ParseUInt16(in.substr(colon+1), &n)) {
115+              portOut = n;
116+              valid = (portOut != 0);
117+            }
118         }
119+        in = in.substr(0, colon);
    ryanofsky commented at 4:27 pm on September 27, 2022:

    In commit “Validate port value in SplitHostPort” (1ad78b07c68f9564c583bc414e26a4323e483bbb)

    This is changing behavior in the case of an invalid port number. Previously if the port string was invalid portOut would be empty and the string would remain part of hostOut. Now portOut will be empty and the port string will be dropped completely, not added to hostOut.

    I don’t think the old behavior is great, but the new behavior doesn’t seem better, and I think if it is going to change at all it should be done as a separate commit. Safest thing for this commit just to add a new return value, not to change existing return values. This is especially important because the new return value is not marked [[nodiscard]] so there’s nothing that prevents code from relying on previous behavior.

    amadeuszpawlik commented at 5:05 pm on October 5, 2022:
    I agree, this PR is about validating port-options and not cleaning up the host string.
  96. in src/test/netbase_tests.cpp:87 in 1ad78b07c6 outdated 
    83@@ -84,31 +84,48 @@ BOOST_AUTO_TEST_CASE(netbase_properties)
84 
85 }
86 
87-bool static TestSplitHost(const std::string& test, const std::string& host, uint16_t port)
88+bool static TestSplitHost(const std::string& test, const std::string& host, uint16_t port, bool validPort)
    ryanofsky commented at 4:29 pm on September 27, 2022:

    In commit “Validate port value in SplitHostPort” (1ad78b07c68f9564c583bc414e26a4323e483bbb)

    Just a suggestion but if you set a default argument value bool validPort=true less code below would have to change and this would be a little easier to review.

  97. in src/init.cpp:1299 in 1dae86bfd2 outdated 
    1294+    for (const std::string port_option : {
1295+        "-port",
1296+        "-rpcport",
1297+    }) {
1298+        if (args.IsArgSet(port_option)) {
1299+            const std::string port = args.GetArgs(port_option)[0];
    ryanofsky commented at 5:21 pm on September 27, 2022:

    In commit “Validate port options” (1dae86bfd2259e4d9a9f1cd292a10cc17f626876)

    Suggest replacing args.GetArgs(port_option)[0] with args.GetArg(port_option). IsArgSet will true and the [0] subscript will be out of bounds if the argument is negated (-noport)

    amadeuszpawlik commented at 6:35 pm on October 3, 2022:
    Good catch, thanks
  98. ryanofsky commented at 5:54 pm on September 27, 2022: contributor

    Reviewed 1dae86bfd2259e4d9a9f1cd292a10cc17f626876, and it looks almost ok, but I am concerned different SplitHostPort hostOut values when port is invalid could potentially be bad for callers. Or even if it doesn’t break anything, it seems better just to add a additional return value not change hostOut values in this PR.

    It would also be good to note the change of behavior in the PR description or release notes that -port and -rpcport values that previously worked and were considered valid can now result in errors. For example port values with leading whitespace or trailing non-digit characters.

    I do think code in second commit is a little awkward because it is parsing and validating address and port options in different place where they are used. But probably this is fine, since in the future this will probably be cleaned up by introducing options structs and moving new code here to a function like ApplyArgsManOptions that does parsing and error-checking all in one place.

  99. fanquake commented at 3:28 pm on October 2, 2022: member
    @amadeuszpawlik any chance you’re still around / interested in responding to the latest review feedback?
  100. amadeuszpawlik commented at 8:00 am on October 3, 2022: contributor

    @amadeuszpawlik any chance you’re still around / interested in responding to the latest review feedback? @fanquake Yes, definitely. Will have a go at it shortly 👍

  101. amadeuszpawlik force-pushed on Oct 3, 2022
  102. amadeuszpawlik force-pushed on Oct 3, 2022
  103. amadeuszpawlik force-pushed on Oct 3, 2022
  104. amadeuszpawlik force-pushed on Oct 3, 2022
  105. in src/util/strencodings.cpp:108 in 7a7eb90959 outdated 
    106     bool fMultiColon{fHaveColon && colon != 0 && (in.find_last_of(':', colon - 1) != in.npos)};
107     if (fHaveColon && (colon == 0 || fBracketed || !fMultiColon)) {
108         uint16_t n;
109-        if (ParseUInt16(in.substr(colon + 1), &n)) {
110-            in = in.substr(0, colon);
111+        if (ParseUInt16(in.substr(colon+1), &n)) {
    luke-jr commented at 10:54 pm on October 3, 2022:
    Why are you changing this? It’s better with the spaces IMO
  106. amadeuszpawlik force-pushed on Oct 5, 2022
  107. amadeuszpawlik force-pushed on Oct 5, 2022
  108. Validate port value in `SplitHostPort` 
    Forward the validation of the port from `ParseUInt16(...)`.
Consider port 0 as invalid.
Add suitable test for the `SplitHostPort` function.
Add doxygen description to the `SplitHostPort` function.
    f8387c4234
  109. Validate `port` options 
    Check `port` options for invalid values (ports are parsed as uint16, so
in practice values >65535 are invalid; port 0 is undefined and therefore
considered invalid too). This allows for an early rejection of faulty
values and an supplying an informative message to the user.

Splits tests in `feature_proxy.py` to cover both invalid `hostname`
and `port` values.

Adds a release-note as previously valid `-port` and `-rpcport` values
can now result in errors.
    04526787b5
  110. amadeuszpawlik force-pushed on Oct 5, 2022
  111. amadeuszpawlik commented at 5:33 pm on October 5, 2022: contributor
    Thanks @ryanofsky for taking your time. I have now resolved all your comments and added a release-note.
  112. ryanofsky approved
  113. ryanofsky commented at 5:48 pm on October 5, 2022: contributor
    Code review ACK 04526787b5f6613d1f1ad78434e1dd24ab88dd76. Just suggested changes since last review: reverting some SplitHostPort changes, adding release notes, avoiding ‘GetArgs[0]` problem.
  114. luke-jr commented at 0:37 am on October 6, 2022: member
    utACK 04526787b5f6613d1f1ad78434e1dd24ab88dd76
  115. luke-jr commented at 0:39 am on October 6, 2022: member
    (In the future, though, please don’t rebase every time you make changes - only when needed)
  116. fanquake commented at 7:23 am on October 11, 2022: member
    @ryanofsky just fyi. You’ve re-ACK the previous commit hash in your latest comment
  117. ryanofsky commented at 4:23 pm on October 11, 2022: contributor

    @ryanofsky just fyi. You’ve re-ACK the previous commit hash in your latest comment

    Thank you, fixed now

  118. fanquake merged this on Oct 12, 2022
  119. fanquake closed this on Oct 12, 2022
  120. sidhujag referenced this in commit 21024d1719 on Oct 23, 2022
  121. bitcoin locked this on Jul 1, 2024


amadeuszpawlik jonatack DrahtBot klementtan laanwj jhay666 luke-jr thonkle ghost ryanofsky fanquake


laanwj

Labels
Utils/log/libs

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: 2024-11-21 18:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me