net: Split socket create/connect #11363

theuni · 2017-09-19T02:24:58Z

Requirement for #11227.

We'll need to create sockets and perform the actual connect in separate steps, so break them up.

#11227 adds an RAII wrapper around connection attempts, as a belt-and-suspenders in case a CloseSocket is missed.

promag

I'm not yet familiar with P2P code to ACK. In any case see my comment below.

promag · 2017-09-20T00:19:30Z

src/netbase.cpp

@@ -554,8 +567,6 @@ bool ConnectThroughProxy(const proxyType &proxy, const std::string& strDest, int
        if (!Socks5(strDest, (unsigned short)port, 0, hSocket))
            return false;
    }
-
-    hSocketRet = hSocket;


If socks negotiation fail then hSocket will not be INVALID_SOCKET which is a different behavior than before. This also indicates that there is a leak in master because hSocket is not freed?

Nice Catch! Force-pushed the missing CloseSocket()s.

diff --git a/src/netbase.cpp b/src/netbase.cpp index 18763d2..4b72826 100644 --- a/src/netbase.cpp +++ b/src/netbase.cpp @@ -561,11 +561,15 @@ bool ConnectThroughProxy(const proxyType &proxy, const std::string& strDest, int ProxyCredentials random_auth; static std::atomic_int counter(0); random_auth.username = random_auth.password = strprintf("%i", counter++); - if (!Socks5(strDest, (unsigned short)port, &random_auth, hSocket)) + if (!Socks5(strDest, (unsigned short)port, &random_auth, hSocket)) { + CloseSocket(hSocket); return false; + } } else { - if (!Socks5(strDest, (unsigned short)port, 0, hSocket)) + if (!Socks5(strDest, (unsigned short)port, 0, hSocket)) { + CloseSocket(hSocket); return false; + } } return true; }

CloseSocket sets hSocket to INVALID_SOCKET so this LGTM. I wonder if you could just return CloseSocket(hSocket)?

CloseSocket's return value indicates whether the close succeeded or not, which would be true in this case. Not what we want :)

Ahhh yeah... now I win the most stupid comment award.

Hah, no worries. We'll call it even for pointing out my mistake here

theuni · 2017-09-28T18:12:31Z

Rebased after #10663.

Also, changed from the last version: Rather than passing in a socket reference and returning a bool, CreateSocket() now returns a socket (which may be invalid) instead. That's much more straightforward, IMO, and matches the usage of socket().

TheBlueMatt · 2017-09-28T23:48:28Z

src/netbase.cpp

@@ -452,10 +452,8 @@ static bool Socks5(const std::string& strDest, int port, const ProxyCredentials
    return true;
 }

-bool ConnectSocketDirectly(const CService &addrConnect, SOCKET& hSocketRet, int nTimeout)
+SOCKET CreateSocket(const CService &addrConnect)


You need to change the return values values of all the "return false"s in here - INVALID_SOCKET is (currently) ~0, so we will pretend the socket creation succeeded.

Thanks, don't know how I missed those.

TheBlueMatt · 2017-09-28T23:50:23Z

src/netbase.cpp

+    }
+    if (!addrConnect.GetSockAddr((struct sockaddr*)&sockaddr, &len)) {
+        LogPrintf("Cannot connect to %s: unsupported network\n", addrConnect.ToString());
+        CloseSocket(hSocket);


At the risk of making you redo a bunch of work, is it possible to const-ify the socket here and make the caller do the CloseSocket instead? Shits currently a maze and it'd be nice to clean it up sooner rather than try to follow it through the libevent changes.

Yes, that makes sense. Now that we don't have chained connect functions, it's much more clear what "the caller" means. The maze was only there because we had to close at all levels to be safe.

Will do.

theuni · 2017-11-07T21:22:57Z

Rebased and fixed the check against the connect() return value, which was already wrong, but doesn't really matter because it's always detected on first use anyway.

TheBlueMatt · 2017-11-07T21:23:23Z

utACK f766175ebe4400561f60b75b0ede4015003bad8c

laanwj · 2017-12-12T10:46:02Z

This gives me a new warning:

/.../bitcoin/src/netbase.cpp:449:17: warning: comparison of integers of different signs: 'SOCKET' (aka 'unsigned int') and 'int' [-Wsign-compare]
    if (hSocket == -1)
        ~~~~~~~ ^  ~~
1 warning generated.

laanwj · 2017-12-12T11:37:08Z

src/net.cpp

 #ifndef WIN32
-#ifdef SO_NOSIGPIPE
-    // Different way of disabling SIGPIPE on BSD
-    setsockopt(hListenSocket, SOL_SOCKET, SO_NOSIGPIPE, (void*)&nOne, sizeof(int));


NOSIGPIPE handling moved to CreateSocket(addrBind)?

Yes, I think part of the goal is to get random socket flag-fiddling things out of net and into netbase, its kinda split between both right now. @theuni ?

Yep. If it's something we require for all sockets, imo it makes sense to do it in CreateSocket.

laanwj · 2017-12-12T11:38:09Z

src/net.cpp

 #else
    setsockopt(hListenSocket, SOL_SOCKET, SO_REUSEADDR, (const char*)&nOne, sizeof(int));
-    setsockopt(hListenSocket, IPPROTO_TCP, TCP_NODELAY, (const char*)&nOne, sizeof(int));


Where did this move? The only remaining reference to TCP_NODELAY I see is in SetSocketNoDelay, which is called on accepted sockets only.

Its also called in netbase.cpp in CreateSocket, so should be set for outbound and inbound connection sockets, though not the listen socket, but that makes sense.

It's set on the listen socket as well (which should pass it down to accepted sockets anyway), since BindListenPort now uses CreateSocket().

Also, check for the correct error during socket creation

We use select in ConnectSocketDirectly, so this check needs to happen before that. IsSelectableSocket will not be relevant after upcoming changes to remove select.

…nections This allows const references to be passed around, making it clear where the socket may and may not be invalidated.

theuni · 2017-12-12T20:34:18Z

@laanwj Yes, thanks. Due to our SOCKET wrappers, we need to compare against INVALID_SOCKET rather than SOCKET_ERROR. Pushed the squashed change as it was very simple (it's actually just reverting a change from this PR, it was already correct in master).

Diff from before: git diff f766175ebe44..3830b6e

diff --git a/src/netbase.cpp b/src/netbase.cpp
index ba2c988..74ea6b1 100644
--- a/src/netbase.cpp
+++ b/src/netbase.cpp
@@ -446,7 +446,7 @@ SOCKET CreateSocket(const CService &addrConnect)
     }
 
     SOCKET hSocket = socket(((struct sockaddr*)&sockaddr)->sa_family, SOCK_STREAM, IPPROTO_TCP);
-    if (hSocket == SOCKET_ERROR)
+    if (hSocket == INVALID_SOCKET)
         return INVALID_SOCKET;
 
     if (!IsSelectableSocket(hSocket)) {

laanwj · 2017-12-13T04:42:22Z

utACK 3830b6e

3830b6e net: use CreateSocket for binds (Cory Fields) df3bcf8 net: pass socket closing responsibility up to caller for outgoing connections (Cory Fields) 9e3b2f5 net: Move IsSelectableSocket check into socket creation (Cory Fields) 1729c29 net: split socket creation out of connection (Cory Fields) Pull request description: Requirement for #11227. We'll need to create sockets and perform the actual connect in separate steps, so break them up. #11227 adds an RAII wrapper around connection attempts, as a belt-and-suspenders in case a CloseSocket is missed. Tree-SHA512: de675bb718cc56d68893c303b8057ca062c7431eaa17ae7c4829caed119fa3f15b404d8f52aca22a6bca6e73a26fb79e898b335d090ab015bf6456cf417fc694

3830b6e net: use CreateSocket for binds (Cory Fields) df3bcf8 net: pass socket closing responsibility up to caller for outgoing connections (Cory Fields) 9e3b2f5 net: Move IsSelectableSocket check into socket creation (Cory Fields) 1729c29 net: split socket creation out of connection (Cory Fields) Pull request description: Requirement for bitcoin#11227. We'll need to create sockets and perform the actual connect in separate steps, so break them up. bitcoin#11227 adds an RAII wrapper around connection attempts, as a belt-and-suspenders in case a CloseSocket is missed. Tree-SHA512: de675bb718cc56d68893c303b8057ca062c7431eaa17ae7c4829caed119fa3f15b404d8f52aca22a6bca6e73a26fb79e898b335d090ab015bf6456cf417fc694

theuni changed the title ~~net: Split socket create connect~~ net: Split socket create/connect Sep 19, 2017

theuni mentioned this pull request Sep 19, 2017

WIP: switch to libevent for node socket handling #11227

Closed

2 tasks

jonasschnelli added the P2P label Sep 19, 2017

promag reviewed Sep 20, 2017

View reviewed changes

theuni force-pushed the split-socket-create-connect branch 2 times, most recently from d555b09 to ced8190 Compare September 20, 2017 01:50

theuni force-pushed the split-socket-create-connect branch from ced8190 to 29e0a16 Compare September 28, 2017 18:08

TheBlueMatt reviewed Sep 28, 2017

View reviewed changes

theuni force-pushed the split-socket-create-connect branch from 29e0a16 to 0606eb5 Compare October 2, 2017 21:13

theuni force-pushed the split-socket-create-connect branch 2 times, most recently from 17b2a71 to f766175 Compare November 7, 2017 21:20

laanwj reviewed Dec 12, 2017

View reviewed changes

theuni added 4 commits December 12, 2017 15:25

net: split socket creation out of connection

1729c29

Also, check for the correct error during socket creation

net: Move IsSelectableSocket check into socket creation

9e3b2f5

We use select in ConnectSocketDirectly, so this check needs to happen before that. IsSelectableSocket will not be relevant after upcoming changes to remove select.

net: pass socket closing responsibility up to caller for outgoing con…

df3bcf8

…nections This allows const references to be passed around, making it clear where the socket may and may not be invalidated.

net: use CreateSocket for binds

3830b6e

theuni force-pushed the split-socket-create-connect branch from f766175 to 3830b6e Compare December 12, 2017 20:31

laanwj merged commit 3830b6e into bitcoin:master Dec 13, 2017

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

net: Split socket create/connect #11363

net: Split socket create/connect #11363

theuni commented Sep 19, 2017 •

edited

promag left a comment

promag Sep 20, 2017

theuni Sep 20, 2017

promag Sep 20, 2017

theuni Sep 20, 2017 •

edited

promag Sep 21, 2017

theuni Sep 21, 2017

theuni commented Sep 28, 2017

TheBlueMatt Sep 28, 2017

theuni Sep 29, 2017

TheBlueMatt Sep 28, 2017

theuni Sep 29, 2017

theuni commented Nov 7, 2017

TheBlueMatt commented Nov 7, 2017

laanwj commented Dec 12, 2017

laanwj Dec 12, 2017

TheBlueMatt Dec 12, 2017

theuni Dec 12, 2017

laanwj Dec 12, 2017

TheBlueMatt Dec 12, 2017

theuni Dec 12, 2017

theuni commented Dec 12, 2017

laanwj commented Dec 13, 2017

net: Split socket create/connect #11363

net: Split socket create/connect #11363

Conversation

theuni commented Sep 19, 2017 • edited

promag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theuni Sep 20, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theuni commented Sep 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theuni commented Nov 7, 2017

TheBlueMatt commented Nov 7, 2017

laanwj commented Dec 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theuni commented Dec 12, 2017

laanwj commented Dec 13, 2017

theuni commented Sep 19, 2017 •

edited

theuni Sep 20, 2017 •

edited