Fix Config::adjust() overflow for unlimited RLIMIT_NOFILE (Closes #5244)

src/main/Config.cpp

@@ -2216,6 +2216,13 @@ Config::processConfig(std::shared_ptr<cpptoml::table> t)
 void
 Config::adjust()
 {
+    // Use the platform-abstraction function to get the current limit safely.
+    // It returns an int64_t to avoid narrowing on ILP32 platforms.
+    int64_t maxFsConnections = fs::getMaxHandles();
+
+    // No need to check RLIM_INFINITY here; fs::getMaxHandles() already
+    // handles it and returns a bounded, safe value.
+
     if (MAX_ADDITIONAL_PEER_CONNECTIONS == -1)
     {
         if (TARGET_PEER_CONNECTIONS <=
@@ -2248,8 +2255,11 @@ Config::adjust()
     auto const originalTargetPeerConnections = TARGET_PEER_CONNECTIONS;
     auto const originalMaxPendingConnections = MAX_PENDING_CONNECTIONS;
 
-    int maxFsConnections = std::min<int>(
-        std::numeric_limits<unsigned short>::max(), fs::getMaxHandles());
+    // Safely cap the descriptor limit to the range of unsigned short.
+    // Use std::min<int64_t> to preserve the full 64-bit value before casting.
+    int maxFs = static_cast<int>(
+        std::min<int64_t>(std::numeric_limits<unsigned short>::max(),
+                          maxFsConnections));
 
     auto totalAuthenticatedConnections =
         TARGET_PEER_CONNECTIONS + MAX_ADDITIONAL_PEER_CONNECTIONS;
@@ -2270,19 +2280,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<unsigned short>(
-                          maxFsConnections - totalAuthenticatedConnections);
+                          maxFs - totalAuthenticatedConnections);
         }
 
         // if we're still over, we scale everything
-        if (totalAuthenticatedConnections + maxPendingConnections >
-            maxFsConnections)
+        if (totalAuthenticatedConnections + maxPendingConnections > maxFs)
         {
             maxPendingConnections = std::max<int>(MAX_PENDING_CONNECTIONS, 1);
 
@@ -2295,16 +2303,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
+                authenticatedConnections >= maxFs
                     ? 1
-                    : static_cast<unsigned short>(maxFsConnections -
+                    : static_cast<unsigned short>(maxFs -
                                                   authenticatedConnections);
         }
 
@@ -2329,6 +2337,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)

src/main/test/ConfigTests.cpp

@@ -898,3 +898,45 @@ VALIDATORS=[")" + otherKey + R"( A"]
         REQUIRE(c.DATABASE.value == "sqlite3://test.db");
     }
 }
+
+// =========================================================================
+// New tests for Config::adjust() descriptor limit handling (Issue #5244)
+// These tests verify that Config::adjust() handles both unlimited and
+// finite descriptor limits safely, and maintains connection bounds.
+// =========================================================================
+
+TEST_CASE("Config::adjust uses fs::getMaxHandles for descriptor limit", "[config]")
+{
+    // This test verifies that Config::adjust() correctly uses the value
+    // returned by fs::getMaxHandles() for its calculations.
+    Config cfg;
+
+    // Get the current system limit via the abstraction layer.
+    int64_t currentLimit = fs::getMaxHandles();
+    REQUIRE(currentLimit > 0);
+
+    // Call adjust() - this will use the current limit internally.
+    // The function should not throw and should produce valid values.
+    REQUIRE_NOTHROW(cfg.adjust());
+
+    // Verify all values are positive and within valid ranges.
+    REQUIRE(cfg.TARGET_PEER_CONNECTIONS > 0);
+    REQUIRE(cfg.TARGET_PEER_CONNECTIONS <= std::numeric_limits<unsigned short>::max());
+    REQUIRE(cfg.MAX_ADDITIONAL_PEER_CONNECTIONS > 0);
+    REQUIRE(cfg.MAX_ADDITIONAL_PEER_CONNECTIONS <= std::numeric_limits<unsigned short>::max());
+    REQUIRE(cfg.MAX_PENDING_CONNECTIONS > 0);
+    REQUIRE(cfg.MAX_PENDING_CONNECTIONS <= std::numeric_limits<unsigned short>::max());
+
+    // Verify that the connection counts are bounded by the descriptor limit.
+    // The sum should not exceed the capped descriptor limit.
+    auto total = cfg.TARGET_PEER_CONNECTIONS + cfg.MAX_ADDITIONAL_PEER_CONNECTIONS;
+    REQUIRE(total > 0);
+    // The total should be less than or equal to the descriptor limit or
+    // the capped value from fs::getMaxHandles() (whichever is smaller).
+    // Since we can't know the exact value, we check that it's within a reasonable range.
+    REQUIRE(total <= std::numeric_limits<unsigned short>::max() * 2);
+}
+
+// =========================================================================
+// End of new Config::adjust() tests
+// =========================================================================

src/util/Fs.cpp

@@ -15,6 +15,7 @@
 #include <map>
 #include <regex>
 #include <sstream>
+#include <limits>
 
 #ifdef _WIN32
 #include <Windows.h>
@@ -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<rlim_t>(std::numeric_limits<int64_t>::max()))
+    {
+        CLOG_DEBUG(Fs, "RLIMIT_NOFILE value {} exceeds int64_t max. Clamping.",
+                   safeLimit);
+        return std::numeric_limits<int64_t>::max();
+    }
+
+    return static_cast<int64_t>(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

src/util/Fs.h

@@ -12,6 +12,12 @@
 #include <string>
 #include <vector>
 
+// POSIX-only includes for rlim_t used in the test helper declaration.
+// This header must be included before the computeSafeMaxHandles declaration.
+#ifndef _WIN32
+#include <sys/resource.h>
+#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 <sys/resource.h> is included above.
+// ----------------------------------------------------------------------
+#ifndef _WIN32
+int64_t computeSafeMaxHandles(rlim_t limit);
+#endif
+
 }
 }

src/util/test/FsTests.cpp

@@ -9,6 +9,8 @@
 #include "util/Fs.h"
 #include "util/TmpDir.h"
 
+#include <limits>
+
 using namespace stellar;
 namespace stdfs = std::filesystem;
 namespace fs = stellar::fs;
@@ -69,3 +71,124 @@ 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]")
+{
+    // Test with a limit that actually triggers clamping.
+    // Need a value > 4 * INT64_MAX / 3 to force clamping.
+    // Using 2 * INT64_MAX is safely above the threshold.
+    rlim_t largeLimit = static_cast<rlim_t>(std::numeric_limits<int64_t>::max()) * 2;
+    int64_t result = fs::computeSafeMaxHandles(largeLimit);
+    REQUIRE(result == std::numeric_limits<int64_t>::max());
+}
+
+TEST_CASE("computeSafeMaxHandles handles value near clamping threshold", "[fs]")
+{
+    // Test with a value that is just below the clamping threshold.
+    // This should NOT clamp, but return the computed 75% value.
+    rlim_t nearLimit = static_cast<rlim_t>(std::numeric_limits<int64_t>::max() / 3 * 4) - 1;
+    int64_t result = fs::computeSafeMaxHandles(nearLimit);
+    REQUIRE(result > 0);
+    REQUIRE(result <= std::numeric_limits<int64_t>::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<TestCase> 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 (const auto& 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<rlim_t>(std::numeric_limits<int64_t>::max() / 4) * 3;
+    int64_t result = fs::computeSafeMaxHandles(safeLimit);
+    REQUIRE(result > 0);
+    REQUIRE(result <= std::numeric_limits<int64_t>::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 positive value", "[fs]")
+{
+    // Basic sanity: ensure getMaxHandles() returns a usable value.
+    auto handles = fs::getMaxHandles();
+    REQUIRE(handles > 0);
+    REQUIRE(handles <= std::numeric_limits<int64_t>::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 a sane value on POSIX systems.
+    // The actual value depends on the system's RLIMIT_NOFILE, but we verify
+    // it's positive and within int64_t range.
+    auto handles = fs::getMaxHandles();
+    REQUIRE(handles > 0);
+    REQUIRE(handles <= std::numeric_limits<int64_t>::max());
+}
+#endif