The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
LookupIntern, Lookup and LookupHost by brunoerg)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.
8+
9+SocketAddrError hasValidHostPort(std::string_view socketAddr)
10+{
11+ uint16_t port{0};
12+ std::string hostname;
13+ bool validPort = SplitHostPort(socketAddr, port, hostname);
Build error here
0util/network.cpp:13:10: error: cannot initialize a variable of type 'bool' with an rvalue of type 'void'
1 bool validPort = SplitHostPort(socketAddr, port, hostname);
2 ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
31 error generated.
14+
15+ SocketAddrError res = SocketAddrError::OK;
16+ if (!validPort || port == 0)
17+ res = SocketAddrError::NO_PORT;
18+ if (hostname.empty() || hostname.find(' ') != std::string::npos)
19+ res = (res == SocketAddrError::NO_PORT) ? SocketAddrError::NO_HOSTPORT
0 res = (res == SocketAddrError::NO_PORT) ? SocketAddrError::NO_HOSTPORT
to appease the whitespace linter
0+ res = (res == SocketAddrError::NO_PORT) ? SocketAddrError::NO_HOSTPORT
1^---- failure generated from lint-whitespace.py
116+ valid = (portOut != 0);
117+ }
118 }
119+ in = in.substr(0, colon);
120+ } else {
121+ valid = true;
This contains in.substr(colon + 1) two times. Maybe put it in a variable (port_str?) to make the code a bit easier to read.
There are a bunch of style issues. I would suggest to run clang-format or contrib/devtools/clang-format-diff.py on this.
92+ * @param[in] in The socket address string to split.
93+ * @param[out] portOut Port-portion of the input, if found and parsable.
94+ * @param[out] hostOut Host-portion of the input, if found.
95+ * @return true if port-portion is absent or within its allowed range, otherwise false
96+ */
97+bool SplitHostPort(std::string_view in, uint16_t& portOut, std::string& hostOut);
doc/developer-notes.md.
12+ NO_HOST,
13+ NO_PORT,
14+ NO_HOSTPORT,
15+};
16+
17+SocketAddrError hasValidHostPort(std::string_view socketAddr);
In commit d8f230435bd828a80c56c66d4e0a7db11fd627ce util: Adds network, socket address validation this is added like:
0SocketAddrError hasValidHostPort(std::string_view socketAddr) {
1
2#endif // BITCOIN_UTIL_NETWORK_H
which I guess will not compile. All commits should compile on their own.
netbase.cpp, looks in a similar category as e.g. ParseNetwork().
5+#ifndef BITCOIN_UTIL_NETWORK_H
6+#define BITCOIN_UTIL_NETWORK_H
7+
8+#include <string_view>
9+
10+enum class SocketAddrError {
This utility implements the function `hasValidHostPort`, which conducts
a sanity check for a valid host:port combination in a string.
This can be used for rejection of invalid option strings. Rules for
a valid hostname are: not empty and no spaces, for port: not empty
and within `uint16` bounds.
- Returns error at init if `-torcontrol` is not a valid host:port combo.
- `StartTorControl` returns a `bool`, and has a return parameter `error`
where a suitable message is set at failure.
Fixes #23589
83@@ -84,6 +84,43 @@ BOOST_AUTO_TEST_CASE(netbase_properties)
84
85 }
86
87+bool static TestHasHostPort(const std::string& test, bool hasHost, bool hasPort)
88+{
89+ NetworkAddrError ret = HasValidHostPort(test);
90+ switch (ret) {
91+ case NetworkAddrError::OK:
style: case should be below switch
It is good to use clang-format or contrib/devtools/clang-format-diff.py to avoid such suggestions.
657@@ -658,23 +658,47 @@ static void TorControlThread(CService onion_service_target)
658 event_base_dispatch(gBase);
659 }
660
661-void StartTorControl(CService onion_service_target)
662+bool TorControlOptCheck(bilingual_str& error)
663+{
664+ NetworkAddrError eOpt = HasValidHostPort(gArgs.GetArg("-torcontrol", DEFAULT_TOR_CONTROL));
doc/developer-notes.md variables should use snake_case.
671+ return false;
672+ case NetworkAddrError::NO_PORT:
673+ error = Untranslated("No port specified in -torcontrol");
674+ return false;
675+ case NetworkAddrError::NO_HOSTPORT:
676+ error = Untranslated("No hostname:port specified in torcontrol");
nit: for consistency with above
0 error = Untranslated("No hostname:port specified in -torcontrol");
ACK 5be4336fc5b0ca17fbd27998d5aa17bb83ab4d40
The code looks ok, but I think it can be better: what about doing the checks in SplitHostPort() and not adding a new function HasValidHostPort()? The former already does some validity checks and it makes sense to do all checks in one place.
Then the code would look something like this:
0/**
1 * Splits a host:port string into host and port.
2 * Validates the host and the port.
3 *
4 * [@param](/bitcoin-bitcoin/contributor/param/)[in] in The string to split.
5 * [@param](/bitcoin-bitcoin/contributor/param/)[out] host Host-portion of the input, if found and valid.
6 * [@param](/bitcoin-bitcoin/contributor/param/)[out] port Port-portion of the input, if found and valid.
7 */
8void SplitHostPort(std::string_view in,
9 std::optional<std::string>& host,
10 std::optional<uint16_t>& port);
11
12...
13
14for (const auto& [option, port_required] :
15 std::vector<std::tuple<const char*, bool>>{{"-i2psam", true},
16 {"-onion", true},
17 {"-proxy", true},
18 {"-rpcbind", false},
19 {"-torcontrol", true},
20 {"-whitebind", false},
21 {"-zmqpubhashblock", true},
22 {"-zmqpubhashtx", true},
23 {"-zmqpubrawblock", true},
24 {"-zmqpubrawtx", true},
25 {"-zmqpubsequence", true}}) {
26 for (const std::string& str : args.GetArgs(option)) {
27 std::optional<std::string> host;
28 std::optional<uint16_t> port;
29 SplitHostPort(str, host, port);
30 if (!host.has_value()) {
31 return InitError(InvalidHostErrMsg(option, str));
32 }
33 if (port_required && !port.has_value()) {
34 return InitError(InvalidPortErrMsg(option, str));
35 }
36 }
37}
That would benefit/check also the other options, not just -torcontrol and there would not be a need to modify StartTorControl().
