diff --git a/src/main/Config.cpp b/src/main/Config.cpp index ec5725e6d3..9cee0ce139 100644 --- a/src/main/Config.cpp +++ b/src/main/Config.cpp @@ -2215,6 +2215,15 @@ Config::processConfig(std::shared_ptr t) void Config::adjust() +{ + // Query the current descriptor limit through the platform abstraction. + // fs::getMaxHandles() always returns a bounded, non-negative int64_t, so + // RLIM_INFINITY and very large finite limits can never overflow here. + adjust(fs::getMaxHandles()); +} + +void +Config::adjust(int64_t maxFsConnections) { if (MAX_ADDITIONAL_PEER_CONNECTIONS == -1) { @@ -2248,8 +2257,13 @@ Config::adjust() auto const originalTargetPeerConnections = TARGET_PEER_CONNECTIONS; auto const originalMaxPendingConnections = MAX_PENDING_CONNECTIONS; - int maxFsConnections = std::min( - std::numeric_limits::max(), fs::getMaxHandles()); + // Safely clamp the descriptor budget to the range representable by + // unsigned short. Keep the arithmetic in int64_t and bound both ends so + // that negative (or huge) budgets are normalized before the narrowing + // cast below. + int maxFs = static_cast(std::max( + 0, std::min(std::numeric_limits::max(), + maxFsConnections))); auto totalAuthenticatedConnections = TARGET_PEER_CONNECTIONS + MAX_ADDITIONAL_PEER_CONNECTIONS; @@ -2270,19 +2284,17 @@ Config::adjust() }; // see if we need to reduce maxPendingConnections - if (totalAuthenticatedConnections + maxPendingConnections > - maxFsConnections) + if (totalAuthenticatedConnections + maxPendingConnections > maxFs) { maxPendingConnections = - totalAuthenticatedConnections >= maxFsConnections + totalAuthenticatedConnections >= maxFs ? 1 : static_cast( - maxFsConnections - totalAuthenticatedConnections); + maxFs - totalAuthenticatedConnections); } // if we're still over, we scale everything - if (totalAuthenticatedConnections + maxPendingConnections > - maxFsConnections) + if (totalAuthenticatedConnections + maxPendingConnections > maxFs) { maxPendingConnections = std::max(MAX_PENDING_CONNECTIONS, 1); @@ -2295,17 +2307,16 @@ Config::adjust() totalRequiredConnections; TARGET_PEER_CONNECTIONS = - doubleToNonzeroUnsignedShort(maxFsConnections * outboundRate); + doubleToNonzeroUnsignedShort(maxFs * outboundRate); MAX_ADDITIONAL_PEER_CONNECTIONS = - doubleToNonzeroUnsignedShort(maxFsConnections * inboundRate); + doubleToNonzeroUnsignedShort(maxFs * inboundRate); auto authenticatedConnections = TARGET_PEER_CONNECTIONS + MAX_ADDITIONAL_PEER_CONNECTIONS; - maxPendingConnections = - authenticatedConnections >= maxFsConnections - ? 1 - : static_cast(maxFsConnections - - authenticatedConnections); + maxPendingConnections = authenticatedConnections >= maxFs + ? 1 + : static_cast( + maxFs - authenticatedConnections); } MAX_PENDING_CONNECTIONS = static_cast(std::min( @@ -2329,6 +2340,7 @@ Config::adjust() MAX_OUTBOUND_PENDING_CONNECTIONS = 0; MAX_INBOUND_PENDING_CONNECTIONS = 0; } + auto warnIfChanged = [&](std::string const name, auto const originalValue, auto const newValue) { if (originalValue != newValue) diff --git a/src/main/Config.h b/src/main/Config.h index 3f6b6b4c9e..7800a35704 100644 --- a/src/main/Config.h +++ b/src/main/Config.h @@ -998,6 +998,11 @@ class Config : public std::enable_shared_from_this // fixes values of connection-relates settings void adjust(); + // Like adjust(), but takes an explicit file-descriptor budget instead of + // querying the OS limit. Exposed so the connection-limit narrowing logic + // can be exercised deterministically in tests (see Issue #5244). + void adjust(int64_t maxFsConnections); + std::string toShortString(NodeID const& pk) const; // fullKey true => returns full StrKey corresponding to pk diff --git a/src/main/test/ConfigTests.cpp b/src/main/test/ConfigTests.cpp index 71671f5bc3..21b7d5616a 100644 --- a/src/main/test/ConfigTests.cpp +++ b/src/main/test/ConfigTests.cpp @@ -14,6 +14,7 @@ #include #include #include +#include using namespace stellar; namespace stdfs = std::filesystem; @@ -898,3 +899,104 @@ VALIDATORS=[")" + otherKey + R"( A"] REQUIRE(c.DATABASE.value == "sqlite3://test.db"); } } + +// ========================================================================= +// Tests for Config::adjust() descriptor limit handling (Issue #5244). +// +// fs::getMaxHandles() itself is thoroughly tested in FsTests.cpp through the +// computeSafeMaxHandles() helper (RLIM_INFINITY, large finite values, and the +// fallback path). Here we test the consumer side: Config::adjust(int64_t) +// lets us supply an explicit descriptor budget so the connection-limit +// narrowing logic is exercised deterministically, independent of the host's +// actual RLIMIT_NOFILE. +// ========================================================================= +TEST_CASE("Config::adjust handles a very large descriptor limit", "[config]") +{ + // A budget above INT32_MAX used to wrap to a negative int in the old + // code path, collapsing the entire connection budget. The fix stores the + // limit as an int64_t and caps it with std::min before any + // narrowing conversion, so a huge (or unlimited) budget must simply cap + // at unsigned short range and preserve the configured defaults. + Config cfg; + cfg.adjust(std::numeric_limits::max()); + + REQUIRE(cfg.TARGET_PEER_CONNECTIONS == 8); + REQUIRE(cfg.MAX_ADDITIONAL_PEER_CONNECTIONS == 64); + REQUIRE(cfg.MAX_PENDING_CONNECTIONS == 500); + REQUIRE(cfg.MAX_OUTBOUND_PENDING_CONNECTIONS == 56); + REQUIRE(cfg.MAX_INBOUND_PENDING_CONNECTIONS == 444); + + // The outbound/inbound split must always sum back to the total pending + // connection budget; the pre-fix overflow path broke this invariant. + REQUIRE(cfg.MAX_OUTBOUND_PENDING_CONNECTIONS + + cfg.MAX_INBOUND_PENDING_CONNECTIONS == + cfg.MAX_PENDING_CONNECTIONS); +} + +TEST_CASE("Config::adjust scales down a small descriptor limit", "[config]") +{ + // A tight budget must scale the connection counts down proportionally + // without ever producing zero or overflowing unsigned short. + Config cfg; + cfg.adjust(10); + + REQUIRE(cfg.TARGET_PEER_CONNECTIONS == 1); + REQUIRE(cfg.MAX_ADDITIONAL_PEER_CONNECTIONS == 2); + REQUIRE(cfg.MAX_PENDING_CONNECTIONS == 7); + REQUIRE(cfg.MAX_OUTBOUND_PENDING_CONNECTIONS == 1); + REQUIRE(cfg.MAX_INBOUND_PENDING_CONNECTIONS == 6); + REQUIRE(cfg.MAX_OUTBOUND_PENDING_CONNECTIONS + + cfg.MAX_INBOUND_PENDING_CONNECTIONS == + cfg.MAX_PENDING_CONNECTIONS); +} + +TEST_CASE("Config::adjust keeps connection limits in range for all budgets", + "[config]") +{ + // Sweep a range of descriptor budgets, including negative values, zero, + // and values that exceed every relevant type range, and verify the + // invariants that keep the overlay safe: no connection setting is ever + // zero and none overflows unsigned short. Negative budgets must be clamped + // to zero (not narrowed) so the cast to int can never be + // implementation-defined. + std::vector budgets = { + std::numeric_limits::min(), + -1024, + -1, + 0, + 1, + 2, + 10, + 1024, + std::numeric_limits::max(), + std::numeric_limits::max(), + }; + + for (auto budget : budgets) + { + INFO("budget = " << budget); + Config cfg; + cfg.adjust(budget); + + REQUIRE(cfg.TARGET_PEER_CONNECTIONS >= 1); + REQUIRE(cfg.MAX_ADDITIONAL_PEER_CONNECTIONS >= 1); + REQUIRE(cfg.MAX_PENDING_CONNECTIONS >= 1); + REQUIRE(cfg.MAX_OUTBOUND_PENDING_CONNECTIONS >= 1); + REQUIRE(cfg.MAX_INBOUND_PENDING_CONNECTIONS >= 1); + + REQUIRE(cfg.TARGET_PEER_CONNECTIONS <= + std::numeric_limits::max()); + REQUIRE(cfg.MAX_ADDITIONAL_PEER_CONNECTIONS <= + std::numeric_limits::max()); + REQUIRE(cfg.MAX_PENDING_CONNECTIONS <= + std::numeric_limits::max()); + REQUIRE(cfg.MAX_OUTBOUND_PENDING_CONNECTIONS <= + std::numeric_limits::max()); + REQUIRE(cfg.MAX_INBOUND_PENDING_CONNECTIONS <= + std::numeric_limits::max()); + } +} + +// ========================================================================= +// End of ConfigTests.cpp +// ========================================================================= diff --git a/src/util/Fs.cpp b/src/util/Fs.cpp index ada28d9ec9..b4abc53f55 100644 --- a/src/util/Fs.cpp +++ b/src/util/Fs.cpp @@ -12,6 +12,7 @@ #include #include +#include #include #include #include @@ -70,7 +71,6 @@ lockFile(std::string const& path) NULL); if (h == INVALID_HANDLE_VALUE) { - // not sure if there is more verbose info that can be obtained here errmsg << "unable to create lock file: " << path; throw FileSystemException(errmsg.str()); } @@ -210,7 +210,6 @@ unlockFile(std::string const& path) auto it = lockMap.find(path); if (it != lockMap.end()) { - // cannot unlink to avoid potential race close(it->second); lockMap.erase(it); } @@ -429,13 +428,57 @@ size(std::string const& filename) return stdfs::file_size(stdfs::path(filename)); } +#ifndef _WIN32 + +// ---------------------------------------------------------------------- +// Helper function to make the limit calculation directly testable. +// This extracts the core logic from getMaxHandles() so we can test +// boundary cases (RLIM_INFINITY, large values, small remainders) +// without depending on the system's actual rlimit. +// This function is POSIX-only because it uses rlim_t and RLIM_INFINITY. +// ---------------------------------------------------------------------- +int64_t +computeSafeMaxHandles(rlim_t limit) +{ + // Check for infinity before any arithmetic to prevent overflow. + if (limit == RLIM_INFINITY) + { + // Log the capping of unlimited limit to help diagnose issues. + CLOG_DEBUG(Fs, "RLIMIT_NOFILE is unlimited. Capping to 1,000,000."); + return 1000000; + } + + // Compute floor(limit * 3 / 4) without overflow. + // Using (limit / 4) * 3 alone loses the remainder, which matters + // for small limits (e.g., limit=3 should yield 2, not 0). + // The correct safe formula is: + // floor(limit * 3 / 4) = (limit / 4) * 3 + (limit % 4) * 3 / 4 + rlim_t quotient = limit / 4; + rlim_t remainder = limit % 4; + rlim_t safeLimit = quotient * 3 + (remainder * 3) / 4; + + // Clamp to int64_t range to avoid implementation-defined conversion + // when the value exceeds the maximum representable value. + if (safeLimit > static_cast(std::numeric_limits::max())) + { + CLOG_DEBUG(Fs, "RLIMIT_NOFILE value {} exceeds int64_t max. Clamping.", + safeLimit); + return std::numeric_limits::max(); + } + + return static_cast(safeLimit); +} + +#endif // !_WIN32 + #ifdef _WIN32 int64_t getMaxHandles() { - // on Windows, there is no limit on handles - // only limits based on ephemeral ports, etc + // On Windows, there is no system-imposed hard limit on handles. + // The effective limit is typically governed by ephemeral port availability + // and per-process resources. Returning a reasonably high, safe value. return 32000; } @@ -446,10 +489,12 @@ getMaxHandles() struct rlimit rl; if (getrlimit(RLIMIT_NOFILE, &rl) == 0) { - // leave some buffer - return (rl.rlim_cur * 3) / 4; + // Delegate to the testable helper function. + return computeSafeMaxHandles(rl.rlim_cur); } - // could not query the limit, default to a value that should work + + // Fallback if getrlimit fails. + CLOG_DEBUG(Fs, "getrlimit(RLIMIT_NOFILE) failed. Using fallback value 64."); return 64; } #endif diff --git a/src/util/Fs.h b/src/util/Fs.h index 44800ea55b..cb83bef489 100644 --- a/src/util/Fs.h +++ b/src/util/Fs.h @@ -12,6 +12,12 @@ #include #include +// POSIX-only includes for rlim_t used in the test helper declaration. +// This header must be included before the computeSafeMaxHandles declaration. +#ifndef _WIN32 +#include +#endif + namespace stellar { namespace fs @@ -120,5 +126,17 @@ int64_t getOpenHandleCount(); // failed. bool removeWithLog(std::string const& path, bool ignoreEnoent = true); +// ---------------------------------------------------------------------- +// Exposed for testing only - computes safe 75% of an rlimit value. +// This helper extracts the core logic from getMaxHandles() so that +// boundary cases (RLIM_INFINITY, large values, small remainders) can +// be tested directly without depending on the system's actual rlimit. +// On Windows, this function is not defined (rlim_t is POSIX-only). +// The required header is included above. +// ---------------------------------------------------------------------- +#ifndef _WIN32 +int64_t computeSafeMaxHandles(rlim_t limit); +#endif + } } diff --git a/src/util/test/FsTests.cpp b/src/util/test/FsTests.cpp index 900a4f7acd..38e05a9a28 100644 --- a/src/util/test/FsTests.cpp +++ b/src/util/test/FsTests.cpp @@ -9,6 +9,8 @@ #include "util/Fs.h" #include "util/TmpDir.h" +#include + using namespace stellar; namespace stdfs = std::filesystem; namespace fs = stellar::fs; @@ -69,3 +71,178 @@ TEST_CASE("filesystem remoteName", "[fs]") fs::hexStr(0x0abbccdd), "xdr.gz") == "ledger/0a/bb/cc/ledger-0abbccdd.xdr.gz"); } + +// ------------------------------------------------------------------ +// Tests for computeSafeMaxHandles() helper - direct testing of boundary cases +// These tests are POSIX-only because they use rlim_t and RLIM_INFINITY. +// On Windows, computeSafeMaxHandles is not defined. +// ------------------------------------------------------------------ + +#ifndef _WIN32 + +TEST_CASE("computeSafeMaxHandles handles RLIM_INFINITY", "[fs]") +{ + // Direct test of the helper function with RLIM_INFINITY. + // This does NOT depend on the system's actual limit. + // The helper should return the capped value of 1,000,000. + int64_t result = fs::computeSafeMaxHandles(RLIM_INFINITY); + REQUIRE(result == 1000000); +} + +TEST_CASE("computeSafeMaxHandles handles very large finite limits", "[fs]") +{ + // Only exercise the clamping path when rlim_t can actually represent + // values above INT64_MAX. Architecture macros such as __LP64__ are not a + // reliable proxy: on some supported 64-bit platforms (e.g., FreeBSD) + // rlim_t is signed 64-bit and cannot hold 4/3 * INT64_MAX, so constructing + // the value below would overflow before the helper is even called. Branch + // on the type's value bits instead. + if constexpr (std::numeric_limits::digits > + std::numeric_limits::digits) + { + // rlim_t is wider than int64_t, so we can construct a value that is: + // 1. Above the clamping threshold (4/3 * INT64_MAX) + // 2. Explicitly NOT equal to RLIM_INFINITY + rlim_t largeLimit = + static_cast(std::numeric_limits::max() / 3) * 4 + + 3; + + // Safety check: ensure we're not hitting RLIM_INFINITY by accident + REQUIRE(largeLimit != RLIM_INFINITY); + + int64_t result = fs::computeSafeMaxHandles(largeLimit); + REQUIRE(result == std::numeric_limits::max()); + } + else + { + // rlim_t cannot represent values above INT64_MAX, so the clamping + // path cannot be triggered. Just verify the function returns a sane + // value for a large finite limit. Use a value that is not + // RLIM_INFINITY. + rlim_t largeLimit = 1000000; + int64_t result = fs::computeSafeMaxHandles(largeLimit); + REQUIRE(result > 0); + REQUIRE(result <= std::numeric_limits::max()); + } +} + +TEST_CASE("computeSafeMaxHandles handles value near clamping threshold", "[fs]") +{ + // The intended value is approximately 4/3 * INT64_MAX, which cannot be + // represented when rlim_t is signed 64-bit (e.g., FreeBSD). Guard this + // threshold test the same way as the large-limit test: only run it when + // rlim_t has more value bits than int64_t. + if constexpr (std::numeric_limits::digits > + std::numeric_limits::digits) + { + // Test with a value that is just below the clamping threshold. + // This should NOT clamp, but return the computed 75% value. + // Cast to rlim_t BEFORE multiplication to avoid signed overflow. + rlim_t nearLimit = + static_cast(std::numeric_limits::max() / 3) * 4 - + 1; + int64_t result = fs::computeSafeMaxHandles(nearLimit); + REQUIRE(result > 0); + REQUIRE(result <= std::numeric_limits::max()); + } +} + +TEST_CASE( + "computeSafeMaxHandles preserves floor(limit * 3 / 4) for small values", + "[fs]") +{ + // Test with small values to verify the remainder handling. + // This ensures the formula (limit / 4) * 3 + (limit % 4) * 3 / 4 + // correctly computes floor(limit * 3 / 4) without overflow. + struct TestCase + { + rlim_t input; + int64_t expected; + }; + + std::vector cases = { + {0, 0}, + {1, 0}, // floor(1 * 0.75) = 0 + {2, 1}, // floor(2 * 0.75) = 1 + {3, 2}, // floor(3 * 0.75) = 2 + {4, 3}, // floor(4 * 0.75) = 3 + {5, 3}, // floor(5 * 0.75) = 3 + {6, 4}, // floor(6 * 0.75) = 4 + {7, 5}, // floor(7 * 0.75) = 5 + {8, 6}, // floor(8 * 0.75) = 6 + {10, 7}, // floor(10 * 0.75) = 7 + {100, 75}, // floor(100 * 0.75) = 75 + {1000, 750}, // floor(1000 * 0.75) = 750 + {1000000, 750000}, // floor(1,000,000 * 0.75) = 750,000 + }; + + for (auto const& tc : cases) + { + int64_t result = fs::computeSafeMaxHandles(tc.input); + INFO("Input: " << tc.input << ", Expected: " << tc.expected + << ", Got: " << result); + REQUIRE(result == tc.expected); + } +} + +TEST_CASE("computeSafeMaxHandles handles value near int64_t max", "[fs]") +{ + // Test with a value that is close to the maximum but safe. + // This ensures the clamping logic works correctly at the boundary. + rlim_t safeLimit = + static_cast(std::numeric_limits::max() / 4) * 3; + int64_t result = fs::computeSafeMaxHandles(safeLimit); + REQUIRE(result > 0); + REQUIRE(result <= std::numeric_limits::max()); +} + +TEST_CASE("computeSafeMaxHandles handles zero", "[fs]") +{ + // Edge case: zero limit should return zero. + int64_t result = fs::computeSafeMaxHandles(0); + REQUIRE(result == 0); +} + +#endif // !_WIN32 + +// ------------------------------------------------------------------ +// Integration tests for getMaxHandles() - verify it calls the helper +// ------------------------------------------------------------------ + +TEST_CASE("getMaxHandles returns a value within int64_t range", "[fs]") +{ + // Basic sanity: ensure getMaxHandles() returns a value within int64_t + // range. The value may be 0 if the system limit is 0 or 1, which is valid. + auto handles = fs::getMaxHandles(); + REQUIRE(handles <= std::numeric_limits::max()); +} + +#ifdef _WIN32 +TEST_CASE("getMaxHandles Windows returns fixed value", "[fs]") +{ + // On Windows, getMaxHandles() returns a fixed value of 32,000. + auto handles = fs::getMaxHandles(); + REQUIRE(handles == 32000); +} +#else +TEST_CASE("getMaxHandles POSIX integration test", "[fs]") +{ + // This test verifies that getMaxHandles() delegates to + // computeSafeMaxHandles() and returns the expected value based on the + // system's RLIMIT_NOFILE. + struct rlimit rl; + if (getrlimit(RLIMIT_NOFILE, &rl) == 0) + { + // The value should match computeSafeMaxHandles(rl.rlim_cur) + int64_t expected = fs::computeSafeMaxHandles(rl.rlim_cur); + int64_t actual = fs::getMaxHandles(); + REQUIRE(actual == expected); + } + else + { + // If getrlimit fails, getMaxHandles() should return 64. + int64_t actual = fs::getMaxHandles(); + REQUIRE(actual == 64); + } +} +#endif