Test whether created sockets are select()able #6412

pull sipa wants to merge 1 commits into bitcoin:master from sipa:nonselectsocket changing 3 files +29 −0

sipa commented at 10:38 PM on July 9, 2015: member

This provides a belt-and-suspends check against file descriptor overflowing, to fix #6411.
in src/net.cpp:None in 082ff21630 outdated
```
 384 | @@ -385,6 +385,12 @@ CNode* ConnectNode(CAddress addrConnect, const char *pszDest)
 385 |      if (pszDest ? ConnectSocketByName(addrConnect, hSocket, pszDest, Params().GetDefaultPort(), nConnectTimeout, &proxyConnectionFailed) :
 386 |                    ConnectSocket(addrConnect, hSocket, nConnectTimeout, &proxyConnectionFailed))
 387 |      {
 388 | +        if (!IsSelectableSocket(hSocket)) {
 389 | +            LogPrintf("Cannot create connection: non-selectable socket created (file descriptor limit exceeded?)\n");
```
luke-jr commented at 10:48 PM on July 9, 2015:

Suggest removing " (file descriptor limit exceeded?)" since this description is inaccurate and likely to confuse someone trying to debug it.

sipa commented at 10:49 PM on July 9, 2015:

"non-selectable socket created" is completely unhelpful for indicating the reason or helping debug it. So I prefer to explain it a bit at least. Do you have a better suggestion?

laanwj commented at 4:58 AM on July 10, 2015:

I'd say the file descriptor limit exceeded is the part of the message that matters most.

luke-jr commented at 5:03 AM on July 10, 2015:

" (socket cannot be included in fdset)" perhaps? The actual fd limit very much has not been exceeded in most situations you'd get this error...

laanwj commented at 5:27 AM on July 10, 2015:

Well OK @luke-jr is right in that this is not "the" file descriptor limit, just the select file descriptor limit. Let's just name the beast as it is: Cannot create connection: non-selectable socket created (>=FD_SETSIZE)
ajweiss commented at 11:26 PM on July 9, 2015: contributor

select() is also used in netbase, seems it would make sense to check there. Also, if connect() were to return an EINVAL for any reason, that could trigger this in netbase. Have you considered just printing the fd value? For language I'd go simple "invalid file descriptor %ud from netbase on connect".
laanwj commented at 4:59 AM on July 10, 2015: member

utACK

in src/compat.h:None in 082ff21630 outdated

  91 | @@ -92,4 +92,12 @@ typedef u_int SOCKET;
  92 |  size_t strnlen( const char *start, size_t max_len);
  93 |  #endif // HAVE_DECL_STRNLEN
  94 |  
  95 | +bool static inline IsSelectableSocket(SOCKET s) {
  96 | +#ifdef WIN32
  97 | +    return true;

luke-jr commented at 5:03 AM on July 10, 2015:

return s != INVALID_SOCKET; ?

laanwj commented at 5:32 AM on July 10, 2015:

Don' think it should double as "is this an error return value"

sipa force-pushed on Jul 10, 2015
sipa force-pushed on Jul 10, 2015
sipa commented at 2:29 PM on July 10, 2015: member

Updated. The test is now done also in netbase (causing simple failure, as netbase does not usually print error messages), and when creating a listen socket.
sipa force-pushed on Jul 10, 2015
sipa force-pushed on Jul 10, 2015
laanwj commented at 2:58 PM on July 10, 2015: member

Needs backport to 0.11 and 0.10 (doesn't seem to involved, I'm willing to do that)
sipa commented at 3:03 PM on July 10, 2015: member

See my branches nonselectsocket-0.11 and nonselectsocket-0.10.
Test whether created sockets are select()able d422f9b1fd

in src/net.cpp:None in 68c081938a outdated

1607 | @@ -1597,6 +1608,13 @@ bool BindListenPort(const CService &addrBind, string& strError, bool fWhiteliste
1608 |          LogPrintf("%s\n", strError);
1609 |          return false;
1610 |      }
1611 | +    if (!IsSelectableSocket(hListenSocket))
1612 | +    {
1613 | +        strError = strprintf("Error: Couldn't create a listenable socket for incoming connections");

laanwj commented at 4:00 PM on July 10, 2015:

strprintf doesn't work without arguments (and is redundant)

sipa force-pushed on Jul 10, 2015
sipa commented at 4:18 PM on July 10, 2015: member

@laanwj Done.
laanwj added the label Bug on Jul 11, 2015

laanwj commented at 11:54 AM on July 13, 2015: member

This brings up a warning, as SOCKET is defined as unsigned int:

compat.h: In function ‘bool IsSelectableSocket(SOCKET)’:
compat.h:99:18: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]
     return (s >= 0 && s < FD_SETSIZE);

laanwj commented at 8:01 PM on July 17, 2015: member

@sipa are you planning to fix the warning or should we not hold this up on that?
laanwj merged this on Jul 20, 2015
laanwj closed this on Jul 20, 2015
laanwj referenced this in commit 1a2de3237f on Jul 20, 2015
laanwj referenced this in commit 89289d875d on Jul 20, 2015
laanwj referenced this in commit e8b87c8f78 on Jul 20, 2015
laanwj commented at 3:24 PM on July 20, 2015: member

Backported to 0.11 as e8b87c8f78fd66ebeb10ce5dfcf72b0a5e73f120 Warning fixed in 89289d875da108c42ca013f33597eda46cb6eb53 (e8b87c8f78fd66ebeb10ce5dfcf72b0a5e73f120 in 0.11)

To 0.10 as 0739e6e57a4b59966588a79ac2646eb3264c4dfa ae52a7ffd1b3743e76220812c7f581c5c08f975b
sipa referenced this in commit 0739e6e57a on Jul 20, 2015
laanwj referenced this in commit ae52a7ffd1 on Jul 20, 2015
dexX7 referenced this in commit 904cb2bd37 on Aug 18, 2015
reddink referenced this in commit 72d3187d62 on May 27, 2020
reddink referenced this in commit 083d86609e on May 27, 2020
DrahtBot locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#6411 connection limit calculation for select() is overly ambitious with -txindex