Merge bitcoin#31022: test: Add mockable steady clock, tests for PCP and NATPMP implementations

0f716f2 qa: cover PROTOCOL_ERROR variant in PCP unit tests (Antoine Poinsot)
fc700bb test: Add tests for PCP and NATPMP implementations (laanwj)
caf9521 net: Use mockable steady clock in PCP implementation (laanwj)
0364832 util: Add mockable steady_clock (laanwj)
ab1d3ec net: Add optional length checking to CService::SetSockAddr (laanwj)

Pull request description:

  Add a NodeSteadyClock, a steady_clock that can be mocked with millisecond precision. Use this in the PCP implementation.

  Then add a mock for a simple scriptable UDP server,, which is used to test various code paths (including successful mappings, timeouts and errors) in the PCP and NATPMP implementations.

  Includes "net: Add optional length checking to CService::SetSockAddr" from bitcoin#31014 as a prerequisite.

ACKs for top commit:
  darosior:
    re-ACK 0f716f2
  i-am-yuvi:
    Concept ACK 0f716f2
  achow101:
    ACK 0f716f2

Tree-SHA512: 6f91b24e6fe46a3fded7a13972efd77c98e6ef235f8898e4ae44068c5df32d1cdabb22cb66c351b338dc98cb2073b624e43607a28107f4999302bfbe7a138229

Author: achow101

src/common/netif.cpp

@@ -169,16 +169,9 @@ std::optional<CNetAddr> QueryDefaultGatewayImpl(sa_family_t family)
 
 std::optional<CNetAddr> FromSockAddr(const struct sockaddr* addr)
 {
-    // Check valid length. Note that sa_len is not part of POSIX, and exists on MacOS and some BSDs only, so we can't
-    // do this check in SetSockAddr.
-    if (!(addr->sa_family == AF_INET && addr->sa_len == sizeof(struct sockaddr_in)) &&
-        !(addr->sa_family == AF_INET6 && addr->sa_len == sizeof(struct sockaddr_in6))) {
-        return std::nullopt;
-    }
-
     // Fill in a CService from the sockaddr, then drop the port part.
     CService service;
-    if (service.SetSockAddr(addr)) {
+    if (service.SetSockAddr(addr, addr->sa_len)) {
         return (CNetAddr)service;
     }
     return std::nullopt;

src/common/pcp.cpp

@@ -235,9 +235,9 @@ std::optional<std::vector<uint8_t>> PCPSendRecv(Sock &sock, const std::string &p
         }
 
         // Wait for response(s) until we get a valid response, a network error, or time out.
-        auto cur_time = time_point_cast<milliseconds>(steady_clock::now());
+        auto cur_time = time_point_cast<milliseconds>(MockableSteadyClock::now());
         auto deadline = cur_time + timeout_per_try;
-        while ((cur_time = time_point_cast<milliseconds>(steady_clock::now())) < deadline) {
+        while ((cur_time = time_point_cast<milliseconds>(MockableSteadyClock::now())) < deadline) {
             Sock::Event occurred = 0;
             if (!sock.Wait(deadline - cur_time, Sock::RECV, &occurred)) {
                 LogPrintLevel(BCLog::NET, BCLog::Level::Warning, "%s: Could not wait on socket: %s\n", protocol, NetworkErrorString(WSAGetLastError()));
@@ -426,7 +426,7 @@ std::variant<MappingResult, MappingError> PCPRequestPortMap(const PCPMappingNonc
         return MappingError::NETWORK_ERROR;
     }
     CService internal;
-    if (!internal.SetSockAddr((struct sockaddr*)&internal_addr)) return MappingError::NETWORK_ERROR;
+    if (!internal.SetSockAddr((struct sockaddr*)&internal_addr, internal_addrlen)) return MappingError::NETWORK_ERROR;
     LogPrintLevel(BCLog::NET, BCLog::Level::Debug, "pcp: Internal address after connect: %s\n", internal.ToStringAddr());
 
     // Build request packet. Make sure the packet is zeroed so that reserved fields are zero

src/net.cpp

@@ -387,7 +387,7 @@ static CAddress GetBindAddress(const Sock& sock)
     struct sockaddr_storage sockaddr_bind;
     socklen_t sockaddr_bind_len = sizeof(sockaddr_bind);
     if (!sock.GetSockName((struct sockaddr*)&sockaddr_bind, &sockaddr_bind_len)) {
-        addr_bind.SetSockAddr((const struct sockaddr*)&sockaddr_bind);
+        addr_bind.SetSockAddr((const struct sockaddr*)&sockaddr_bind, sockaddr_bind_len);
     } else {
         LogPrintLevel(BCLog::NET, BCLog::Level::Warning, "getsockname failed\n");
     }
@@ -1746,7 +1746,7 @@ void CConnman::AcceptConnection(const ListenSocket& hListenSocket) {
         return;
     }
 
-    if (!addr.SetSockAddr((const struct sockaddr*)&sockaddr)) {
+    if (!addr.SetSockAddr((const struct sockaddr*)&sockaddr, len)) {
         LogPrintLevel(BCLog::NET, BCLog::Level::Warning, "Unknown socket family\n");
     } else {
         addr = CAddress{MaybeFlipIPv6toCJDNS(addr), NODE_NONE};

src/netaddress.cpp

@@ -807,13 +807,15 @@ CService::CService(const struct sockaddr_in6 &addr) : CNetAddr(addr.sin6_addr, a
    assert(addr.sin6_family == AF_INET6);
 }
 
-bool CService::SetSockAddr(const struct sockaddr *paddr)
+bool CService::SetSockAddr(const struct sockaddr *paddr, socklen_t addrlen)
 {
     switch (paddr->sa_family) {
     case AF_INET:
+        if (addrlen != sizeof(struct sockaddr_in)) return false;
         *this = CService(*(const struct sockaddr_in*)paddr);
         return true;
     case AF_INET6:
+        if (addrlen != sizeof(struct sockaddr_in6)) return false;
         *this = CService(*(const struct sockaddr_in6*)paddr);
         return true;
     default:

src/netaddress.h

@@ -539,7 +539,14 @@ class CService : public CNetAddr
     explicit CService(const struct sockaddr_in& addr);
     uint16_t GetPort() const;
     bool GetSockAddr(struct sockaddr* paddr, socklen_t* addrlen) const;
-    bool SetSockAddr(const struct sockaddr* paddr);
+    /**
+     * Set CService from a network sockaddr.
+     * @param[in] paddr Pointer to sockaddr structure
+     * @param[in] addrlen Length of sockaddr structure in bytes. This will be checked to exactly match the length of
+     * a socket address of the provided family, unless std::nullopt is passed
+     * @returns true on success
+     */
+    bool SetSockAddr(const struct sockaddr* paddr, socklen_t addrlen);
     /**
      * Get the address family
      * @returns AF_UNSPEC if unspecified

src/test/CMakeLists.txt

@@ -87,6 +87,7 @@ add_executable(test_bitcoin
   netbase_tests.cpp
   node_warnings_tests.cpp
   orphanage_tests.cpp
+  pcp_tests.cpp
   peerman_tests.cpp
   pmt_tests.cpp
   policy_fee_tests.cpp