Introduce a class to manage the lifetime of a socket - when the object that contains the socket goes out of scope, the underlying socket will be closed.
In addition, the new Sock class has a Send(), Recv() and Wait()
methods that can be overridden by unit tests to mock the socket
operations.
The Wait() method also hides the
#ifdef USE_POLL poll() #else select() #endif technique from higher
level code.
The following two PRs would benefit from this:
0@@ -0,0 +1,134 @@
1+// Copyright (c) 2012-2020 The Bitcoin Core developers
2+// Distributed under the MIT software license, see the accompanying
3+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
4+
5+#include <compat.h>
6+#include <netaddress.h>
7+#include <netbase.h>
0#include <netbase.h> // For CloseSocket
It seems this is the only Bitcoin Core related dependency. Are the other includes needed at all? If not, it would be good to move this module to util, which is the place for general utility stuff (not bitcoin related).
No, the other includes are not needed (and I removed them). However there is a circular dependency now:
netbase.cpp uses the Sock class, of course)sock.cpp uses MillisToTimeval() and CloseSocket() which are defined in netbase.cpp)One way to resolve that is to put the Sock class in netbase.{h,cpp} and ditch sock.{h,cpp}.
Another one would be to move MillisToTimeval() and CloseSocket() outside of netbase.cpp, somewhere in src/util/ and use them from both netbase.cpp and from sock.cpp.
Hmm…
One way to resolve that is to put the
Sockclass innetbase.{h,cpp}…
Did that.
Concept ACK, was unable to build this pull @ 99b8e88a15cb12483f28079d0f91fe8cf361da56
0$ gcc --version
1gcc (Debian 10.2.1-1) 10.2.1 20201207
2
3.../...
4
5 AR qt/libbitcoinqt.a
6/usr/bin/ld: libbitcoin_common.a(libbitcoin_common_a-netbase.o): in function `std::_MakeUniq<Sock>::__single_object std::make_unique<Sock, unsigned int&>(unsigned int&)':
7/usr/include/c++/10/bits/unique_ptr.h:962: undefined reference to `Sock::Sock(unsigned int)'
8 CXXLD qt/bitcoin-qt
9 CXXLD bitcoin-gui
10/usr/bin/ld: libbitcoin_common.a(libbitcoin_common_a-netbase.o): in function `std::_MakeUniq<Sock>::__single_object std::make_unique<Sock, unsigned int&>(unsigned int&)':
11/usr/include/c++/10/bits/unique_ptr.h:962: undefined reference to `Sock::Sock(unsigned int)'
12collect2: error: ld returned 1 exit status
13make[2]: *** [Makefile:5503: bitcoin-node] Error 1
14make[2]: *** Waiting for unfinished jobs....
15collect2: error: ld returned 1 exit status
16make[2]: *** [Makefile:5515: bitcoind] Error 1
17/usr/bin/ld: libbitcoin_common.a(libbitcoin_common_a-netbase.o): in function `std::_MakeUniq<Sock>::__single_object std::make_unique<Sock, unsigned int&>(unsigned int&)':
18/usr/include/c++/10/bits/unique_ptr.h:962: undefined reference to `Sock::Sock(unsigned int)'
19/usr/bin/ld: libbitcoin_common.a(libbitcoin_common_a-netbase.o): in function `std::_MakeUniq<Sock>::__single_object std::make_unique<Sock, unsigned int&>(unsigned int&)':
20/usr/include/c++/10/bits/unique_ptr.h:962: undefined reference to `Sock::Sock(unsigned int)'
21collect2: error: ld returned 1 exit status
22make[2]: *** [Makefile:5521: qt/bitcoin-qt] Error 1
23collect2: error: ld returned 1 exit status
24make[2]: *** [Makefile:5499: bitcoin-gui] Error 1
25make[2]: Leaving directory '/home/jon/projects/bitcoin/bitcoin/src'
26make[1]: *** [Makefile:14882: all-recursive] Error 1
27make[1]: Leaving directory '/home/jon/projects/bitcoin/bitcoin/src'
28make: *** [Makefile:806: all-recursive] Error 1
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.
348@@ -347,15 +349,15 @@ enum class IntrRecvError {
349 * Sockets can be made non-blocking with SetSocketNonBlocking(const
350 * SOCKET&, bool).
351 */
352-static IntrRecvError InterruptibleRecv(uint8_t* data, size_t len, int timeout, const SOCKET& hSocket)
353+static IntrRecvError InterruptibleRecv(uint8_t* data, size_t len, int timeout, const Sock& sock)
Sock? (Something similar to Sock::recv but with a timeout parameter)
Yes, indeed. That would be convenient together with a send method that retries upon temporary failures (see below).
I did not include such methods in this PR because it is self-sufficient as it is and achieves the initial purpose to introduce a mockable socket that can be used for fuzzing in #19203.
Possible future improvements (which I resisted adding in this PR):
InterruptibleRecv() (or change that function to a method of the Sock class)SendComplete() from #20685Sock in more places instead of the bare SOCKET - chase all places where Sock::Release() or Sock::Get() is called and see if the consumer can be changed to use Sock instead. E.g. in CNode constructor.
Strong concept ACK
This abstraction would be extremely useful to have when fuzzing low-level networking code.
99b8e88a1…408a9473b: moved the Sock class inside netbase.cpp to resolve a circular dependency.
Another option would be to keep the Sock class in its own file src/sock.cpp or src/util/sock.cpp. Since it uses MillisToTimeval() and CloseSocket() they would have to be moved out of netbase.cpp.
122+ */
123+ virtual ssize_t Send(const void* data, size_t len, int flags) const;
124+
125+ /**
126+ * recv(2) wrapper. Equivalent to `recv(this->Get(), buf, len, flags);`. Code that uses this
127+ * wrapper can be unit-tested if this method is overriden by a mock Sock implementation.
496- CAddress addr_bind = GetBindAddress(hSocket);
497- CNode* pnode = new CNode(id, nLocalServices, GetBestHeight(), hSocket, addrConnect, CalculateKeyedNetGroup(addrConnect), nonce, addr_bind, pszDest ? pszDest : "", conn_type);
498+ CAddress addr_bind = GetBindAddress(sock->Get());
499+ CNode* pnode = new CNode(id, nLocalServices, GetBestHeight(), sock->Release(), addrConnect,
500+ CalculateKeyedNetGroup(addrConnect), nonce, addr_bind,
501+ pszDest ? pszDest : "", conn_type);
nit: i would like to suggest rename Sock::Release as Sock::GetAndRelease. IMO it makes it just a tiny bit more readable. Feel free to ignore this comment.
0 CNode* pnode = new CNode(id, nLocalServices, GetBestHeight(), sock->GetAndRelease(), addrConnect,
1 CalculateKeyedNetGroup(addrConnect), nonce, addr_bind,
2 pszDest ? pszDest : "", conn_type);
GetAndRelease() is more descriptive, but I am trying to mimic std::unique_ptr and its method which is called release().
Tested approach ACK 408a9473bed6b3be82b124c171435247100b9d34 (modulo missing test coverage?)
607+ssize_t Sock::Recv(void* buf, size_t len, int flags) const
608+{
609+ return recv(m_socket, static_cast<char*>(buf), len, flags);
610+}
611+
612+bool Sock::Wait(const std::chrono::milliseconds& timeout, Event requested, Event* returned) const
returned parameter is never used by any caller. It seems this is something that was added for future usage maybe. Does it make sense to have it at this point in time, I mean, if tomorrow it is needed then it could be added in a future PR.
Sock object in CNode?
moving the Sock class to its own file and the common deps to a utility seems appealing
I will try this. Opinions, anybody? That would mean:
Sock from netbase.cpp to util/sock.cppMillisToTimeval() from netbase.cpp to util/where?CloseSocket() from netbase.cpp to util/where? (it uses LogPrintf())do you plan to add unit tests for the Sock class?
I did not but any ideas are welcome. I think it may be required to create a TCP server and client and do some communication between them to check that send() is indeed being called by the Send() method. May be an overkill, given that the Send() method is 1 line.
I don’t know how to reliably check if the socket was closed by the destructor, given that the same file descriptor number could be assigned to a newly created socket (or opened file) after we close it. @jnewbery
Is there any reason not to use the
Sockobject inCNode?
No, other than minimizing the size of this PR. See also #20788 (review) :-)
I agree with moving it to its own file and with adding UTs for it. I am not sure a real socket is necessary for testing this abstraction, could it be enough to simply mock send, recv and select? Something it could be useful is to make sure the ownership of the SOCKET is always correct (copy ctor, Release, operator =)
btw, utACK 8fc63e344660f52a35bd97c1f881afdb1643b52a
8fc63e3…3fa2a11:
Sock to its dedicated src/util/sock.{cpp,h}0@@ -0,0 +1,150 @@
1+// Copyright (c) 2021-2021 The Bitcoin Core developers
2+// Distributed under the MIT software license, see the accompanying
3+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
4+
5+#ifndef WIN32
socketpair(), but since not all tests use this function I have now enabled the tests that don’t.
80+ const SOCKET s = CreateSocket();
81+ Sock* sock = new Sock(s);
82+ BOOST_CHECK_EQUAL(sock->Release(), s);
83+ delete sock;
84+ BOOST_CHECK(!SocketIsClosed(s));
85+ BOOST_REQUIRE_EQUAL(close(s), 0);
CloseSocket here?
CloseSocket().
136+ CreateSocketPair(s);
137+
138+ Sock sock0(s[0]);
139+ Sock sock1(s[1]);
140+
141+ std::thread waiter([&sock0]() { sock0.Wait(24h, Sock::RECV); });
24h means 24 hours? If that’s the case it looks too much to me, even for a UT.
Yes, 24 hours. I wanted to use 1y (1 year), but y is not supported.
If this test detects a failure, it would result in a “test timeout in CI” which waits for 2 hours.
I don’t know how else to check if Wait() works as expected. Notice - it does not signal to the caller whether a timeout occurred or the socket is ready for the event. It was possible before, but we simplified it because nobody of the callers used that information. I think it is not worth restoring it for the sake of this test, what’s your take?
If that functionality is provided then the test could do: “wait 1ms; ensure it caused timeout, send some data to the socket, wait 1ms; ensure it did not timeout, but signaled that the socket is ready”.
I think it is not worth restoring it for the sake of this test, what’s your take?
I agree with you. I don’t think it is a good idea to have code the sake of one test only.
29+}
30+
31+static SOCKET CreateSocket()
32+{
33+ const int s = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
34+ BOOST_REQUIRE(s != -1);
SOCKET_ERROR here.
237@@ -238,6 +238,7 @@ BITCOIN_CORE_H = \
238 util/rbf.h \
239 util/ref.h \
240 util/settings.h \
241+ util/sock.h \
src/compat, src/support or src/util, not in the source root.
sock.h is in src/util, so that’s already the case, no?
1397+ int ret = close(socket);
1398+#endif
1399+ if (ret) {
1400+ LogPrintf("Socket close failed: %d. Error: %s\n", socket, NetworkErrorString(WSAGetLastError()));
1401+ }
1402+ socket = INVALID_SOCKET;
hSocket was renamed, making it more error-prone and difficult to verify that the code isn’t changed, suggest clarifying the commit message with the renaming or, better yet, making this change move-only
hSocket.
66+struct timeval MillisToTimeval(int64_t nTimeout);
67+
68+/**
69+ * Convert milliseconds to a struct timeval for e.g. select.
70+ */
71+struct timeval MillisToTimeval(const std::chrono::milliseconds& ms);
1dcf817 here and in the definition and in Sock::Wait()
0struct timeval MillisToTimeval(std::chrono::milliseconds ms);
5+#ifndef BITCOIN_UTIL_SOCK_H
6+#define BITCOIN_UTIL_SOCK_H
7+
8+#include <compat.h> // SOCKET
9+
10+#include <chrono> // std::chrono::milliseconds
1dcf817
0#include <chrono>
522@@ -541,58 +523,52 @@ static bool Socks5(const std::string& strDest, int port, const ProxyCredentials
523 uint8_t pchRet3[256];
524 switch (pchRet2[3])
525 {
526- case SOCKS5Atyp::IPV4: recvr = InterruptibleRecv(pchRet3, 4, SOCKS5_RECV_TIMEOUT, hSocket); break;
527- case SOCKS5Atyp::IPV6: recvr = InterruptibleRecv(pchRet3, 16, SOCKS5_RECV_TIMEOUT, hSocket); break;
528+ case SOCKS5Atyp::IPV4: recvr = InterruptibleRecv(pchRet3, 4, SOCKS5_RECV_TIMEOUT, sock); break;
529+ case SOCKS5Atyp::IPV6: recvr = InterruptibleRecv(pchRet3, 16, SOCKS5_RECV_TIMEOUT, sock); break;
I split this commit in two: variable type change from SOCKET to Sock + variable rename from hSocket to sock.
It is good to have the implied convention of naming SOCKET variables hSocket (old code) or socket (new code) and Sock variables sock.
487- CAddress addr_bind = GetBindAddress(hSocket);
488- CNode* pnode = new CNode(id, nLocalServices, hSocket, addrConnect, CalculateKeyedNetGroup(addrConnect), nonce, addr_bind, pszDest ? pszDest : "", conn_type);
489+ CAddress addr_bind = GetBindAddress(sock->Get());
490+ CNode* pnode = new CNode(id, nLocalServices, sock->Release(), addrConnect,
491+ CalculateKeyedNetGroup(addrConnect), nonce, addr_bind,
492+ pszDest ? pszDest : "", conn_type);
It seems more logical to me to move the CloseSocket and NetworkErrorString functions to sock.h, not system.h? Also do we still need an explicit (oh those are separate types, I see)CloseSocket instead of Sock::Reset?
Cursory code review ACK otherwise.
Move `MillisToTimeval()` from `netbase.{h,cpp}` to
`src/util/system.{h,cpp}`.
This is necessary in order to use `MillisToTimeval()` from a newly
introduced `src/util/sock.{h,cpp}` which cannot depend on netbase
because netbase will depend on it.bb5231615…7b6e4ee30: move CloseSocket() and NetworkErrorString() to util/sock.{h,cpp} instead of to util/system.{h,cpp}.
@laanwj,
It seems more logical to me to move the
CloseSocketandNetworkErrorStringfunctions tosock.h, notsystem.h?
Done.
Also do we still need an explicitCloseSocketinstead ofSock::Reset?
Actually this question makes sense - we can remove CloseSocket() altogether once its callers have switched to using the newly introduced Sock from this PR. There are about 10 places where CloseSocket() is called - should be possible (not in this PR).
Move `CloseSocket()` (and `NetworkErrorString()` which it uses) from
`netbase.{h,cpp}` to newly added `src/util/sock.{h,cpp}`.
This is necessary in order to use `CloseSocket()` from a newly
introduced Sock class (which will live in `src/util/sock.{h,cpp}`).
`sock.{h,cpp}` cannot depend on netbase because netbase will depend
on it.
Introduce a class to manage the lifetime of a socket - when the object
that contains the socket goes out of scope, the underlying socket will
be closed.
In addition, the new `Sock` class has a `Send()`, `Recv()` and `Wait()`
methods that can be overridden by unit tests to mock the socket
operations.
The `Wait()` method also hides the
`#ifdef USE_POLL poll() #else select() #endif` technique from higher
level code.
Use the `Sock` class instead of `SOCKET` for `InterruptibleRecv()` and
`Socks5()`.
This way the `Socks5()` function can be tested by giving it a mocked
instance of a socket.
Co-authored-by: practicalswift <practicalswift@users.noreply.github.com>
In the arguments of `InterruptibleRecv()`, `Socks5()` and
`ConnectThroughProxy()` the variable `hSocket` was previously of type
`SOCKET`, but has been changed to `Sock`. Thus rename it to `sock` to
imply its type, to distinguish from other `SOCKET` variables and to
abide to the coding style wrt variables' names.
@vasild Thanks! Yes, agree with not getting rid of CloseSocket in this PR, but glad to hear there’s a path to removing the loose function.
Code review and light testing (outbound connections over Tor still work) ACK e8e372a43122366fab7d6c7a45e05d4ad18d6da7
17+
18+static bool SocketIsClosed(const SOCKET& s)
19+{
20+ // Notice that if another thread is running and creates its own socket after `s` has been
21+ // closed, it may get assigned the same file descriptor number. In this case our test will
22+ // wronly pretend that the socket is not closed.
0 // wrongly pretend that the socket is not closed.
or “incorrectly”
16+BOOST_FIXTURE_TEST_SUITE(sock_tests, BasicTestingSetup)
17+
18+static bool SocketIsClosed(const SOCKET& s)
19+{
20+ // Notice that if another thread is running and creates its own socket after `s` has been
21+ // closed, it may get assigned the same file descriptor number. In this case our test will
0 // closed, it may be assigned the same file descriptor number. In this case, our test will
ACK e8e372a43122366fab7d6c7a45e05d4ad18d6da7 rebased to master, debug build + code review at each commit, at head ran the new sock tests and the other unit tests locally and ran bitcoind while observing peer behavior–operation appears to be nominal
Thanks for the recent updates that I found made review much easier. Below a couple of doc nits in the unit test that can be ignored or done in the follow-up if you’re inclined.
(Review and comment made from gh pr as an experiment.)
Edit: learned to never write gh pr in a comment from github cli, as it converts it to the general help.
@vasild Thanks for making our socket handling mockable. This will be extremely useful when fuzzing!
Similar to what you’ve done for TCP connections it would be really useful to be able to globally mock out DNS lookup to be able to have the responses be generated by a FuzzedDataProvider driven class instead of the ordinary lookup facility provided by the OS.
If you want to address this mocking need too you can find some inspiration in #19415 :)
Just for reference: previous incarnations of this PR had the Sock::Wait() method report to the caller whether one of the requested events happened or a timeout occurred. This was removed because none of the callers needed it.
However this is needed in #20685, so that PR contains a tiny commit to re-introduce that functionality.