net: expand Sock and fuzz-test more of CConnman

vasild commented at 3:15 pm on April 15, 2021: member

Expand the Sock class
- Add wrapper methods for setsockopt(), getsockname(), bind() and listen()
- Convert the standalone functions SetSocketNoDelay() and IsSelectableSocket() to Sock methods
Expand the usage of Sock: CConnman::CreateNodeFromAcceptedSocket() and GetBindAddress() changed to take Sock as an argument instead of SOCKET
Add fuzz tests for OpenNetworkConnection(), CreateNodeFromAcceptedSocket() and InitBinds() CConnman methods.

net: add new method Sock::SetSockOpt() that wraps setsockopt()

This will help to increase `Sock` usage and make more code mockable.

6647e6af73

net: use Sock::SetSockOpt() instead of setsockopt() e2eafa88db

fanquake added the label P2P on Apr 15, 2021

practicalswift commented at 3:18 pm on April 15, 2021: contributor

Wow great work @vasild! Love it. Will review.

Super strong Concept ACK obviously :)

vasild force-pushed on Apr 15, 2021

DrahtBot commented at 7:26 pm on April 15, 2021: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#21506 (p2p, refactor: make NetPermissionFlags an enum class by jonatack)

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.

vasild force-pushed on Apr 16, 2021

vasild commented at 6:19 am on April 16, 2021: member

386676a9f...6b86187a8: use the proper socket variable after std::move

practicalswift commented at 9:34 am on April 16, 2021: contributor

Random note:

AFAICT the only remaining non-test usage of SOCKET Socket::Get() after this PR is this call in CreateSockTCP(…):

0    // Set the non-blocking option on the socket.
1    if (!SetSocketNonBlocking(sock->Get(), true)) {

What about adding Socket::SetNonBlocking(bool) as part of this PR? That would allow us to drop Socket::Get (from sock.h at least), which in turn would mean that there is no risk of a SOCKET “escaping” Socket (other than using Socket::Release, and Socket::Release makes is clear that the responsibility of closing is taken over by the caller).

WDYT? :)

vasild commented at 10:05 am on April 16, 2021: member

I actually did that, but then removed this change because I realized it is not necessary - CreateSockTCP() is never supposed to be fuzz- or unit- tested. In fuzz/unit tests we override the global CreateSock() to do something else than the real CreateSockTCP().

I think it is still useful to have Get(). If removed then what about the unit tests that use it? Maybe if at some point Sock is used all over the place, then both Get() and Release() could be removed.

practicalswift commented at 10:19 am on April 16, 2021: contributor

More random notes:

These are the direct uses of socket related syscalls and library functions with this PR applied:

 0$ git grep -E '[^a-z_:.](accept|bind|connect|getsockopt|listen|poll|recv|select|send|setsockopt|socket)\(' -- \
 1      "src/" ":(exclude)src/compat/stdin.cpp" ":(exclude)src/qt/" ":(exclude)src/test/" \
 2      ":(exclude)src/wallet/" | grep -vE "\.(cpp|h): *(//|\*|Log|\"|ListenSocket|throw|strprintf)"
 3src/net.cpp:            nBytes = send(node.hSocket, reinterpret_cast<const char*>(data.data()) + node.nSendOffset, data.size() - node.nSendOffset, MSG_NOSIGNAL | MSG_DONTWAIT);
 4src/net.cpp:    SOCKET hSocket = accept(hListenSocket.socket, (struct sockaddr*)&sockaddr, &len);
 5src/net.cpp:    if (poll(vpollfds.data(), vpollfds.size(), SELECT_TIMEOUT_MILLISECONDS) < 0) return;
 6src/net.cpp:    int nSelect = select(hSocketMax + 1, &fdsetRecv, &fdsetSend, &fdsetError, &timeout);
 7src/net.cpp:                nBytes = recv(pnode->hSocket, (char*)pchBuf, sizeof(pchBuf), MSG_DONTWAIT);
 8src/netbase.cpp:    SOCKET hSocket = socket(((struct sockaddr*)&sockaddr)->sa_family, SOCK_STREAM, IPPROTO_TCP);
 9src/util/sock.cpp:    return send(m_socket, static_cast<const char*>(data), len, flags);
10src/util/sock.cpp:    return recv(m_socket, static_cast<char*>(buf), len, flags);
11src/util/sock.cpp:    return connect(m_socket, addr, addr_len);
12src/util/sock.cpp:    return bind(m_socket, addr, addr_len);
13src/util/sock.cpp:    return listen(m_socket, backlog);
14src/util/sock.cpp:    return getsockopt(m_socket, level, opt_name, static_cast<char*>(opt_val), opt_len);
15src/util/sock.cpp:    return setsockopt(m_socket, level, opt_name, static_cast<const char*>(opt_val), opt_len);
16src/util/sock.cpp:    if (poll(&fd, 1, count_milliseconds(timeout)) == SOCKET_ERROR) {
17src/util/sock.cpp:    if (select(m_socket + 1, &fdset_recv, &fdset_send, nullptr, &timeout_struct) == SOCKET_ERROR) {

Great to see our socket related syscalls and library functions being further encapsulated by Socket (src/util/sock.cpp). It simplifies fuzzing, it simplifies syscall analysis and generally makes our socket use easier to reason about. Great stuff! :)

The remaining src/net.cpp cases are probably out of scope for this PR, but what about moving CreateSockTCP from src/netbase.{cpp,h} to src/util/sock.{cpp,h}. I think it belongs there and it would then make src/net.cpp the only remaining place where we make direct use of socket related syscalls and library functions without using Socket. WDYT @vasild?

vasild commented at 10:40 am on April 16, 2021: member

Moving CreateSockTCP() - it makes sense if it works (e.g. does not create a circular dependency).

Looking at the above list - it is not that many left! :) (btw close(2) is missing)

net: change CreateNodeFromAcceptedSocket() to take Sock

Change `CConnman::CreateNodeFromAcceptedSocket()` to take a `Sock`
argument instead of `SOCKET`.

This makes the method mockable and also a little bit shorter as some
`CloseSocket()` calls are removed (the socket will be closed
automatically by the `Sock` destructor on early return).

1a0e0ecbc5

net: convert standalone SetSocketNoDelay() to Sock::SetNoDelay()

This makes the callers mockable.

4cff13a36d

net: convert standalone IsSelectableSocket() to Sock::IsSelectable()

This makes the callers mockable.

22a2847c77

net: add new method Sock::GetSockName() that wraps getsockname()

This will help to increase `Sock` usage and make more code mockable.

23702fa911

net: add Sock method to check if it owns a socket

Mimic https://en.cppreference.com/w/cpp/memory/unique_ptr/operator_bool

462799293f

net: change GetBindAddress() to take Sock argument

This avoids the direct call to `getsockname()` and allows mocking.

7cec5ea86c

vasild force-pushed on Apr 16, 2021

vasild commented at 2:44 pm on April 16, 2021: member

6b86187a8...9155dee01: use pointers to Sock so that mocking via inheritance works properly

practicalswift commented at 9:23 pm on April 18, 2021: contributor

Here is a patch that removes some unused code (+22 -30), further encapsulates socket operations to Sock (src/util/sock.{cpp,h}) and gets rid of the last non-test use of Sock::Get.

Feel free to use as you wish! :)

 0diff --git a/src/netbase.cpp b/src/netbase.cpp
 1index b03fd1028..69ef63b0e 100644
 2--- a/src/netbase.cpp
 3+++ b/src/netbase.cpp
 4@@ -303,8 +303,7 @@ enum class IntrRecvError {
 5  *          read.
 6  *
 7  * [@see](/bitcoin-bitcoin/contributor/see/) This function can be interrupted by calling InterruptSocks5(bool).
 8- *      Sockets can be made non-blocking with SetSocketNonBlocking(const
 9- *      SOCKET&, bool).
10+ *      Sockets can be made non-blocking with Sock::SetNonBlocking().
11  */
12 static IntrRecvError InterruptibleRecv(uint8_t* data, size_t len, int timeout, const Sock& sock)
13 {
14@@ -520,7 +519,7 @@ std::unique_ptr<Sock> CreateSockTCP(const CService& address_family)
15     }
16
17     // Set the non-blocking option on the socket.
18-    if (!SetSocketNonBlocking(sock->Get(), true)) {
19+    if (!sock->SetNonBlocking()) {
20         LogPrintf("Error setting socket to non-blocking: %s\n", NetworkErrorString(WSAGetLastError()));
21         return nullptr;
22     }
23@@ -718,33 +717,6 @@ bool LookupSubNet(const std::string& strSubnet, CSubNet& ret, DNSLookupFn dns_lo
24     return false;
25 }
26
27-bool SetSocketNonBlocking(const SOCKET& hSocket, bool fNonBlocking)
28-{
29-    if (fNonBlocking) {
30-#ifdef WIN32
31-        u_long nOne = 1;
32-        if (ioctlsocket(hSocket, FIONBIO, &nOne) == SOCKET_ERROR) {
33-#else
34-        int fFlags = fcntl(hSocket, F_GETFL, 0);
35-        if (fcntl(hSocket, F_SETFL, fFlags | O_NONBLOCK) == SOCKET_ERROR) {
36-#endif
37-            return false;
38-        }
39-    } else {
40-#ifdef WIN32
41-        u_long nZero = 0;
42-        if (ioctlsocket(hSocket, FIONBIO, &nZero) == SOCKET_ERROR) {
43-#else
44-        int fFlags = fcntl(hSocket, F_GETFL, 0);
45-        if (fcntl(hSocket, F_SETFL, fFlags & ~O_NONBLOCK) == SOCKET_ERROR) {
46-#endif
47-            return false;
48-        }
49-    }
50-
51-    return true;
52-}
53-
54 void InterruptSocks5(bool interrupt)
55 {
56     interruptSocks5Recv = interrupt;
57diff --git a/src/util/sock.cpp b/src/util/sock.cpp
58index e560f5b90..7f9b6d17c 100644
59--- a/src/util/sock.cpp
60+++ b/src/util/sock.cpp
61@@ -107,6 +107,20 @@ bool Sock::SetNoDelay() const
62     return SetSockOpt(IPPROTO_TCP, TCP_NODELAY, &on, sizeof(on)) == 0;
63 }
64
65+bool Sock::SetNonBlocking() const
66+{
67+#ifdef WIN32
68+    const u_long one{1};
69+    if (ioctlsocket(m_socket, FIONBIO, &nOne) == SOCKET_ERROR) {
70+#else
71+    const int flags{fcntl(m_socket, F_GETFL, 0)};
72+    if (fcntl(m_socket, F_SETFL, flags | O_NONBLOCK) == SOCKET_ERROR) {
73+#endif
74+        return false;
75+    }
76+    return true;
77+}
78+
79 bool Sock::IsSelectable() const
80 {
81 #if defined(USE_POLL) || defined(WIN32)
82diff --git a/src/util/sock.h b/src/util/sock.h
83index 0ce2decd8..9e33b9159 100644
84--- a/src/util/sock.h
85+++ b/src/util/sock.h
86@@ -140,6 +140,12 @@ public:
87      */
88     [[nodiscard]] virtual bool SetNoDelay() const;
89
90+    /**
91+     * Set the non-blocking option on the socket.
92+     * [@return](/bitcoin-bitcoin/contributor/return/) true if set successfully
93+     */
94+    [[nodiscard]] virtual bool SetNonBlocking() const;
95+
96     /**
97      * Check if the underlying socket can be used for `select(2)` (or the `Wait()` method).
98      * [@return](/bitcoin-bitcoin/contributor/return/) true if selectable

net: add new method Sock::Bind() that wraps bind()

This will help to increase `Sock` usage and make more code mockable.

a0e2ff0a08

net: add new method Sock::Listen() that wraps listen()

This will help to increase `Sock` usage and make more code mockable.

4dc21c8b0f

vasild force-pushed on Apr 19, 2021

vasild commented at 12:53 pm on April 19, 2021: member

9155dee01...6a91cac15:

take the above patch that adds Sock::SetNonBlocking()
remove the newly added tests - they require “all or nothing” - to remove all occurrences of Sock::Get() and Sock::Release() otherwise dummy values of Sock::m_socket from the mocked Sock implementation leak into the main code during fuzzing.

This PR is reviewable and mergeable in its current state. I will see what it takes to remove Sock::Get() and Sock::Release() and decide whether to append to this PR or file a separate one.

net: convert standalone SetSocketNonBlocking() to Sock::SetNonBlocking()

This further encapsulates syscalls inside the `Sock` class.

Co-authored-by: practicalswift <practicalswift@users.noreply.github.com>

67097b29c0

vasild force-pushed on Apr 21, 2021

vasild commented at 12:52 pm on April 21, 2021: member

6a91cac15...67097b29c: fix Windows compilation

jonatack commented at 5:32 pm on April 27, 2021: member

Concept ACK

vasild commented at 4:18 pm on May 7, 2021: member

Closing: superseded by #21878 which replaces all usage of SOCKET with Sock and removes Sock::Get() and Sock::Release(), making it impossible to leak dummy file descriptors from mocked Sock implementations into the real code during tests (and accidentally call close(), write() or whatever on the dummy fds during tests).

vasild closed this on May 7, 2021

DrahtBot locked this on Aug 16, 2022

net: expand Sock and fuzz-test more of CConnman #21700

Conflicts