Skip to content

Commit 1a2523e

Browse files
achow101achow101
committed
Merge bitcoin#34937: Fix startup failure with RLIM_INFINITY fd limits
735b255 support: clamp RLIMIT_MEMLOCK to size_t (Sjors Provoost) 8ab4b9f init: clamp fd limits to int (Sjors Provoost) 4afbabd Fix startup failure with RLIM_INFINITY fd limits (Sjors Provoost) Pull request description: When setting the fd limit to unlimited, the node fails to start: ```sh ulimit -n unlimited build/bin/bitcoind Error: Not enough file descriptors available. -1 available, 160 required. ``` This was caused by `RaiseFileDescriptorLimit()` (introduced in bitcoin#2568) casting `limitFD.rlim_cur` to `int`, which for `RLIM_INFINITY` overflows to `-1`. Fix it by returning `std::numeric_limits<int>::max()` instead. Some platforms implement `RLIM_INFINITY` as the maximum uint64, others as int64 (-1). So simply changing the return type to `uint64_t` wouldn't work. Similarly, though unlikely to actually happen: ```sh ulimit -n 214748364 build/bin/bitcoind Error: Not enough file descriptors available. -2147483648 available, 160 required. ``` The second commit expands the fix by clamping all values above `std::numeric_limits<int>::max()` instead of letting them overflow. This PR also expands `test/functional/feature_init.py` to cover these, using `resource.setrlimit`. The check is skipped on environments with a hard limit below infinity (or that don't have the Python [Resource module](https://docs.python.org/3/library/resource.html)). macOS by default has a hard limit of infinity, but on e.g. Ubuntu the default hard limit is 524288. The third commit applies a similar fix to `PosixLockedPageAllocator::GetLimit()` for 32-bit systems, but without a test. ACKs for top commit: winterrdog: Re-ACK 735b255 achow101: ACK 735b255 sedited: Re-ACK 735b255 pinheadmz: ACK 735b255 Tree-SHA512: 0ce0292ecd61456bdec6943b06cbb9ecfc5180ee6dce850f8496ef54af22c1fae6ea473085202f5ba6f72e4dc51a29247620c9a0eae31e96658adc77b293129f
2 parents 5883ba7 + 735b255 commit 1a2523e

5 files changed

Lines changed: 89 additions & 14 deletions

File tree

‎src/support/lockedpool.cpp‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -262,7 +262,8 @@ size_t PosixLockedPageAllocator::GetLimit()
262262
#ifdef RLIMIT_MEMLOCK
263263
struct rlimit rlim;
264264
if (getrlimit(RLIMIT_MEMLOCK, &rlim) == 0) {
265-
if (rlim.rlim_cur != RLIM_INFINITY) {
265+
if (rlim.rlim_cur != RLIM_INFINITY &&
266+
std::cmp_less_equal(rlim.rlim_cur, static_cast<rlim_t>(std::numeric_limits<size_t>::max()))) {
266267
return rlim.rlim_cur;
267268
}
268269
}

‎src/util/fs_helpers.cpp‎

Lines changed: 27 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -10,12 +10,14 @@
1010
#include <sync.h>
1111
#include <tinyformat.h>
1212
#include <util/byte_units.h> // IWYU pragma: keep
13+
#include <util/check.h>
1314
#include <util/fs.h>
1415
#include <util/log.h>
1516
#include <util/syserror.h>
1617

1718
#include <cerrno>
1819
#include <fstream>
20+
#include <limits>
1921
#include <map>
2022
#include <memory>
2123
#include <optional>
@@ -154,27 +156,40 @@ bool TruncateFile(FILE* file, unsigned int length)
154156
#endif
155157
}
156158

157-
/**
158-
* this function tries to raise the file descriptor limit to the requested number.
159-
* It returns the actual file descriptor limit (which may be more or less than nMinFD)
160-
*/
161-
int RaiseFileDescriptorLimit(int nMinFD)
159+
int RaiseFileDescriptorLimit(int min_fd)
162160
{
161+
Assert(min_fd >= 0);
163162
#if defined(WIN32)
164163
return 2048;
165164
#else
166165
struct rlimit limitFD;
167166
if (getrlimit(RLIMIT_NOFILE, &limitFD) != -1) {
168-
if (limitFD.rlim_cur < (rlim_t)nMinFD) {
169-
limitFD.rlim_cur = nMinFD;
170-
if (limitFD.rlim_cur > limitFD.rlim_max)
167+
// If the current soft limit is already higher, don't raise it
168+
if (limitFD.rlim_cur != RLIM_INFINITY && std::cmp_less(limitFD.rlim_cur, min_fd)) {
169+
const auto current_limit{limitFD.rlim_cur};
170+
static_assert(std::in_range<rlim_t>(std::numeric_limits<int>::max()));
171+
limitFD.rlim_cur = static_cast<rlim_t>(min_fd);
172+
// Don't raise soft limit beyond hard limit
173+
if ((limitFD.rlim_max != RLIM_INFINITY) && (limitFD.rlim_cur > limitFD.rlim_max)) {
171174
limitFD.rlim_cur = limitFD.rlim_max;
172-
setrlimit(RLIMIT_NOFILE, &limitFD);
173-
getrlimit(RLIMIT_NOFILE, &limitFD);
175+
}
176+
if (current_limit != limitFD.rlim_cur) {
177+
setrlimit(RLIMIT_NOFILE, &limitFD);
178+
getrlimit(RLIMIT_NOFILE, &limitFD);
179+
}
180+
}
181+
// Check the (possibly raised) current soft limit against the special
182+
// value of RLIM_INFINITY. Some platforms implement this as the maximum
183+
// uint64, others as int64 (-1). Avoid casting even if the return type
184+
// is changed to uint64_t. We also cap unlikely but possible values
185+
// that would overflow int.
186+
if (limitFD.rlim_cur == RLIM_INFINITY ||
187+
std::cmp_greater_equal(limitFD.rlim_cur, std::numeric_limits<int>::max())) {
188+
return std::numeric_limits<int>::max();
174189
}
175-
return limitFD.rlim_cur;
190+
return static_cast<int>(limitFD.rlim_cur);
176191
}
177-
return nMinFD; // getrlimit failed, assume it's fine
192+
return min_fd; // getrlimit failed, assume it's fine
178193
#endif
179194
}
180195

‎src/util/fs_helpers.h‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,18 @@ bool FileCommit(FILE* file);
4545
void DirectoryCommit(const fs::path& dirname);
4646

4747
bool TruncateFile(FILE* file, unsigned int length);
48-
int RaiseFileDescriptorLimit(int nMinFD);
48+
49+
/**
50+
* Try to raise the file descriptor limit to the requested number.
51+
*
52+
* @param[in] min_fd The requested minimum number of file descriptors.
53+
* @returns The actual file descriptor limit. It may be lower or
54+
* higher than min_fd. Returns std::numeric_limits<int>::max()
55+
* if the OS imposes no limit (RLIM_INFINITY).
56+
*
57+
*/
58+
int RaiseFileDescriptorLimit(int min_fd);
59+
4960
void AllocateFileRange(FILE* file, unsigned int offset, unsigned int length);
5061

5162
/**

‎test/functional/feature_init.py‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -323,12 +323,48 @@ def init_empty_test(self):
323323
for option in options:
324324
self.restart_node(1, option)
325325

326+
def restart_node_with_fd_limit(self, limit):
327+
"""Restart node 1 with a given soft RLIMIT_NOFILE. Skips if the limit cannot be set."""
328+
import resource
329+
soft, hard = resource.getrlimit(resource.RLIMIT_NOFILE)
330+
try:
331+
resource.setrlimit(resource.RLIMIT_NOFILE, (limit, hard))
332+
except (ValueError, OSError):
333+
self.log.info(f"Skipping rlimit test: cannot set soft limit (hard={hard})")
334+
return
335+
try:
336+
self.restart_node(1)
337+
self.log.debug(f"Node started successfully with RLIM_INFINITY limit (soft={limit})")
338+
finally:
339+
resource.setrlimit(resource.RLIMIT_NOFILE, (soft, hard))
340+
self.log.debug(f"Restored previous RLIMIT_NOFILE limits (soft={soft}, hard={hard})")
341+
342+
def init_rlimit_test(self):
343+
"""Test that bitcoind starts correctly when the soft RLIMIT_NOFILE limit is RLIM_INFINITY."""
344+
if self.RLIM_INFINITY is None:
345+
self.log.info("Skipping: resource module not available")
346+
return
347+
348+
self.log.info("Testing node startup with RLIM_INFINITY fd limit")
349+
self.restart_node_with_fd_limit(self.RLIM_INFINITY)
350+
351+
def init_rlimit_large_test(self):
352+
"""Test that bitcoind starts correctly when the soft RLIMIT_NOFILE limit is above INT_MAX."""
353+
if self.RLIM_INFINITY is None:
354+
self.log.info("Skipping: resource module not available")
355+
return
356+
357+
self.log.info("Testing node startup with fd limit above INT_MAX")
358+
self.restart_node_with_fd_limit(1 << 31)
359+
326360
def run_test(self):
327361
self.init_pid_test()
328362
self.init_stress_test_interrupt()
329363
self.init_stress_test_removals()
330364
self.break_wait_test()
331365
self.init_empty_test()
366+
self.init_rlimit_test()
367+
self.init_rlimit_large_test()
332368

333369

334370
if __name__ == '__main__':

‎test/functional/test_framework/test_framework.py‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
from enum import Enum
99
import argparse
1010
from datetime import datetime, timezone
11+
from importlib.util import find_spec
1112
import logging
1213
import os
1314
from pathlib import Path
@@ -1082,6 +1083,17 @@ def skip_if_no_previous_releases(self):
10821083
if not self.has_previous_releases():
10831084
raise SkipTest("previous releases not available or disabled")
10841085

1086+
def has_resource_module(self):
1087+
"""Checks whether the resource module is available."""
1088+
return find_spec('resource') is not None
1089+
1090+
@property
1091+
def RLIM_INFINITY(self):
1092+
if not self.has_resource_module():
1093+
return None
1094+
import resource
1095+
return resource.RLIM_INFINITY
1096+
10851097
def has_previous_releases(self):
10861098
"""Checks whether previous releases are present and enabled."""
10871099
if not os.path.isdir(self.options.previous_releases_path):

0 commit comments

Comments
 (0)