refactor: make bind() and listen() mockable/testable

vasild commented at 10:33 am on February 18, 2022: member

This is a piece of #21878, chopped off to ease review.

Add new methods Sock::Bind() and Sock::Listen() that wrap bind() and listen(). This will help to increase Sock usage and make more code mockable.

DrahtBot added the label P2P on Feb 18, 2022

DrahtBot added the label Refactoring on Feb 18, 2022

DrahtBot added the label Utils/log/libs on Feb 18, 2022

theStack commented at 5:43 pm on February 27, 2022: member

Concept ACK

I’m not too familiar on fuzzing and setting errno. May I ask what is the rationale for choosing the errno arrays in the fuzz tests? According to the linux manpages, there are some more possible errno vaues that bind and listen could set: https://man7.org/linux/man-pages/man2/bind.2.html (EACCES, EADDRINUSE, EBADF, EINVAL, ENOTSOCK) https://man7.org/linux/man-pages/man2/listen.2.html (EADDRINUSE, EBADF, ENOTSOCK, EOPNOTSUPP)

Note that there’s a typo in the PR title: s/mockatble/mockable/

vasild renamed this:
~~refactor: make bind() and listen() mockatble/testable~~
refactor: make bind() and listen() mockable/testable
on Mar 8, 2022

vasild commented at 8:13 am on March 8, 2022: member

what is the rationale for choosing the errno arrays in the fuzz tests?

Not much, other than the first one must be a permanent error in order to avoid infinite loops in case the app code decides to retry on EAGAIN (for example) and the syscall keeps returning EAGAIN forever:

https://github.com/bitcoin/bitcoin/blob/c9ed9927bbb7c422c4e01c0c1adc9722b8671009/src%2Ftest%2Ffuzz%2Futil.cpp#L78-L80

I think it is good to have both temporary errors (e.g. EAGAIN or EINTR) and permanent ones because that may trigger different code paths:

https://github.com/bitcoin/bitcoin/blob/c9ed9927bbb7c422c4e01c0c1adc9722b8671009/src%2Futil%2Fsock.cpp#L28

https://github.com/bitcoin/bitcoin/blob/c9ed9927bbb7c422c4e01c0c1adc9722b8671009/src%2Fnet.cpp#L1677

I added the comments and EADDRINUSE to Listen(), thanks!

vasild force-pushed on Mar 8, 2022

vasild commented at 8:14 am on March 8, 2022: member

5249fc7b7f...c2f80ba98d: add comments about errno arrays in fuzz-mocked bind() and listen() syscalls and also return EADDRINUSE from Listen().

DrahtBot commented at 6:29 pm on March 9, 2022: 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:

#21878 (Make all networking code mockable by vasild)

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.

pk-b2 commented at 4:35 pm on May 9, 2022: none

ACK c2f80ba98d2b4cd8b1ded6f507cd457de05a7884

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

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

3ad7de225e

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

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

b2733ab6a8

in src/test/fuzz/util.cpp:167 in c2f80ba98d outdated

159@@ -160,6 +160,41 @@ int FuzzedSock::Connect(const sockaddr*, socklen_t) const
160     return 0;
161 }
162 
163+int FuzzedSock::Bind(const sockaddr*, socklen_t) const
164+{
165+    // Have a permanent error at bind_errnos[0] because when the fuzzed data is exhausted
166+    // SetFuzzedErrNo() will always return the first element and we want to avoid Bind()
167+    // returning -1 and setting errno to EAGAIN repeatedly.

pk-b2 commented at 4:38 pm on May 9, 2022:

This comment threw me off a little. Is this clearer to what is actually happening inside SetFuzzedEerrNo?

0    // Have a permanent error at bind_errnos[0] because when the fuzzed data is exhausted
1    // SetFuzzedErrNo() will always set the first element. We want to avoid Bind()
2    // returning -1 and setting errno to EAGAIN repeatedly.

vasild commented at 6:49 am on May 10, 2022:

Hmm, I see where the confusion comes from - SetFuzzedErrNo() does not return the value but rather sets the global variable errno to it.

What about this:

0    // Have a permanent error at bind_errnos[0] because when the fuzzed data is exhausted
1    // SetFuzzedErrNo() will always set the global errno to bind_errnos[0]. We want to avoid Bind()
2    // returning -1 and setting errno to EAGAIN repeatedly because a legit code should retry on
3    // temporary errors leading to an infinite loop.

pk-b2 commented at 3:01 pm on May 10, 2022:

Thanks, that makes it clearer.

vasild commented at 2:41 pm on May 18, 2022:

Done, thanks!

vasild force-pushed on May 18, 2022

vasild commented at 2:41 pm on May 18, 2022: member

c2f80ba98d...b2733ab6a8: improve wording in comments

pk-b2 commented at 2:43 pm on May 18, 2022: none

ACK b2733ab6a85b234a88b83bdc77a0d043e18385b3

in src/net.cpp:2411 in b2733ab6a8

2407@@ -2409,7 +2408,7 @@ bool CConnman::BindListenPort(const CService& addrBind, bilingual_str& strError,
2408     LogPrintf("Bound to %s\n", addrBind.ToString());
2409 
2410     // Listen for incoming connections
2411-    if (listen(sock->Get(), SOMAXCONN) == SOCKET_ERROR)
2412+    if (sock->Listen(SOMAXCONN) == SOCKET_ERROR)

jonatack commented at 10:01 pm on May 23, 2022:

if you retouch, maybe fix up the bracket here like you did above

jonatack commented at 10:13 pm on May 23, 2022: member

Approach ACK, light ACK

Should #include <sys/socket.h> headers be added? (socklen_t, etc.)

vasild commented at 2:02 pm on May 24, 2022: member

Should #include <sys/socket.h> headers be added? (socklen_t, etc.)

You mean in sock.h? I think “no” because including sys/socket.h involves some #ifdef WIN32 magic and is only done in compat.h which is included in sock.h.

laanwj commented at 12:28 pm on June 22, 2022: member

Code review ACK b2733ab6a85b234a88b83bdc77a0d043e18385b3

laanwj merged this on Jun 28, 2022

laanwj closed this on Jun 28, 2022

vasild deleted the branch on Jun 28, 2022

sidhujag referenced this in commit 968d887367 on Jun 28, 2022

DrahtBot locked this on Jun 28, 2023

refactor: make bind() and listen() mockable/testable #24378

Conflicts