Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Jun 28, 2022
Merged

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

merged 2 commits into from
Jun 28, 2022

Conversation

vasild
Copy link
Contributor

@vasild vasild commented Feb 18, 2022

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.

@vasild vasild mentioned this pull request Feb 18, 2022
Closed
12 tasks
theStack
theStack reviewed Feb 27, 2022
View reviewed changes
Copy link
Contributor

@theStack theStack left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 vasild changed the title refactor: make bind() and listen() mockatble/testable refactor: make bind() and listen() mockable/testable Mar 8, 2022
@vasild
Copy link
Contributor Author

vasild commented Mar 8, 2022

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:

bitcoin/src/test/fuzz/util.cpp

Lines 78 to 80 in c9ed992

// Have a permanent error at recv_errnos[0] because when the fuzzed data is exhausted
// SetFuzzedErrNo() will always return the first element and we want to avoid Recv()
// returning -1 and setting errno to EAGAIN repeatedly.

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:

bitcoin/src/util/sock.cpp

Line 28 in c9ed992

return err != WSAEAGAIN && err != WSAEINTR && err != WSAEWOULDBLOCK && err != WSAEINPROGRESS;

bitcoin/src/net.cpp

Line 1677 in c9ed992

if (nErr != WSAEWOULDBLOCK && nErr != WSAEMSGSIZE && nErr != WSAEINTR && nErr != WSAEINPROGRESS)

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

@vasild vasild force-pushed the mockable_bind_and_listen branch from 5249fc7 to c2f80ba Compare March 8, 2022 08:14
@vasild
Copy link
Contributor Author

vasild commented Mar 8, 2022

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

@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 9, 2022 •
edited
Loading

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.

@DrahtBot DrahtBot mentioned this pull request Apr 8, 2022
Closed
@pk-b2
Copy link

pk-b2 commented May 9, 2022

ACK c2f80ba

pk-b2
pk-b2 reviewed May 9, 2022
View reviewed changes
vasild added 2 commits May 18, 2022 16:40
@vasild
net: add new method Sock::Bind() that wraps bind()
3ad7de2 
This will help to increase `Sock` usage and make more code mockable.
@vasild
net: add new method Sock::Listen() that wraps listen()
b2733ab 
This will help to increase `Sock` usage and make more code mockable.
@vasild vasild force-pushed the mockable_bind_and_listen branch from c2f80ba to b2733ab Compare May 18, 2022 14:41
@vasild
Copy link
Contributor Author

vasild commented May 18, 2022

c2f80ba98d...b2733ab6a8: improve wording in comments

@pk-b2
Copy link

pk-b2 commented May 18, 2022

ACK b2733ab

jonatack
jonatack reviewed May 23, 2022
View reviewed changes
Copy link
Member

@jonatack jonatack left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approach ACK, light ACK

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

src/net.cpp
@@ -2409,7 +2408,7 @@ bool CConnman::BindListenPort(const CService& addrBind, bilingual_str& strError,
LogPrintf("Bound to %s\n", addrBind.ToString());

// Listen for incoming connections
if (listen(sock->Get(), SOMAXCONN) == SOCKET_ERROR)
if (sock->Listen(SOMAXCONN) == SOCKET_ERROR)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@vasild
Copy link
Contributor Author

vasild commented May 24, 2022

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
Copy link
Member

laanwj commented Jun 22, 2022

Code review ACK b2733ab

@laanwj laanwj merged commit 55c9e2d into bitcoin:master Jun 28, 2022
@vasild vasild deleted the mockable_bind_and_listen branch June 28, 2022 13:12
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 28, 2022
@laanwj
Merge bitcoin#24378: refactor: make bind() and listen() mockable/test…
968d887 
…able

b2733ab net: add new method Sock::Listen() that wraps listen() (Vasil Dimov)
3ad7de2 net: add new method Sock::Bind() that wraps bind() (Vasil Dimov)

Pull request description:

  _This is a piece of bitcoin#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.

ACKs for top commit:
  pk-b2:
    ACK b2733ab
  laanwj:
    Code review ACK b2733ab

Tree-SHA512: c6e737606703e2106fe60cc000cfbbae3a7f43deadb25f70531e2cac0457e0b0581440279d14c76c492eb85c12af4adde52c30baf74542c41597e419817488e8
@bitcoin bitcoin locked and limited conversation to collaborators Jun 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
3 more reviewers

@theStack theStack theStack left review comments

@jonatack jonatack jonatack left review comments

@pk-b2 pk-b2 pk-b2 left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
P2P Refactoring Utils/log/libs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@vasild @DrahtBot @pk-b2 @laanwj @theStack @jonatack