This is a piece of #21878, chopped off to ease review.
Introduce an accept(2)
wrapper Sock::Accept()
and extend the usage of Sock
in CConnman::ListenSocket
and CreateNodeFromAcceptedSocket()
.
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.
fb10bf2ba7...0f83150f14
: rebase due to conflicts
9f69aa45a8...9f04449a8e
: apply upstream changes.
116+ memset(addr, 0x00, *addr_len);
117+ if (*addr_len >= sizeof(sockaddr_in)) {
118+ *addr_len = sizeof(sockaddr_in);
119+ sockaddr_in* addr_in = reinterpret_cast<sockaddr_in*>(addr);
120+ addr_in->sin_family = AF_INET;
121+ memset(&addr_in->sin_addr, 0x05, sizeof(addr_in->sin_addr));
inet_pton(AF_INET, "5.5.5.5", &addr_in->sin_addr);
be clearer?
inet_...()
function ended up using syscalls (it created sockets internally, IIRC) on Linux. @practicalswift, what do you think?
117+ if (*addr_len >= sizeof(sockaddr_in)) {
118+ *addr_len = sizeof(sockaddr_in);
119+ sockaddr_in* addr_in = reinterpret_cast<sockaddr_in*>(addr);
120+ addr_in->sin_family = AF_INET;
121+ memset(&addr_in->sin_addr, 0x05, sizeof(addr_in->sin_addr));
122+ addr_in->sin_port = htons(6789);
5.5.5.5
.
crACK tACK 9f04449a8ec7dc167a0d6d0cb47c5facd205a81a
Thank you for helping decouple our tests from system calls, @vasild. I also ran tests, and manually tested incoming connections on regtest using:
0# Start node0 with -listen
1$ src/bitcoind -regtest -datadir=`pwd`/node0 -rpccookiefile=.cookie -port=2222 -rpcport=3333 -listen
2
3# Generate a block on node0
4$ src/bitcoin-cli -regtest -datadir=`pwd`/node0 -rpccookiefile=.cookie -rpcport=3333 createwallet test
5$ src/bitcoin-cli -regtest -datadir=`pwd`/node0 -rpccookiefile=.cookie -rpcport=3333 -generate
6
7# node0 has a block
8$ src/bitcoin-cli -regtest -datadir=`pwd`/node0 -rpccookiefile=.cookie -rpcport=3333 getblockcount
91
10
11$ src/bitcoin-cli -regtest -datadir=`pwd`/node0 -rpccookiefile=.cookie -rpcport=3333 getbestblockhash
121878296b74ee617ff119d2fc3f85c9fb35eae952e0360e505bb7d325dfe9e825
13
14# Start node1
15$ src/bitcoind -regtest -datadir=`pwd`/node1 -rpccookiefile=.cookie -port=2223 -rpcport=3334
16
17# node1 has no blocks
18$ src/bitcoin-cli -regtest -datadir=`pwd`/node1 -rpccookiefile=.cookie -rpcport=3334 getblockcount
190
20
21# Restart node1 with -addnode=node0
22$ src/bitcoind -regtest -datadir=`pwd`/node1 -rpccookiefile=.cookie -port=2223 -rpcport=3334 -addnode=127.0.0.1:2222
23
24# node1 completes IBD and has the block
25$ src/bitcoin-cli -regtest -datadir=`pwd`/node1 -rpccookiefile=.cookie -rpcport=3334 getblockcount
261
27
28$ src/bitcoin-cli -regtest -datadir=`pwd`/node1 -rpccookiefile=.cookie -rpcport=3334 getbestblockhash
291878296b74ee617ff119d2fc3f85c9fb35eae952e0360e505bb7d325dfe9e825
Would the goal eventually be to not have to invoke Sock::Release()
anywhere and instead use move semantics across the board as more code is RAII-fied?
Very nice work.
Non-blocking suggestion: Would it be possible to add a fuzzing harness utilizing FuzzedSock::Accept
as part of this PR? It would be really nice to see this in action, and it would also transition FuzzedSock::Accept
out of “unused code” territory :)
Would the goal eventually be to not have to invoke
Sock::Release()
anywhere and instead use move semantics across the board as more code is RAII-fied?
Yes! #21878 removes Sock::Release()
in a later commit, called… eh… net: remove now unused Sock::Release()
:smile:. It uses either move semantics or passes the Sock
objects by reference if a function wants to use the socket but not claim ownership of it.
Would it be possible to add a fuzzing harness utilizing
FuzzedSock::Accept
as part of this PR? …
The Accept()
method is only called from CConnman::AcceptConnection()
which itself calls a bunch of other functions that are not mocked yet in this PR. All those are dealt with in #21878 which also adds fuzzing of CConnman::SocketHandler()
(which calls CConnman::AcceptConnection()
) in commit fuzz: add CConnman::SocketHandler() to the tests
.
ACK 9f04449a8ec7dc167a0d6d0cb47c5facd205a81a (jamesob/ackr/21879.1.vasild.wrap_accept_and_extend_u
)
Change looks good to me and, as others have noted, making network code more mockable will make writing more comprehensive tests feasible. Cloned locally and ran unit, functional tests.
When I made the dumb mutation below, both test suites still passed so I guess we don’t have any tests exercising socket errors.
0diff --git a/src/util/sock.cpp b/src/util/sock.cpp
1index 9a92fd1b5f..e25c1bab7f 100644
2--- a/src/util/sock.cpp
3+++ b/src/util/sock.cpp
4@@ -76,7 +76,7 @@ std::unique_ptr<Sock> Sock::Accept(sockaddr* addr, socklen_t* addr_len) const
5 #ifdef WIN32
6 static constexpr auto ERR = INVALID_SOCKET;
7 #else
8- static constexpr auto ERR = SOCKET_ERROR;
9+ static constexpr auto ERR = SOCKET_ERROR + 1;
10 #endif
11
12 const auto socket = accept(m_socket, addr, addr_len);
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA512
2
3ACK 9f04449a8ec7dc167a0d6d0cb47c5facd205a81a ([`jamesob/ackr/21879.1.vasild.wrap_accept_and_extend_u`](https://github.com/jamesob/bitcoin/tree/ackr/21879.1.vasild.wrap_accept_and_extend_u))
4
5Change looks good to me and, as others have noted, making network code more mockable will make writing comprehensive tests more feasible. Cloned locally and ran unit, functional tests.
6
7When I made the dumb mutation below, both test suites passed so I guess we don't have any tests exercising invalid socket errors.
8
9<patch>
10-----BEGIN PGP SIGNATURE-----
11
12iQIzBAEBCgAdFiEEGNRVI1NPYuZCSIrGepNdrbLETwUFAmEgIVsACgkQepNdrbLE
13TwWAvA/8C6OfGufmfaEdYzaaGVPyq1jmF7GmXsFo0T3AkflMBbMqri4Q+1ai1Yzs
14CkVOuxGBDj8wiKxEeRsbsCsp3gbcOcp21ccvn6EQkPi0Mk8Rusj1iXgs+sDPuf7V
15tKq3uQ54IbxkavKKWWhp5VeGrm4ddm8s2qF5uaN7UcGlGAPTw1915YW2HmEa3sC3
169hQb+VGw3nkczUBJXMZls0AUF6E8XYTX3QycSGjlpazgSJac5J54trfvdCzpR1Ae
170WlWndO/VROlePi6p2Mg0DHMCuuRyk78qCHx547WcaQxYWOic+XMEU1OELueLVbf
18gf16So4iITHJWeK/GOLuecRfGm58uquWtQuzl5twzvPmzHRrwYyHrvgQP9eANzkQ
19VCIbZWV7zL5ksgDGRJTwwzUERICvB6PqlzSU3a49f7kD2DnW+CMzHROe2YW+uLM7
20lIn6fmw7WCLBAi990aOtlTf9eT4YYrkneNQYz4vnS/uWvNFBqrkCFxhTdBDHg0jr
218EDzNVFtBFnxKBLYia+ybnPb8pqqAak2AYlrpYVSw6NWSlaWLq8d5JwqkNjgY2K1
22anpssdubMvHmWk+N5e0rFbRg1froCMB3Fn7BhbSRbNlnKecv1M+WE1UgI8Y4Hlcn
23nLxE/xMFHD94q9N79b76ENqDN8lOgTQmIbUVVcW+LKJpZ5bIQnU=
24=EpEl
25-----END PGP SIGNATURE-----
0Tested on Linux-4.19.0-17-amd64-x86_64-with-glibc2.28
1
2Configured with ./configure LDFLAGS=-L/home/james/src/bitcoin/db4/lib/ CPPFLAGS=-I/home/james/src/bitcoin/db4/include/ CXXFLAGS=-fPIE -pipe -O2 -g -Wthread-safety-analysis -Wall -Werror=sign-compare --enable-wallet --with-daemon CXX=/usr/local/bin/clang++ CC=/usr/local/bin/clang --with-bignum=no PYTHONPATH= --disable-shared --with-pic --enable-benchmark=no --enable-module-recovery --enable-module-schnorrsig --enable-experimental --disable-shared --with-pic --enable-benchmark=no --enable-module-recovery --enable-module-schnorrsig --enable-experimental --no-create --no-recursion
3
4Compiled with /usr/bin/ccache /usr/local/bin/clang++ -std=c++17 -mavx -mavx2 -fPIE -pipe -O2 -g -Wthread-safety-analysis -Wall -Werror=sign-compare -fdebug-prefix-map=$(abs_srcdir)=. -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection -msse4 -msha -msse4.1 -msse4.2 i
5
6Compiler version: Debian clang version 11.1.0-++20210428103820+1fdec59bffc1-1~exp1~20210428204437.162
@jamesob, thanks for looking into this!
Your mod changes ERR
from -1
to 0
and the code looks like this:
0 const auto socket = accept(m_socket, addr, addr_len);
1 if (socket == 0) {
2 return nullptr;
3 }
4 return std::make_unique<Sock>(socket);
This will change the behavior in two cases:
accept()
succeeds and returns socket=0. Could happen if stdin (fd=0) is closed and the descriptor number 0
is reused? In this case Sock::Accept()
would return nullptr
, signaling failure.accept()
fails and returns -1
, then the check -1 == 0
would be false and Sock::Accept()
would return a Sock
object with internal m_socket
equal to -1
.I think it is unlikely that any of that happens (the real syscall accept()
to return -1
or 0
).
accept()
-fails branches. I think this PR will make it feasible to add such coverage, if that ends up being worth doing.
9@@ -10,6 +10,7 @@
10 #include <util/time.h>
11
12 #include <chrono>
13+#include <memory>
Tested ACK 9f04449a8ec7dc167a0d6d0cb47c5facd205a81a rebased to master at a8a272ac32, code review, debug build and ran unit+functional tests at each commit, ran bitcoind on this branch head rebased to master on mainnet with a full range of connections including inbounds, behavior and debug log nominal
git range-diff a9d0cec 9f04449 78e21e
following my previous full review (https://github.com/bitcoin/bitcoin/pull/21879#pullrequestreview-761251278)
git range-diff efa227f 78e21e5 07d998f
git range-diff 2ef186a 07d998f 0f4474
, re-review, rebase/debug build/ran units following my previous full review (https://github.com/bitcoin/bitcoin/pull/21879#pullrequestreview-761251278)
This will help to increase `Sock` usage and make more code mockable.
Change `CConnman::ListenSocket` to use a pointer to `Sock` instead of a
bare `SOCKET` and use `Sock::Accept()` instead of bare `accept()`. This
will help mocking / testing / fuzzing more code.
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).
ACK 6bf6e9fd9dece67878595a5f3361851c25833c49 (jamesob/ackr/21879.2.vasild.wrap_accept_and_extend_u
)
Examined interdiff and built/tested locally with a pre-populated datadir.
Changes since last review include rebasing on master (CJDNS changes, vNodes -> m_nodes), some && sock function sig changes, write_len
rephrasing, and catching exceptions during unique_ptr construction in Sock::Accept.
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA512
2
3ACK 6bf6e9fd9dece67878595a5f3361851c25833c49 ([`jamesob/ackr/21879.2.vasild.wrap_accept_and_extend_u`](https://github.com/jamesob/bitcoin/tree/ackr/21879.2.vasild.wrap_accept_and_extend_u))
4
5Examined interdiff and built/tested locally with a pre-populated datadir.
6
7Changes since last review include rebasing on master (CJDNS changes, vNodes -> m_nodes), some
8&& sock function sig changes, `write_len` rephrasing, and catching exceptions during unique_ptr
9construction in Sock::Accept.
10-----BEGIN PGP SIGNATURE-----
11
12iQIzBAEBCgAdFiEEGNRVI1NPYuZCSIrGepNdrbLETwUFAmGn3PIACgkQepNdrbLE
13TwXzpw/+KLFtcxY3gB2hbfsjsm0IUUiY/FWnFL1S5ADu5HaFAMHHkT9cEfCsMhAn
14oQeGgtKJ4+kp3j01JEHwo7Sa/XLj1CEKWLbLd3OFJyFgQ7GJr0X9Ej0RmHN67Rjz
15B70DzBNe6FeRz/tST+fkVOULLJqj0bwg1FeVMsHK1bbAWCtl358O25flNeXy5zCI
16tY5IAevJII5RP1YjbeTddY5VRPXBNP/dohEL6M5mvZdNsz5/OHz7VfuCbJtH1LBC
17E7HcaMl8y70feJ4bVHuRmupx1+TYy5JUx1Aj05EcNzmkC/APHdTcL9bsZXiiOeHU
18nvp81Ozq7oRe9ZkEzTPiKScPP6rqHbFILUE+dYW22P6qjDczHdge2sPVzcyIWKMR
19CW7tG9PSo4dzRly+jh8T90ApnPe7oVQr6pzPVTjAlyuNHcS7CWYnEWS0Nha1zWyv
20TYV2jC/53ugrqVnUAm+HbfTR2SIhTpYgYpUkYArHmEyXL/PZOnYB02+vgEhTsMkQ
21JSYHb7AwlHoKr9R+S7XvkNG2TN2/2x4XZNTYr8sDvdNlxWLzsfTbTv1g9gfX/U1U
22VFvJNFUaY/i8qq0FUKYgHvz0JMvaoRnsTx5w1K4m9tvdiNtA34TJy+y1sP1VPnnr
237dA09N0rRu9BuAzioViAsd2eVZQS3TUOjvsYnKFL6EmsC/36lgg=
24=HB8m
25-----END PGP SIGNATURE-----
0Tested on Linux-5.10.0-9-amd64-x86_64-with-glibc2.28
1
2Configured with ./configure LDFLAGS=-L/home/james/src/bitcoin/db4/lib/ CPPFLAGS=-I/home/james/src/bitcoin/db4/include/ CXXFLAGS=-fPIE -pipe -O2 -g -Wthread-safety-analysis -Wall -Werror=sign-compare -Wsign-compare -Werror=thread-safety-analysis --enable-wallet --enable-debug --with-daemon --enable-natpmp-default CXX=/usr/bin/clang++ CC=/usr/bin/clang PYTHONPATH= --disable-openssl-tests --disable-shared --with-pic --enable-benchmark=no --enable-module-recovery --enable-module-schnorrsig --enable-experimental --disable-shared --with-pic --enable-benchmark=no --enable-module-recovery --enable-module-schnorrsig --enable-experimental --disable-openssl-tests --no-create --no-recursion
3
4Compiled with /usr/bin/ccache /usr/bin/clang++ -std=c++17 -mavx -mavx2 -mpclmul -fPIE -pipe -O2 -g -Wthread-safety-analysis -Wall -Werror=sign-compare -Wsign-compare -Werror=thread-safety-analysis -O0 -g3 -ftrapv -fdebug-prefix-map=$(abs_top_srcdir)=. -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection -msse4 -msha -msse4.1 -msse4.2 i
5
6Compiler version: Debian clang version 13.0.1-++20211110052940+162f3f18c945-1~exp1~20211110053502.25
git range-diff ea989de 976f6e8 6bf6e9f
– only change since my last review was s/listen_socket.socket/listen_socket.sock->Get()/
in src/net.cpp: CConnman::SocketHandlerListening()
– re-read the code changes, rebase/debug build/ran units following my previous full review (https://github.com/bitcoin/bitcoin/pull/21879#pullrequestreview-761251278)
tACK 6bf6e9f
Ran the node on signet and the unit / functional tests.
Seems good. Sock::Accept
encapsulates the accept()
system call, making the code more mockable and simpler due to the use of the RAII Sock
class.
If I understand correctly, instead of manually closing each socket from vhListenSocket
, Sock
destructor will automatically handle this after vhListenSocket.clear()
.
If I understand correctly, instead of manually closing each socket from
vhListenSocket
,Sock
destructor will automatically handle this aftervhListenSocket.clear()
.
Yes!