bitcoin · vasild · May 25, 2021 · May 25, 2021 · May 25, 2021 · Apr 29, 2021
diff --git a/src/i2p.cpp b/src/i2p.cpp
@@ -119,15 +119,13 @@ Session::Session(const fs::path& private_key_file,
     : m_private_key_file{private_key_file},
       m_control_host{control_host},
       m_interrupt{interrupt},
-      m_control_sock{std::make_unique<Sock>(INVALID_SOCKET)},
       m_transient{false}
 {
 }
 
 Session::Session(const CService& control_host, CThreadInterrupt* interrupt)
     : m_control_host{control_host},
       m_interrupt{interrupt},
-      m_control_sock{std::make_unique<Sock>(INVALID_SOCKET)},
       m_transient{true}
 {
 }
@@ -315,7 +313,7 @@ void Session::CheckControlSock()
     LOCK(m_mutex);
 
     std::string errmsg;
-    if (!m_control_sock->IsConnected(errmsg)) {
+    if (m_control_sock && !m_control_sock->IsConnected(errmsg)) {
         Log("Control socket error: %s", errmsg);
         Disconnect();
     }
@@ -364,7 +362,7 @@ Binary Session::MyDestination() const
 void Session::CreateIfNotCreatedAlready()
 {
     std::string errmsg;
-    if (m_control_sock->IsConnected(errmsg)) {
+    if (m_control_sock && m_control_sock->IsConnected(errmsg)) {
         return;
     }
 
@@ -437,14 +435,14 @@ std::unique_ptr<Sock> Session::StreamAccept()
 
 void Session::Disconnect()
 {
-    if (m_control_sock->Get() != INVALID_SOCKET) {
+    if (m_control_sock) {
         if (m_session_id.empty()) {
             Log("Destroying incomplete SAM session");
         } else {
             Log("Destroying SAM session %s", m_session_id);
         }
+        m_control_sock.reset();
     }
-    m_control_sock = std::make_unique<Sock>(INVALID_SOCKET);
     m_session_id.clear();
 }
 } // namespace sam

diff --git a/src/i2p.h b/src/i2p.h
@@ -261,6 +261,7 @@ class Session
      * ("SESSION CREATE"). With the established session id we later open
      * other connections to the SAM service to accept incoming I2P
      * connections and make outgoing ones.
+     * If not connected then this unique_ptr will be empty.
      * See https://geti2p.net/en/docs/api/samv3
      */
     std::unique_ptr<Sock> m_control_sock GUARDED_BY(m_mutex);

diff --git a/src/net.cpp b/src/net.cpp
@@ -427,12 +427,10 @@ static CAddress GetBindAddress(const Sock& sock)
     CAddress addr_bind;
     struct sockaddr_storage sockaddr_bind;
     socklen_t sockaddr_bind_len = sizeof(sockaddr_bind);
-    if (sock.Get() != INVALID_SOCKET) {
-        if (!sock.GetSockName((struct sockaddr*)&sockaddr_bind, &sockaddr_bind_len)) {
-            addr_bind.SetSockAddr((const struct sockaddr*)&sockaddr_bind);
-        } else {
-            LogPrintLevel(BCLog::NET, BCLog::Level::Warning, "getsockname failed\n");
-        }
+    if (!sock.GetSockName((struct sockaddr*)&sockaddr_bind, &sockaddr_bind_len)) {
+        addr_bind.SetSockAddr((const struct sockaddr*)&sockaddr_bind);
+    } else {
+        LogPrintLevel(BCLog::NET, BCLog::Level::Warning, "getsockname failed\n");
     }
     return addr_bind;
 }

diff --git a/src/netbase.cpp b/src/netbase.cpp
@@ -514,10 +514,6 @@ bool ConnectSocketDirectly(const CService &addrConnect, const Sock& sock, int nT
     // Create a sockaddr from the specified service.
     struct sockaddr_storage sockaddr;
     socklen_t len = sizeof(sockaddr);
-    if (sock.Get() == INVALID_SOCKET) {
-        LogPrintf("Cannot connect to %s: invalid socket\n", addrConnect.ToStringAddrPort());
-        return false;
-    }
     if (!addrConnect.GetSockAddr((struct sockaddr*)&sockaddr, &len)) {
         LogPrintf("Cannot connect to %s: unsupported network\n", addrConnect.ToStringAddrPort());
         return false;

diff --git a/src/test/fuzz/connman.cpp b/src/test/fuzz/connman.cpp
@@ -6,6 +6,7 @@
 #include <chainparams.h>
 #include <common/args.h>
 #include <net.h>
+#include <net_processing.h>
 #include <netaddress.h>
 #include <protocol.h>
 #include <test/fuzz/FuzzedDataProvider.h>
@@ -16,6 +17,7 @@
 #include <util/translation.h>
 
 #include <cstdint>
+#include <memory>
 #include <vector>
 
 namespace {
@@ -32,12 +34,29 @@ FUZZ_TARGET(connman, .init = initialize_connman)
 {
     FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
     SetMockTime(ConsumeTime(fuzzed_data_provider));
+
+    // Mock CreateSock() to create FuzzedSock.
+    auto CreateSockOrig = CreateSock;
+    CreateSock = [&fuzzed_data_provider](const CService&) {
+        return std::make_unique<FuzzedSock>(fuzzed_data_provider);
+    };
+
+    // Mock g_dns_lookup() to return a fuzzed address.
+    g_dns_lookup = [&fuzzed_data_provider](const std::string&, bool) {
+        return std::vector<CNetAddr>{ConsumeNetAddr(fuzzed_data_provider)};
+    };
+
     ConnmanTestMsg connman{fuzzed_data_provider.ConsumeIntegral<uint64_t>(),
                      fuzzed_data_provider.ConsumeIntegral<uint64_t>(),
                      *g_setup->m_node.addrman,
                      *g_setup->m_node.netgroupman,
                      fuzzed_data_provider.ConsumeBool()};
+    CConnman::Options options;
+    options.m_msgproc = g_setup->m_node.peerman.get();
+    connman.Init(options);
+
     CNetAddr random_netaddr;
+    CAddress random_address;
     CNode random_node = ConsumeNode(fuzzed_data_provider);
     CSubNet random_subnet;
     std::string random_string;
@@ -53,6 +72,9 @@ FUZZ_TARGET(connman, .init = initialize_connman)
             [&] {
                 random_netaddr = ConsumeNetAddr(fuzzed_data_provider);
             },
+            [&] {
+                random_address = ConsumeAddress(fuzzed_data_provider);
+            },
             [&] {
                 random_subnet = ConsumeSubNet(fuzzed_data_provider);
             },
@@ -118,6 +140,58 @@ FUZZ_TARGET(connman, .init = initialize_connman)
             },
             [&] {
                 connman.SetTryNewOutboundPeer(fuzzed_data_provider.ConsumeBool());
+            },
+            [&] {
+                const auto& to_addr{random_address};
+
+                const bool count_failure{fuzzed_data_provider.ConsumeBool()};
+
+                CSemaphoreGrant grant;
+                CSemaphoreGrant* grant_ptr{fuzzed_data_provider.ConsumeBool() ? nullptr : &grant};
+
+                const char* to_str{fuzzed_data_provider.ConsumeBool() ? nullptr :
+                                                                        random_string.c_str()};
+
+                ConnectionType conn_type{
+                    fuzzed_data_provider.PickValueInArray(ALL_CONNECTION_TYPES)};
+                if (conn_type == ConnectionType::INBOUND) { // INBOUND is not allowed
+                    conn_type = ConnectionType::OUTBOUND_FULL_RELAY;
+                }
+
+                connman.OpenNetworkConnection(to_addr, count_failure, grant_ptr, to_str, conn_type);
+            },
+            [&] {
+                connman.SetNetworkActive(true);
+
+                NetPermissionFlags permissions{
+                    ConsumeWeakEnum(fuzzed_data_provider, ALL_NET_PERMISSION_FLAGS)};
+                auto me = ConsumeAddress(fuzzed_data_provider);
+                auto peer = ConsumeAddress(fuzzed_data_provider);
+                auto sock = CreateSock(peer);
+
+                connman.CreateNodeFromAcceptedSocketPublic(std::move(sock), permissions, me, peer);
+            },
+            [&] {
+                CConnman::Options options;
+
+                options.vBinds = ConsumeServiceVector(fuzzed_data_provider, 5);
+
+                options.vWhiteBinds = std::vector<NetWhitebindPermissions>{
+                    fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, 5)};
+                for (auto& wb : options.vWhiteBinds) {
+                    wb.m_flags = ConsumeWeakEnum(fuzzed_data_provider, ALL_NET_PERMISSION_FLAGS);
+                    wb.m_service = ConsumeService(fuzzed_data_provider);
+                }
+
+                options.onion_binds = ConsumeServiceVector(fuzzed_data_provider, 5);
+
+                options.bind_on_any = options.vBinds.empty() && options.vWhiteBinds.empty() &&
+                                      options.onion_binds.empty();
+
+                connman.InitBindsPublic(options);
+            },
+            [&] {
+                connman.SocketHandlerPublic();
             });
     }
     (void)connman.GetAddedNodeInfo();
@@ -136,4 +210,5 @@ FUZZ_TARGET(connman, .init = initialize_connman)
     (void)connman.GetUseAddrmanOutgoing();
 
     connman.ClearTestNodes();
+    CreateSock = CreateSockOrig;
 }
diff --git a/src/test/fuzz/util/net.cpp b/src/test/fuzz/util/net.cpp
@@ -56,9 +56,10 @@ CAddress ConsumeAddress(FuzzedDataProvider& fuzzed_data_provider) noexcept
 }
 
 FuzzedSock::FuzzedSock(FuzzedDataProvider& fuzzed_data_provider)
-    : m_fuzzed_data_provider{fuzzed_data_provider}, m_selectable{fuzzed_data_provider.ConsumeBool()}
+    : Sock{fuzzed_data_provider.ConsumeIntegralInRange<SOCKET>(INVALID_SOCKET - 1, INVALID_SOCKET)},
+      m_fuzzed_data_provider{fuzzed_data_provider},
+      m_selectable{fuzzed_data_provider.ConsumeBool()}
 {
-    m_socket = fuzzed_data_provider.ConsumeIntegralInRange<SOCKET>(INVALID_SOCKET - 1, INVALID_SOCKET);
 }
 
 FuzzedSock::~FuzzedSock()

diff --git a/src/test/fuzz/util/net.h b/src/test/fuzz/util/net.h
@@ -95,6 +95,19 @@ inline CService ConsumeService(FuzzedDataProvider& fuzzed_data_provider) noexcep
     return {ConsumeNetAddr(fuzzed_data_provider), fuzzed_data_provider.ConsumeIntegral<uint16_t>()};
 }
 
+inline std::vector<CService> ConsumeServiceVector(FuzzedDataProvider& fuzzed_data_provider,
+                                                  size_t max_vector_size) noexcept
+{
+    std::vector<CService> ret;
+    const size_t size = fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, max_vector_size);
+    ret.reserve(size);
+    for (size_t i = 0; i < size; ++i) {
+        ret.emplace_back(ConsumeNetAddr(fuzzed_data_provider),
+                         fuzzed_data_provider.ConsumeIntegral<uint16_t>());
+    }
+    return ret;
+}
+
 CAddress ConsumeAddress(FuzzedDataProvider& fuzzed_data_provider) noexcept;
 
 template <bool ReturnUniquePtr = false>

diff --git a/src/test/sock_tests.cpp b/src/test/sock_tests.cpp
@@ -38,7 +38,6 @@ BOOST_AUTO_TEST_CASE(constructor_and_destructor)
 {
     const SOCKET s = CreateSocket();
     Sock* sock = new Sock(s);
-    BOOST_CHECK_EQUAL(sock->Get(), s);
     BOOST_CHECK(!SocketIsClosed(s));
     delete sock;
     BOOST_CHECK(SocketIsClosed(s));
@@ -51,22 +50,31 @@ BOOST_AUTO_TEST_CASE(move_constructor)
     Sock* sock2 = new Sock(std::move(*sock1));
     delete sock1;
     BOOST_CHECK(!SocketIsClosed(s));
-    BOOST_CHECK_EQUAL(sock2->Get(), s);
     delete sock2;
     BOOST_CHECK(SocketIsClosed(s));
 }
 
 BOOST_AUTO_TEST_CASE(move_assignment)
 {
-    const SOCKET s = CreateSocket();
-    Sock* sock1 = new Sock(s);
-    Sock* sock2 = new Sock();
+    const SOCKET s1 = CreateSocket();
+    const SOCKET s2 = CreateSocket();
+    Sock* sock1 = new Sock(s1);
+    Sock* sock2 = new Sock(s2);
+
+    BOOST_CHECK(!SocketIsClosed(s1));
+    BOOST_CHECK(!SocketIsClosed(s2));
+
     *sock2 = std::move(*sock1);
+    BOOST_CHECK(!SocketIsClosed(s1));
+    BOOST_CHECK(SocketIsClosed(s2));
+
     delete sock1;
-    BOOST_CHECK(!SocketIsClosed(s));
-    BOOST_CHECK_EQUAL(sock2->Get(), s);
+    BOOST_CHECK(!SocketIsClosed(s1));
+    BOOST_CHECK(SocketIsClosed(s2));
+
     delete sock2;
-    BOOST_CHECK(SocketIsClosed(s));
+    BOOST_CHECK(SocketIsClosed(s1));
+    BOOST_CHECK(SocketIsClosed(s2));
 }
 
 #ifndef WIN32 // Windows does not have socketpair(2).
@@ -98,7 +106,7 @@ BOOST_AUTO_TEST_CASE(send_and_receive)
     SendAndRecvMessage(*sock0, *sock1);
 
     Sock* sock0moved = new Sock(std::move(*sock0));
-    Sock* sock1moved = new Sock();
+    Sock* sock1moved = new Sock(INVALID_SOCKET);
     *sock1moved = std::move(*sock1);
 
     delete sock0;

diff --git a/src/test/util/net.h b/src/test/util/net.h
@@ -42,6 +42,24 @@ struct ConnmanTestMsg : public CConnman {
         m_nodes.clear();
     }
 
+    void CreateNodeFromAcceptedSocketPublic(std::unique_ptr<Sock> sock,
+                                            NetPermissionFlags permissions,
+                                            const CAddress& addr_bind,
+                                            const CAddress& addr_peer)
+    {
+        CreateNodeFromAcceptedSocket(std::move(sock), permissions, addr_bind, addr_peer);
+    }
+
+    bool InitBindsPublic(const CConnman::Options& options)
+    {
+        return InitBinds(options);
+    }
+
+    void SocketHandlerPublic()
+    {
+        SocketHandler();
+    }
+
     void Handshake(CNode& node,
                    bool successfully_connected,
                    ServiceFlags remote_services,
@@ -106,10 +124,10 @@ constexpr auto ALL_NETWORKS = std::array{
 class StaticContentsSock : public Sock
 {
 public:
-    explicit StaticContentsSock(const std::string& contents) : m_contents{contents}
+    explicit StaticContentsSock(const std::string& contents)
+        : Sock{INVALID_SOCKET},
+          m_contents{contents}
     {
-        // Just a dummy number that is not INVALID_SOCKET.
-        m_socket = INVALID_SOCKET - 1;
     }
 
     ~StaticContentsSock() override { m_socket = INVALID_SOCKET; }
@@ -192,6 +210,11 @@ class StaticContentsSock : public Sock
         return true;
     }
 
+    bool IsConnected(std::string&) const override
+    {
+        return true;
+    }
+
 private:
     const std::string m_contents;
     mutable size_t m_consumed{0};

diff --git a/src/util/sock.cpp b/src/util/sock.cpp
@@ -24,8 +24,6 @@ static inline bool IOErrorIsPermanent(int err)
     return err != WSAEAGAIN && err != WSAEINTR && err != WSAEWOULDBLOCK && err != WSAEINPROGRESS;
 }
 
-Sock::Sock() : m_socket(INVALID_SOCKET) {}
-
 Sock::Sock(SOCKET s) : m_socket(s) {}
 
 Sock::Sock(Sock&& other)
@@ -44,8 +42,6 @@ Sock& Sock::operator=(Sock&& other)
     return *this;
 }
 
-SOCKET Sock::Get() const { return m_socket; }
-
 ssize_t Sock::Send(const void* data, size_t len, int flags) const
 {
     return send(m_socket, static_cast<const char*>(data), len, flags);

diff --git a/src/util/sock.h b/src/util/sock.h
@@ -21,17 +21,11 @@
 static constexpr auto MAX_WAIT_FOR_IO = 1s;
 
 /**
- * RAII helper class that manages a socket. Mimics `std::unique_ptr`, but instead of a pointer it
- * contains a socket and closes it automatically when it goes out of scope.
+ * RAII helper class that manages a socket and closes it automatically when it goes out of scope.
  */
 class Sock
 {
 public:
-    /**
-     * Default constructor, creates an empty object that does nothing when destroyed.
-     */
-    Sock();
-
     /**
      * Take ownership of an existent socket.
      */
@@ -62,12 +56,6 @@ class Sock
      */
     virtual Sock& operator=(Sock&& other);
 
-    /**
-     * Get the value of the contained socket.
-     * @return socket or INVALID_SOCKET if empty
-     */
-    [[nodiscard]] virtual SOCKET Get() const;
-
     /**
      * send(2) wrapper. Equivalent to `send(this->Get(), data, len, flags);`. Code that uses this
      * wrapper can be unit tested if this method is overridden by a mock Sock implementation.