Fix a crash on startup when ulimits are set to unlimited.#1615
Fix a crash on startup when ulimits are set to unlimited.#1615cassandras-lies wants to merge 1 commit intofiroorg:masterfrom
Conversation
WalkthroughThe update changes the types of three internal initialization variables from Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (6)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
aleflm
left a comment
aleflm
left a comment
There was a problem hiding this comment.
Looks good to me.
Just a suggestion, how about using uint64_t instead of introducing rlim_t.
|
The return parameter is defined to be rlim_t, which is not necessarily uint64_t, it could be an int or something depending on the platform. I defined it as an unsigned long long on Windows in the #else directive. |
|
bugbot run |
|
@cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
|
|
||
| // Trim requested connection counts, to fit into system limitations | ||
| nMaxConnections = std::max(std::min(nMaxConnections, (int)(FD_SETSIZE - nBind - MIN_CORE_FILEDESCRIPTORS - MAX_ADDNODE_CONNECTIONS)), 0); | ||
| nMaxConnections = std::max(std::min(nMaxConnections, (rlim_t)(FD_SETSIZE - nBind - MIN_CORE_FILEDESCRIPTORS - MAX_ADDNODE_CONNECTIONS)), (rlim_t)0); |
There was a problem hiding this comment.
Format specifier mismatch for rlim_t type variables
Medium Severity
The variables nMaxConnections, nUserMaxConnections, and nFD were changed from int to rlim_t (defined as unsigned long long on Windows), but the strprintf call at line 1183 still uses %d format specifiers which expect int. This format mismatch causes undefined behavior and will print garbage or incorrect values in the warning message. There's also a similar issue at line 1462 using %i with these variables in LogPrintf.
Additional Locations (1)
reubenyap
left a comment
reubenyap
left a comment
There was a problem hiding this comment.
PR #1615 Review: Fix crash on startup when ulimits are set to unlimited
[1 file changed, +8 / -6] — Changes nMaxConnections, nUserMaxConnections, and nFD from int to rlim_t to fix a crash when getrlimit returns RLIM_INFINITY (interpreted as -1 when stored in int).
Static Analysis
cppcheck: No new issues in changed code (pre-existing warnings only). flawfinder: No new hits. semgrep (Firo rules): 38 pre-existing hits, none in changed lines.
Findings Summary
| # | Severity | File | Finding |
|---|---|---|---|
| 1 | HIGH | src/init.cpp:119 |
#define rlim_t is a dangerous macro redefining a POSIX type |
| 2 | HIGH | src/init.cpp:1075 |
rlim_t variables silently narrow when passed to int consumers |
| 3 | MEDIUM | src/init.cpp:1172 |
Signed GetArg return wraps to huge unsigned in rlim_t |
| 4 | LOW | src/init.cpp:121 |
System include buried inside preprocessor block |
Architectural Concern
The root cause of the original crash is in RaiseFileDescriptorLimit() (src/util.cpp:771) which returns rlim_t as int, truncating RLIM_INFINITY to -1. A more targeted fix would be to fix RaiseFileDescriptorLimit itself to clamp rlim_cur to INT_MAX before returning, keeping all the downstream variables as int. This avoids cascading type changes and the narrowing conversion issues identified above.
Each finding is posted as an inline comment on the relevant code — click to view, resolve when addressed. Each comment includes a Prompt for AI agents that can be copy-pasted into Cursor/Copilot/Claude Code to auto-fix the issue.
Review assisted by AI + static analysis (cppcheck, flawfinder, semgrep). Posted by @reubenyap.
| // accessing block files don't count towards the fd_set size limit | ||
| // anyway. | ||
| #define MIN_CORE_FILEDESCRIPTORS 0 | ||
| #define rlim_t unsigned long long |
There was a problem hiding this comment.
[HIGH] #define rlim_t unsigned long long is a dangerous macro that redefines a POSIX type
Using #define to create a type alias is fragile. Unlike typedef, a #define doesn't respect scope, can clash with any header that defines or uses rlim_t, and expands textually (e.g. (rlim_t)x becomes (unsigned long long)x — works by luck but is not a proper cast expression in all contexts). On non-Windows, <sys/resource.h> already provides rlim_t.
Suggested fix:
Use typedef instead:
#ifdef WIN32
#define MIN_CORE_FILEDESCRIPTORS 0
typedef unsigned long long rlim_t;
#else
#include <sys/resource.h>
#define MIN_CORE_FILEDESCRIPTORS 150
#endif🤖 Prompt for AI agents
"In file src/init.cpp, around line 119, #define rlim_t unsigned long long is used inside the #ifdef WIN32 block. Replace it with typedef unsigned long long rlim_t;. Keep the #define MIN_CORE_FILEDESCRIPTORS 0 on the line before it. Keep the #else branch with #include <sys/resource.h> unchanged."
| int nMaxConnections; | ||
| int nUserMaxConnections; | ||
| int nFD; | ||
| rlim_t nMaxConnections; |
There was a problem hiding this comment.
[HIGH] nMaxConnections is rlim_t but downstream consumers expect int
Changing these variables to rlim_t (unsigned long long) introduces implicit narrowing conversions at several downstream sites:
- Line 2079:
connOptions.nMaxConnections = nMaxConnections;—CConnman::Options::nMaxConnectionsisint(net.h:165) - Line 2080:
connOptions.nMaxOutbound = std::min(MAX_OUTBOUND_CONNECTIONS, connOptions.nMaxConnections); - Line 1177:
RaiseFileDescriptorLimit(...)takesintand returnsint(util.h:121) - Lines 1183/1462:
%d/%iformat specifiers used withrlim_tvalues — this is undefined behavior
The real fix should keep nMaxConnections/nFD as int but fix the narrowing at the getrlimit call site in RaiseFileDescriptorLimit(). Or if rlim_t is kept here, all downstream consumers must also be updated.
Suggested fix:
The safer approach: keep these as int, and fix RaiseFileDescriptorLimit to internally handle rlim_t from getrlimit and clamp to INT_MAX before returning. This avoids cascading type changes throughout the codebase.
🤖 Prompt for AI agents
"In file src/init.cpp, lines 1075-1077 change nMaxConnections, nUserMaxConnections, and nFD from int to rlim_t. This causes implicit narrowing conversions downstream: CConnman::Options::nMaxConnections is int (src/net.h:165), RaiseFileDescriptorLimit takes and returns int (src/util.h:121), and format specifiers %d/%i at lines 1183 and 1462 expect int not unsigned long long. Either revert these to int and fix the root cause in RaiseFileDescriptorLimit() in src/util.cpp (line 771-788) to internally use rlim_t from getrlimit but clamp the return value to INT_MAX, or update all downstream consumers to accept rlim_t. The former approach is safer and more localized."
| @@ -1168,10 +1170,10 @@ bool AppInitParameterInteraction() | |||
| (mapMultiArgs.count("-bind") ? mapMultiArgs.at("-bind").size() : 0) + | |||
| (mapMultiArgs.count("-whitebind") ? mapMultiArgs.at("-whitebind").size() : 0), size_t(1)); | |||
| nUserMaxConnections = GetArg("-maxconnections", DEFAULT_MAX_PEER_CONNECTIONS); | |||
There was a problem hiding this comment.
[MEDIUM] GetArg returns signed int64_t, stored in unsigned rlim_t — negative values wrap
GetArg("-maxconnections", DEFAULT_MAX_PEER_CONNECTIONS) returns int64_t. If a user passes -maxconnections=-1, the signed value -1 is stored into unsigned rlim_t and becomes ULLONG_MAX (~1.8×10¹⁹). The old code used int, so std::max(nUserMaxConnections, 0) on line 1173 would clamp it to 0. With rlim_t, std::max(nUserMaxConnections, (rlim_t)0) compares two unsigned values, and the wrapped value is enormous, so the clamp has no effect.
Suggested fix:
Clamp the signed value before converting to unsigned:
int64_t rawMaxConn = GetArg("-maxconnections", DEFAULT_MAX_PEER_CONNECTIONS);
nUserMaxConnections = (rawMaxConn < 0) ? 0 : static_cast<rlim_t>(rawMaxConn);🤖 Prompt for AI agents
"In file src/init.cpp, line 1172, nUserMaxConnections = GetArg(\"-maxconnections\", DEFAULT_MAX_PEER_CONNECTIONS); stores a signed int64_t return value in unsigned rlim_t, causing negative values to wrap to huge unsigned numbers. Add an intermediate signed variable and clamp: int64_t rawMaxConn = GetArg(\"-maxconnections\", DEFAULT_MAX_PEER_CONNECTIONS); nUserMaxConnections = (rawMaxConn < 0) ? 0 : static_cast<rlim_t>(rawMaxConn);. Remove the now-redundant std::max(nUserMaxConnections, (rlim_t)0) on line 1173."
| #define MIN_CORE_FILEDESCRIPTORS 0 | ||
| #define rlim_t unsigned long long | ||
| #else | ||
| #include <sys/resource.h> |
There was a problem hiding this comment.
[LOW] <sys/resource.h> include is buried inside a preprocessor block
Placing #include <sys/resource.h> inside the #else (non-Windows) block at line 121 is unconventional. System includes are typically grouped at the top of the file for discoverability. While functionally correct, this makes it easy to miss during code review.
Suggested fix:
Move the include to the system includes section at the top of the file, wrapped in #ifndef WIN32:
#ifndef WIN32
#include <sys/resource.h>
#endif🤖 Prompt for AI agents
"In file src/init.cpp, line 121, #include <sys/resource.h> is placed inside the #ifdef WIN32 / #else block around line 114-123. Move it to the system includes section near the top of the file (around lines 30-50), wrapped in #ifndef WIN32 / #endif. Remove it from its current location inside the preprocessor block. Keep the #define MIN_CORE_FILEDESCRIPTORS 150 in the #else branch."
🤖 Combined AI Fix PromptVerify each finding against the current code and only fix it if needed.
|
Correction / Clarification on the Architectural ConcernI flagged that the root cause of the crash is in Why the crash happens (on master):
How this PR fixes it (indirectly): Recommended targeted fix — change In // Before:
int RaiseFileDescriptorLimit(int nMinFD);
// After:
int RaiseFileDescriptorLimit(int nMinFD); // no change needed here if we clamp insideIn int RaiseFileDescriptorLimit(int nMinFD) {
#if defined(WIN32)
return 2048;
#else
struct rlimit limitFD;
if (getrlimit(RLIMIT_NOFILE, &limitFD) != -1) {
if (limitFD.rlim_cur < (rlim_t)nMinFD) {
limitFD.rlim_cur = nMinFD;
if (limitFD.rlim_cur > limitFD.rlim_max)
limitFD.rlim_cur = limitFD.rlim_max;
setrlimit(RLIMIT_NOFILE, &limitFD);
getrlimit(RLIMIT_NOFILE, &limitFD);
}
// Clamp to INT_MAX to avoid truncation when rlim_cur is RLIM_INFINITY
if (limitFD.rlim_cur > (rlim_t)std::numeric_limits<int>::max())
return std::numeric_limits<int>::max();
return limitFD.rlim_cur;
}
return nMinFD; // getrlimit failed, assume it's fine
#endif
}This approach fixes the crash at the source without requiring any type changes in |
|
Superseded by #1810 |
When ulimits are set to unlimited, getrlimit() returns rlim_cur as RLIM_INFINITY (~0xFFFFFFFFFFFFFFFF). Since RaiseFileDescriptorLimit() returns int, this value is silently truncated to -1, causing a spurious "Not enough file descriptors available" init error. Clamp the return value to INT_MAX before the implicit narrowing conversion. This fixes the root cause at the source rather than requiring callers to change their variable types. See: #1615 Co-authored-by: Reuben Yap <reuben.yap@gmail.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
3 participants
PR intention
Fix a crash on startup.
Code changes brief
If ulimit is set to unlimited, getrlimit will return a value of UINT_MAX, but it was being interpreted as an int, so this was evaluated as -1 and led to a crash on startup thinking resource limits had been hit.
Note
Medium Risk
Touches early startup logic that computes file descriptor and connection limits; incorrect type conversions could impact peer connection caps across platforms, though the change is small and localized.
Overview
Fixes a startup crash when the process file-descriptor limit is set to unlimited by switching connection/fd-limit bookkeeping in
init.cppfrominttorlim_tand updating thestd::min/std::maxclamping casts accordingly.Adds the needed platform-specific
rlim_tdefinition/include (<sys/resource.h>on non-Windows; typedef on Windows) so resource-limit values returned fromgetrlimitaren’t misinterpreted as negative and treated as exhausted limits during initialization.Written by Cursor Bugbot for commit a2e2da3. This will update automatically on new commits. Configure here.