CService::port is uint16_t but is passed around the codebase and into the ctors as int, which causes uneeded conversions and casts. We can avoid these (including in the incoming I2P code without further changes to it) by using ports with the correct type. The remaining conversions are pushed out to the user input boundaries where they can be range-checked and raise with user feedback in the next patch.
82@@ -83,31 +83,31 @@ BOOST_AUTO_TEST_CASE(netbase_properties)
83
84 }
85
86-bool static TestSplitHost(std::string test, std::string host, int port)
87+bool static TestSplitHost(const std::string& test, const std::string& host, uint16_t port)
88 {
89 std::string hostOut;
90- int portOut = -1;
91+ uint16_t portOut = 1;
Usually 0 is used as “dummy/invalid” port. Port 1 is reserved for (from /etc/services):
0tcpmux 1/tcp #TCP Port Service Multiplexer
1tcpmux 1/udp #TCP Port Service Multiplexer
106@@ -107,7 +107,8 @@ std::vector<unsigned char> ParseHex(const std::string& str)
107 return ParseHex(str.c_str());
108 }
109
110-void SplitHostPort(std::string in, int &portOut, std::string &hostOut) {
111+void SplitHostPort(std::string in, uint16_t& portOut, std::string& hostOut)
112+{
(github does not let me to comment on line 118, so commenting on line 111 instead):
0118 int32_t n;
1119 if (ParseInt32(in.substr(colon + 1), &n) && n > 0 && n < 0x10000) {
2120 in = in.substr(0, colon);
3121 portOut = n;
4122 }
121 does not cause a warning?0x10000 to std::numeric_limits<uint16_t>::max().if (ParseUint32() && n is in [0,64k]) is used in other places, then maybe worth adding ParseUint16(). We have ParseUint{8,32,64}().
ParseUInt16() method and updated SplitHostPort() to use it.
ParseUInt32() and ParseUInt64() but not for ParseUInt8() or ParseUInt16(). Can add in a follow-up or here depending on reviewer preference.
289@@ -290,7 +290,7 @@ static bool ThreadHTTP(struct event_base* base)
290 /** Bind HTTP server to specified addresses */
291 static bool HTTPBindAddresses(struct evhttp* http)
292 {
293- int http_port = gArgs.GetArg("-rpcport", BaseParams().RPCPort());
294+ uint16_t http_port = gArgs.GetArg("-rpcport", BaseParams().RPCPort());
GetArg() returns uint64_t and here we would silently narrow it down to uint16_t. It is ok since before this PR we would narrow it down to int and later narrow the int down to uint16_t.
If the user makes a typo and specifies -rpcport=88332 we will bind on 22796 instead (with and without this PR). Best to print an error and refuse to start, not necessary in this PR.
ACK f8b1c2140eea958ab0e6f42ef2daab7000375d82
I think this PR is good and safe as it is. Some non-blocker comments below. It would be even better if it adds some checks that ports given by the user are in range, like in https://github.com/bitcoin/bitcoin/pull/8394/files#diff-134ccc4e5308f84fb9c03a9a1e1599209560370e49e30c7f545fa7b9ba7e07deR153
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
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.
Strong Concept ACK
Thanks for removing this implicit conversion gotcha :)
uint16_t should be used consistently for port numbers.
Somewhat related: #19415 (comment)
I like the idea of using specific types. But if we’re going to change this, I think I would feel slightly better defining a typedef Port for this. What if we want to support a protocol in the future with 32-bit ports? I do not see this as likely… my intuition is that things are moving to one-endpoint-per-address conceptually instead of ports… and yes the port as a concept is typical to current Internet protocols, where they’re all 16 bits. But I guess it’s clearer too to have a named type.
Anyhow concept ACK.
Not super excited by the introduction of Port TBH.
The problem is probably best illustrated by this UB quiz:
Q. Which of the two cases rely on undefined behaviour?
0// Snippet 1
1Port p;
2if (p != other_p) {
3}
0// Snippet 2
1uint16_t p;
2if (p != other_p) {
3}
Both snippets rely on undefined behaviour (they are technically equivalent thanks to the typedef).
But that’s not the point.
The point is that while the UB in snippet 2 is immediately clear (read of uninitialized uint16_t) the UB in snippet 1 requires looking up information that is not available in the code snippet (more specifically the typedef).
Trying to make it as easy as possible to spot unsafe code is an important part of safe coding practice.
Personally I find that the introduction of Port makes it somewhat harder to spot unsafe code as illustrated above.
316
317 // Bind addresses
318- for (std::vector<std::pair<std::string, uint16_t> >::iterator i = endpoints.begin(); i != endpoints.end(); ++i) {
319- LogPrint(BCLog::HTTP, "Binding RPC on address %s port %i\n", i->first, i->second);
320+ for (std::vector<std::pair<std::string, Port> >::iterator i = endpoints.begin(); i != endpoints.end(); ++i) {
321+ LogPrint(BCLog::HTTP, "Binding RPC on address %s port %u\n", i->first, i->second);
%i -> %u are not necessary because LogPrint() is type safe. The commit p2p: update port logging to unsigned integers can be dropped.
ACK 9b7055f9a9bd7345fc7dfb51ed0bace8257e07cd
Notice that this is not the last commit - I tend to agree with @practicalswift wrt uint16_t vs Port so I ACK the commit before those changes. Two more things to consider about Port:
Port may be too generic. I can imagine that “port” in some other context means something else, e.g. “usb port for connecting to a hardware wallet”. Maybe IPPort would be a better name.Port n; ParseUInt16(..., &n); which kind of defeats the purpose of the Port abstraction. I don’t see IP ports ever changing to something other than 16 bit unsigned integers.
Thanks for the feedback!
I agree that Port is less greppable than IPPort (and we do already have a ToStringIPPort() method).
No strong opinion, but here are a few reasons why I tend to agree with @laanwj and took the time to do it:
CAmount, for example; it makes clear to contributors which type should be used for amounts and the same would be true for port id numbersI’m not sure whether to wait for more feedback or separate the type alias to another pull. Keep the feedback coming :pray:
Q. Which of the two cases rely on undefined behaviour?
Don’t disagree with the point but whyy does C have to turn everything into UB, I hate this, I really do, wish we could use a sane language
I agree with MarcoFalke: if we are to introduce Port consider making it fully type safe in order to avoid the pitfall I described above.
Portability is important, but safety is more important (in general) :)
I suggest moving the Port commit to a separate PR since that is the only part of this PR that is not ready for merge.
Q. Which of the two cases rely on undefined behaviour?
Don’t disagree with the point but whyy does C have to turn everything into UB, I hate this, I really do, wish we could use a sane language
Heh, just don’t shoot the messenger! :)
ACK cf9952da0528b22af77c169822777587bb2a87ee
If in doubt - KISS.
Thanks @vasild! Updated one line per git diff a057361 52dd40a to make a change needed after the merge of #19415.
0+++ b/src/test/fuzz/netbase_dns_lookup.cpp
1@@ -18,7 +18,7 @@ FUZZ_TARGET(netbase_dns_lookup)
2 const std::string name = fuzzed_data_provider.ConsumeRandomLengthString(512);
3 const unsigned int max_results = fuzzed_data_provider.ConsumeIntegral<unsigned int>();
4 const bool allow_lookup = fuzzed_data_provider.ConsumeBool();
5- const int default_port = fuzzed_data_provider.ConsumeIntegral<int>();
6+ const uint16_t default_port = fuzzed_data_provider.ConsumeIntegral<uint16_t>();
Looks like master (at 05757aa86) has a problem that would be fixed by this PR - my “private” CI got this error:
0netbase.cpp:212:37: runtime error: implicit conversion from type 'int' of value -2147483632 (32-bit, signed) to type 'uint16_t' (aka 'unsigned short') changed the value to 16 (16-bit, unsigned)
which seems unrelated to the changes I am testing there.
Edit: also in another PR: https://cirrus-ci.com/task/6488952220680192?command=ci#L3334
