Skip to content

Commit 21ddb8e

Browse files
committed
Address review feedback: Use fs::getMaxHandles and restore connection adjustment logic
- Replaced direct getrlimit call with platform-abstraction fs::getMaxHandles() - Used soft limit (rlim_cur) via fs::getMaxHandles() for accurate capacity - Restored original connection limiting logic (MAX_ADDITIONAL_PEER_CONNECTIONS, etc.) - Replaced CLOG_DEBUG(Config, ...) with LOG_DEBUG(DEFAULT_LOG, ...) - Prevent overflow by capping RLIM_INFINITY safely in adjust() - Kept Config::adjust() platform-independent Resolves #5244
1 parent 83bdc8a commit 21ddb8e

1 file changed

Lines changed: 139 additions & 42 deletions

File tree

src/main/Config.cpp

Lines changed: 139 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -2213,50 +2213,147 @@ Config::processConfig(std::shared_ptr<cpptoml::table> t)
22132213
}
22142214
}
22152215

2216-
void Config::adjust()
2217-
// ================================================================
2218-
// FIX: Handle RLIMIT_NOFILE safely, especially for unlimited values.
2219-
// Addresses GitHub Issue #5244.
2220-
// Previously, fs::getMaxHandles() would overflow for RLIM_INFINITY,
2221-
// leading to potential crashes or undefined behavior when the system
2222-
// imposed no explicit limit on the number of open file descriptors.
2223-
// ================================================================
2224-
struct rlimit rl;
2225-
if (getrlimit(RLIMIT_NOFILE, &rl) == 0)
2226-
{
2227-
// Use a dedicated, explicit type (rlim_t) to match system types
2228-
// and avoid platform-specific size mismatches.
2229-
rlim_t maxHandles = rl.rlim_max;
2230-
2231-
// Check for infinity explicitly to avoid overflow before any
2232-
// arithmetic operations or comparisons.
2233-
if (maxHandles == RLIM_INFINITY)
2234-
{
2235-
// For unlimited, set a practical high boundary that prevents
2236-
// overflow while still allowing high performance for most
2237-
// production workloads. This value is chosen to be safely
2238-
// below typical system limits (e.g., 2^31-1) to avoid
2239-
// any potential side effects from extremely large values.
2240-
rlim_t const SAFE_MAX_HANDLES = 1000000;
2241-
maxHandles = SAFE_MAX_HANDLES;
2242-
CLOG_DEBUG(Config,
2243-
"RLIMIT_NOFILE is unlimited. Capping to {} for safety.",
2244-
SAFE_MAX_HANDLES);
2245-
}
2246-
2247-
// Now assign the safe value to the internal member variable.
2248-
// Casting after the safe check is now guaranteed to be within
2249-
// a reasonable range for the target type.
2250-
mMaxHandles = static_cast<uint64_t>(maxHandles);
2251-
}
2252-
else
2216+
void
2217+
Config::adjust() void Config::adjust()
22532218
{
2254-
// Fallback in case getrlimit fails unexpectedly (e.g., on
2255-
// non-POSIX-compliant systems or due to permission issues).
2256-
CLOG_WARNING(Config, "getrlimit(RLIMIT_NOFILE) failed. Using default.");
2257-
mMaxHandles = DEFAULT_MAX_HANDLES; // Ensure DEFAULT_MAX_HANDLES is defined
2219+
// Use the platform-abstraction function to get the current limit safely.
2220+
long maxFsConnections = fs::getMaxHandles();
2221+
2222+
// Handle the case where the limit is unlimited (RLIM_INFINITY) to prevent
2223+
// overflow.
2224+
if (maxFsConnections == RLIM_INFINITY)
2225+
{
2226+
// Set a practical, safe high boundary.
2227+
maxFsConnections = 1000000;
2228+
LOG_DEBUG(DEFAULT_LOG,
2229+
"RLIMIT_NOFILE is unlimited. Capping connection adjustments "
2230+
"to {} for safety.",
2231+
maxFsConnections);
2232+
}
2233+
2234+
// --- Rest of the original adjust() logic, using the safe maxFsConnections
2235+
// value ---
2236+
if (MAX_ADDITIONAL_PEER_CONNECTIONS == -1)
2237+
{
2238+
if (TARGET_PEER_CONNECTIONS <=
2239+
std::numeric_limits<unsigned short>::max() / 8)
2240+
{
2241+
MAX_ADDITIONAL_PEER_CONNECTIONS = TARGET_PEER_CONNECTIONS * 8;
2242+
}
2243+
else
2244+
{
2245+
MAX_ADDITIONAL_PEER_CONNECTIONS =
2246+
std::numeric_limits<unsigned short>::max();
2247+
}
2248+
}
2249+
2250+
// Ensure outbound connections are capped based on inbound rate
2251+
int limit =
2252+
MAX_ADDITIONAL_PEER_CONNECTIONS / OverlayManager::MIN_INBOUND_FACTOR +
2253+
OverlayManager::MIN_INBOUND_FACTOR;
2254+
if (static_cast<int>(TARGET_PEER_CONNECTIONS) > limit)
2255+
{
2256+
TARGET_PEER_CONNECTIONS = static_cast<unsigned short>(limit);
2257+
LOG_WARNING(DEFAULT_LOG,
2258+
"Adjusted TARGET_PEER_CONNECTIONS to {} due to "
2259+
"insufficient MAX_ADDITIONAL_PEER_CONNECTIONS={}",
2260+
limit, MAX_ADDITIONAL_PEER_CONNECTIONS);
2261+
}
2262+
2263+
// Adjust connection limits based on the safe maxFsConnections
2264+
auto const originalMaxAdditionalPeerConnections =
2265+
MAX_ADDITIONAL_PEER_CONNECTIONS;
2266+
auto const originalTargetPeerConnections = TARGET_PEER_CONNECTIONS;
2267+
auto const originalMaxPendingConnections = MAX_PENDING_CONNECTIONS;
2268+
2269+
int maxFs = std::min<int>(std::numeric_limits<unsigned short>::max(),
2270+
maxFsConnections);
2271+
2272+
auto totalAuthenticatedConnections =
2273+
TARGET_PEER_CONNECTIONS + MAX_ADDITIONAL_PEER_CONNECTIONS;
2274+
int maxPendingConnections = MAX_PENDING_CONNECTIONS;
2275+
2276+
if (totalAuthenticatedConnections > 0)
2277+
{
2278+
auto outboundPendingRate =
2279+
double(TARGET_PEER_CONNECTIONS) / totalAuthenticatedConnections;
2280+
auto doubleToNonzeroUnsignedShort = [](double v) {
2281+
auto rounded = static_cast<int>(std::ceil(v));
2282+
auto cappedToUnsignedShort = std::min<int>(
2283+
std::numeric_limits<unsigned short>::max(), rounded);
2284+
return static_cast<unsigned short>(
2285+
std::max<int>(1, cappedToUnsignedShort));
2286+
};
2287+
2288+
if (totalAuthenticatedConnections + maxPendingConnections > maxFs)
2289+
{
2290+
maxPendingConnections =
2291+
totalAuthenticatedConnections >= maxFs
2292+
? 1
2293+
: static_cast<unsigned short>(
2294+
maxFs - totalAuthenticatedConnections);
2295+
}
2296+
2297+
if (totalAuthenticatedConnections + maxPendingConnections > maxFs)
2298+
{
2299+
maxPendingConnections = std::max<int>(MAX_PENDING_CONNECTIONS, 1);
2300+
int totalRequiredConnections =
2301+
totalAuthenticatedConnections + maxPendingConnections;
2302+
auto outboundRate =
2303+
(double)TARGET_PEER_CONNECTIONS / totalRequiredConnections;
2304+
auto inboundRate = (double)MAX_ADDITIONAL_PEER_CONNECTIONS /
2305+
totalRequiredConnections;
2306+
2307+
TARGET_PEER_CONNECTIONS =
2308+
doubleToNonzeroUnsignedShort(maxFs * outboundRate);
2309+
MAX_ADDITIONAL_PEER_CONNECTIONS =
2310+
doubleToNonzeroUnsignedShort(maxFs * inboundRate);
2311+
2312+
auto authenticatedConnections =
2313+
TARGET_PEER_CONNECTIONS + MAX_ADDITIONAL_PEER_CONNECTIONS;
2314+
maxPendingConnections = authenticatedConnections >= maxFs
2315+
? 1
2316+
: static_cast<unsigned short>(
2317+
maxFs - authenticatedConnections);
2318+
}
2319+
2320+
MAX_PENDING_CONNECTIONS = static_cast<unsigned short>(std::min<int>(
2321+
std::numeric_limits<unsigned short>::max(), maxPendingConnections));
2322+
2323+
if (MAX_OUTBOUND_PENDING_CONNECTIONS == 0 &&
2324+
MAX_INBOUND_PENDING_CONNECTIONS == 0)
2325+
{
2326+
MAX_OUTBOUND_PENDING_CONNECTIONS = std::max<unsigned short>(
2327+
1, doubleToNonzeroUnsignedShort(MAX_PENDING_CONNECTIONS *
2328+
outboundPendingRate));
2329+
MAX_INBOUND_PENDING_CONNECTIONS = std::max<unsigned short>(
2330+
1, MAX_PENDING_CONNECTIONS - MAX_OUTBOUND_PENDING_CONNECTIONS);
2331+
}
2332+
}
2333+
else
2334+
{
2335+
MAX_OUTBOUND_PENDING_CONNECTIONS = 0;
2336+
MAX_INBOUND_PENDING_CONNECTIONS = 0;
2337+
}
2338+
2339+
auto warnIfChanged = [&](std::string const name, auto const originalValue,
2340+
auto const newValue) {
2341+
if (originalValue != newValue)
2342+
{
2343+
LOG_WARNING(DEFAULT_LOG,
2344+
"Adjusted {} from {} to {} due to OS limits (the "
2345+
"maximum number of file descriptors)",
2346+
name, originalValue, newValue);
2347+
}
2348+
};
2349+
warnIfChanged("MAX_ADDITIONAL_PEER_CONNECTIONS",
2350+
originalMaxAdditionalPeerConnections,
2351+
MAX_ADDITIONAL_PEER_CONNECTIONS);
2352+
warnIfChanged("TARGET_PEER_CONNECTIONS", originalTargetPeerConnections,
2353+
TARGET_PEER_CONNECTIONS);
2354+
warnIfChanged("MAX_PENDING_CONNECTIONS", originalMaxPendingConnections,
2355+
MAX_PENDING_CONNECTIONS);
22582356
}
2259-
// ================================================================
22602357

22612358
void
22622359
Config::logBasicInfo() const

0 commit comments

Comments
 (0)