Make sure all ports are 16 bit numbers #8394

1. 
   UdjinM6 commented at 3:20 pm on July 23, 2016: contributor

   Ports should always be 16 bit numbers so use proper type (uint16_t) and format (%u). Also changing unsigned short to uint16_t for such cases for consistency.

   Reference: https://tools.ietf.org/html/rfc793#section-3.1

2. in src/test/netbase_tests.cpp: in 9fa9513307 outdated

   46@@ -47,10 +47,10 @@ BOOST_AUTO_TEST_CASE(netbase_properties)
   47     BOOST_CHECK(CNetAddr("127.0.0.1").IsValid());
   48 }
   49 
   50-bool static TestSplitHost(string test, string host, int port)
   51+bool static TestSplitHost(string test, string host, uint16_t port)
   52 {
   53     string hostOut;
   54-    int portOut = -1;
   55+    uint16_t portOut = -1;
   

   ---

   ---

   

   laanwj commented at 3:22 pm on July 23, 2016:

   This needs a larger type because it can be -1, which is not equal to 65535.

   ---

   

   UdjinM6 commented at 5:13 pm on July 23, 2016:

   True! Changed to use 0 instead.
3. 
   laanwj commented at 3:23 pm on July 23, 2016: member

   This probably overlaps a lot with @theuni’s net code overhaul.
4. 
   UdjinM6 commented at 5:17 pm on July 23, 2016: contributor

   Addressed test issue and changed SplitHostPort to return bool and fail if port is 0 or negative. Also added checks to SplitHostPort calls to further reassure that ports provided in strings are legit.
5. jonasschnelli added the label P2P on Jul 27, 2016
6. 
   theuni commented at 6:41 pm on July 27, 2016: member

   yes, this conflicts somewhat, mostly in net/netbase. I can deal with that though.

   More concerning though is the lack over overflow handling on input in a few places. For example:

   0uint16_t port = GetArg("-rpcport", BaseParams().RPCPort());
   

   what will “-rpcport=65537” do?

   I see a few of those, though admittedly some of them likely already exist. Also looks like a few are fixed, though :) @UdjinM6 Would you mind first ensuring that all input ports are sanitized (including those coming over the net) before doing the replacement?

7. 
   UdjinM6 commented at 11:10 pm on July 27, 2016: contributor

   Thanks for the review @theuni ! If that starts to overlap with your work too much, just tell me - I’m completely fine with closing this PR in favor of your more global work especially if this PR brings some pain in conflicts resolving and such. I would be glad if you any parts of this PR would be useful for your work in some way though.

   Meanwhile:

   • addressed GetArg inputs
   • added test cases for TestSplitHost for ports above upper bound

   I’m not sure which parts of code you were referencing by

   (including those coming over the net)

   Could you provide some guidance pls?

8. 
   theuni commented at 10:10 pm on August 5, 2016: member

   @UdjinM6 by “parts coming over the net”, i meant serialized addresses coming from net messages (version, addr, etc). Looks like that’s already taken care of.

   Concept ACK, and quick review ACK, but It’d be nice to get #8128 in first.

9. 
   MarcoFalke commented at 4:57 pm on August 18, 2016: member

   Needs rebase after #8128
10. Make sure all ports are 16 bit numbers 22e8605f4a
11. SplitHostPort should fail if provided port is 0 or negative 27c1c8967f
12. Validate ports in `GetArg` inputs and add test cases for `TestSplitHost` for ports above upper bound. 7523b76ae4
13. Fix new code after rebase 41028e7ac7
14. 
    UdjinM6 commented at 4:47 am on August 19, 2016: contributor

    Rebased and (re)added fixes to new code and to code which was moved in #8128.
15. in src/bitcoin-cli.cpp: in 41028e7ac7 outdated

    147@@ -148,7 +148,11 @@ static void http_request_done(struct evhttp_request *req, void *ctx)
    148 UniValue CallRPC(const string& strMethod, const UniValue& params)
    149 {
    150     std::string host = GetArg("-rpcconnect", DEFAULT_RPCCONNECT);
    151-    int port = GetArg("-rpcport", BaseParams().RPCPort());
    152+
    153+    int portTmp = GetArg("-rpcport", BaseParams().RPCPort());
    

    ---

    ---

    

    MarcoFalke commented at 8:15 am on August 19, 2016:

    Nit: GetArg returns int64_t. I think the goal of the pull was not to cast without check?

    ---

    

    UdjinM6 commented at 9:10 am on August 19, 2016:

    Agree, will fix
16. in src/bitcoin-cli.cpp: in 41028e7ac7 outdated

    147@@ -148,7 +148,11 @@ static void http_request_done(struct evhttp_request *req, void *ctx)
    148 UniValue CallRPC(const string& strMethod, const UniValue& params)
    149 {
    150     std::string host = GetArg("-rpcconnect", DEFAULT_RPCCONNECT);
    151-    int port = GetArg("-rpcport", BaseParams().RPCPort());
    152+
    153+    int portTmp = GetArg("-rpcport", BaseParams().RPCPort());
    154+    if(portTmp <= 0 || portTmp >= 0x10000)
    

    ---

    ---

    

    MarcoFalke commented at 8:22 am on August 19, 2016:

    Nit: Can you use !IsValidPort(), after changing the type of the arg of IsValidPort to int64_t?

    ---

    

    UdjinM6 commented at 9:20 am on August 19, 2016:

    That would require:

    1. include netbase.h
    2. move IsValidPort definition to netbase.h Not sure it worth it tbh.
17. in src/net.cpp: in 41028e7ac7 outdated

    2074@@ -2069,7 +2075,7 @@ void StartNode(boost::thread_group& threadGroup, CScheduler& scheduler)
    2075     if (pnodeLocalHost == NULL) {
    2076         CNetAddr local;
    2077         LookupHost("127.0.0.1", local, false);
    2078-        pnodeLocalHost = new CNode(INVALID_SOCKET, CAddress(CService(local, 0), nLocalServices));
    2079+        pnodeLocalHost = new CNode(INVALID_SOCKET, CAddress(CService(local, (uint16_t)0), nLocalServices));
    

    ---

    ---

    

    MarcoFalke commented at 8:25 am on August 19, 2016:

    Nit: I think this is too much handholding for developers. I don’t see the benefit considering that this will cause unneeded conflicts with other open pulls such as #8542
18. in src/test/addrman_tests.cpp: in 41028e7ac7 outdated

    91@@ -92,22 +92,22 @@ BOOST_AUTO_TEST_CASE(addrman_simple)
    92     BOOST_CHECK(addr_null.ToString() == "[::]:0");
    93 
    94     // Test 2: Does Addrman::Add work as expected.
    95-    CService addr1 = ResolveService("250.1.1.1", 8333);
    96+    CService addr1 = ResolveService("250.1.1.1", (uint16_t)8333);
    

    ---

    ---

    

    MarcoFalke commented at 8:27 am on August 19, 2016:

    Nit: Same here. I don’t think this is worth the diff. You already changed the interface of ResolveService(), which should be enough, imo.
19. 
    laanwj commented at 9:00 am on August 19, 2016: member

    To be honest, I’m not entirely convinced that merging this is worth the risk (smaller integers are easier to overflow, for example). There’s a huge number of changes but no user-facing improvement.
20. Address nits:

    - use int64_t to get result from GetArg
    - use int64_t for IsPortValid()
    - remove manual numbers conversion to uint16_t

    fb8b5488c2
21. 
    MarcoFalke commented at 9:29 am on August 19, 2016: member

    @laanwj Agree that there are a lot of changes, but I think in the long run we should make sure that arguments are parsed correctly and sanitized. I think this pull improves parsing of port numbers passed in by users.
22. 
    laanwj commented at 10:12 am on August 19, 2016: member

    Improving parsing is a good thing, if we can factor that out I’d have no problem with it.
23. 
    laanwj commented at 6:47 pm on October 18, 2016: member

    Closing this for now.
24. laanwj closed this on Oct 18, 2016

    ---

25. MarcoFalke locked this on Sep 8, 2021

Contributors
UdjinM6 laanwj theuni MarcoFalke

Labels
P2P