Skip to content
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.

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

Don't check if the listening socket is valid #23601

Merged

Conversation

vasild
Copy link
Contributor

@vasild vasild commented Nov 26, 2021

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

Listening sockets in CConnman::vhListenSocket are always valid
(underlying file descriptor is not INVALID_SOCKET).

Listening sockets in `CConnman::vhListenSocket` are always valid
(underlying file descriptor is not `INVALID_SOCKET`).
@vasild vasild mentioned this pull request Nov 26, 2021
12 tasks
@fanquake fanquake added the P2P label Nov 26, 2021
@vasild vasild changed the title net: don't check if the listening socket is valid Don't check if the listening socket is valid Nov 26, 2021
Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

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

Code-review ACK 6c9ee92 🔌

Verified that it's currently not possible that CConnman::vhListenSocket ever contains invalid sockets. The only place where an element gets added to the vector is in the method CConnman::BindListenPort:

vhListenSocket.push_back(ListenSocket(sock->Release(), permissions));

which would already return earlier (directly after CreateSock(...)) if the socket contained in the sock wrapper was not valid. Also built and ran unit tests locally with success.

@DrahtBot
Copy link
Contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #21879 (Wrap accept() and extend usage of Sock by vasild)
  • #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.

Copy link
Member

@luke-jr luke-jr left a comment

Choose a reason for hiding this comment

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

utACK.

If INVALID_SOCKET can get into recv_set, we have UB earlier anyway. Therefore, .count should always be 0 even if somehow listen_socket.socket were INVALID_SOCKET.

@maflcko maflcko merged commit f2074ee into bitcoin:master Dec 1, 2021
@maflcko
Copy link
Member

maflcko commented Dec 1, 2021

If this is safe, is there a reason why it can't be removed here as well?

src/net.cpp=void CConnman::StopNodes()
src/net.cpp-{
...
src/net.cpp-
src/net.cpp-    // Close listening sockets.
src/net.cpp:    for (ListenSocket& hListenSocket : vhListenSocket) {
src/net.cpp-        if (hListenSocket.socket != INVALID_SOCKET) {

@vasild vasild deleted the dont_check_if_listening_socket_is_valid branch December 1, 2021 09:51
@vasild
Copy link
Contributor Author

vasild commented Dec 1, 2021

@MarcoFalke, that's right - can be removed from here too and should have been part of this PR, I missed it.

I missed it because in #21878 that "close listening sockets" code becomes unnecessary and is removed:

-    // Close listening sockets.
-    for (ListenSocket& hListenSocket : vhListenSocket) {
-        if (hListenSocket.socket != INVALID_SOCKET) {
-            if (!CloseSocket(hListenSocket.socket)) {
-                LogPrintf("CloseSocket(hListenSocket) failed with error %s\n", NetworkErrorString(WSAG
-            }
-        }
-    }

This removal is also in #21879 which is chopped off from the big #21878 to ease review.

I will leave it as is to avoid further conflicts, hoping that #21879 will eventually make it in.

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 1, 2021
6c9ee92 net: don't check if the listening socket is valid (Vasil Dimov)

Pull request description:

  _This is a piece of bitcoin#21878, chopped off to ease review._

  Listening sockets in `CConnman::vhListenSocket` are always valid
  (underlying file descriptor is not `INVALID_SOCKET`).

ACKs for top commit:
  theStack:
    Code-review ACK 6c9ee92 🔌

Tree-SHA512: b2e29711c6a0c7c85467ca61cfd7fb734eb06bd83a41f88735901caf90aec095ca80707ce5bb897d39c80fdec16819dbf5a84979c9b1ab3dc3fb8b08cebe7c61
RandyMcMillan pushed a commit to RandyMcMillan/mempool-tab that referenced this pull request Dec 23, 2021
…alid

86ec664 net: don't check if the listening socket is valid (Vasil Dimov)

Pull request description:

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

  Listening sockets in `CConnman::vhListenSocket` are always valid
  (underlying file descriptor is not `INVALID_SOCKET`).

ACKs for top commit:
  theStack:
    Code-review ACK 86ec664 🔌

Tree-SHA512: b2e29711c6a0c7c85467ca61cfd7fb734eb06bd83a41f88735901caf90aec095ca80707ce5bb897d39c80fdec16819dbf5a84979c9b1ab3dc3fb8b08cebe7c61
@bitcoin bitcoin locked and limited conversation to collaborators Dec 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants