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

Test whether created sockets are select()able #6412

Merged
merged 1 commit into from Jul 20, 2015

Conversation

sipa
Copy link
Member

@sipa sipa commented Jul 9, 2015

This provides a belt-and-suspends check against file descriptor overflowing, to fix #6411.

@@ -385,6 +385,12 @@ CNode* ConnectNode(CAddress addrConnect, const char *pszDest)
if (pszDest ? ConnectSocketByName(addrConnect, hSocket, pszDest, Params().GetDefaultPort(), nConnectTimeout, &proxyConnectionFailed) :
ConnectSocket(addrConnect, hSocket, nConnectTimeout, &proxyConnectionFailed))
{
if (!IsSelectableSocket(hSocket)) {
LogPrintf("Cannot create connection: non-selectable socket created (file descriptor limit exceeded?)\n");
Copy link
Member

@luke-jr luke-jr Jul 9, 2015

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@sipa sipa Jul 9, 2015

Choose a reason for hiding this comment

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

"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?

Copy link
Member

@laanwj laanwj Jul 10, 2015

Choose a reason for hiding this comment

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

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

Copy link
Member

@luke-jr luke-jr Jul 10, 2015

Choose a reason for hiding this comment

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

" (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...

Copy link
Member

@laanwj laanwj Jul 10, 2015

Choose a reason for hiding this comment

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

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

ajweiss commented Jul 9, 2015

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

laanwj commented Jul 10, 2015

utACK

@@ -92,4 +92,12 @@ typedef u_int SOCKET;
size_t strnlen( const char *start, size_t max_len);
#endif // HAVE_DECL_STRNLEN

bool static inline IsSelectableSocket(SOCKET s) {
#ifdef WIN32
return true;
Copy link
Member

@luke-jr luke-jr Jul 10, 2015

Choose a reason for hiding this comment

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

return s != INVALID_SOCKET; ?

Copy link
Member

@laanwj laanwj Jul 10, 2015

Choose a reason for hiding this comment

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

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

@sipa sipa force-pushed the nonselectsocket branch 2 times, most recently from b40d7b6 to a29e66c Compare Jul 10, 2015
@sipa
Copy link
Member Author

sipa commented Jul 10, 2015

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 sipa force-pushed the nonselectsocket branch 2 times, most recently from 1cf0d45 to 68c0819 Compare Jul 10, 2015
@laanwj
Copy link
Member

laanwj commented Jul 10, 2015

Needs backport to 0.11 and 0.10 (doesn't seem to involved, I'm willing to do that)

@sipa
Copy link
Member Author

sipa commented Jul 10, 2015

See my branches nonselectsocket-0.11 and nonselectsocket-0.10.

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

@laanwj laanwj Jul 10, 2015

Choose a reason for hiding this comment

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

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

@sipa
Copy link
Member Author

sipa commented Jul 10, 2015

@laanwj Done.

@laanwj
Copy link
Member

laanwj commented Jul 13, 2015

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

laanwj commented Jul 17, 2015

@sipa are you planning to fix the warning or should we not hold this up on that?

@laanwj laanwj merged commit d422f9b into bitcoin:master Jul 20, 2015
laanwj added a commit that referenced this pull request Jul 20, 2015
d422f9b Test whether created sockets are select()able (Pieter Wuille)
laanwj added a commit that referenced this pull request Jul 20, 2015
SOCKET are defined as unsigned integers, thus always >=0.
laanwj added a commit that referenced this pull request Jul 20, 2015
SOCKET are defined as unsigned integers, thus always >=0.

Rebased-From: 89289d8
@laanwj
Copy link
Member

laanwj commented Jul 20, 2015

Backported to 0.11 as e8b87c8
Warning fixed in 89289d8 (e8b87c8 in 0.11)

To 0.10 as 0739e6e ae52a7f

sipa added a commit that referenced this pull request Jul 20, 2015
Conflicts:
	src/net.cpp

Github-Pull: #6412
Rebased-From: d422f9b
laanwj added a commit that referenced this pull request Jul 20, 2015
SOCKET are defined as unsigned integers, thus always >=0.

Rebased-From: 89289d8
dexX7 added a commit to OmniLayer/omnicore that referenced this pull request Aug 18, 2015
ae52a7f Fix warning introduced by bitcoin#6412 (Wladimir J. van der Laan)
0739e6e Test whether created sockets are select()able (Pieter Wuille)
255eced Updated URL location of netinstall for Debian (฿tcDrak)
7e66e9c openssl: avoid config file load/race (Cory Fields)
3f55638 doc: update mailing list address (Wladimir J. van der Laan)
be64204 Add option `-alerts` to opt out of alert system (Wladimir J. van der Laan)
0fd8464 Fix getbalance * (Tom Harding)
09334e0 configure: Detect (and reject) LibreSSL (Luke Dashjr)
181771b json: fail read_string if string contains trailing garbage (Wladimir J. van der Laan)
ecc96f5 Remove P2SH coinbase flag, no longer interesting (Luke Dashjr)
ebd7d8d Parameter interaction: disable upnp if -proxy set (Wladimir J. van der Laan)
ae3d8f3 Fix two problems in CSubNet parsing (Wladimir J. van der Laan)
e4a7d51 Simplify code for CSubnet (Wladimir J. van der Laan)

Conflicts:
	README.md
reddink pushed a commit to reddcoin-project/reddcoin that referenced this pull request May 27, 2020
Conflicts:
	src/net.cpp

Github-Pull: bitcoin#6412
Rebased-From: d422f9b
(cherry picked from commit 0739e6e)
reddink pushed a commit to reddcoin-project/reddcoin that referenced this pull request May 27, 2020
SOCKET are defined as unsigned integers, thus always >=0.

Rebased-From: 89289d8
(cherry picked from commit ae52a7f)
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

connection limit calculation for select() is overly ambitious with -txindex
4 participants