Fix critical issues in close_all_non_term_fd() per code review

This commit addresses critical issues identified in PR #5276 by
gemini-code-assist's code review, which could undermine the goal of
being allocation-free and cause hangs or silent failures.

Bug 1: Vector Passed by Value (Critical)
------------------------------------------
The function took std::vector<int> excludeFDs by value, causing heap
allocation during the copy operation. This undermines the PR's goal of
avoiding heap allocations after fork() to prevent deadlocks in
multi-threaded programs.

Fix: Change to pass by const reference to avoid heap allocation.
  void close_all_non_term_fd(const std::vector<int>& excludeFDs)

Bug 2: Infinite Loop Risk (Critical)
------------------------------------
The loop used unsigned int for the variable while comparing against
rlim_t (unsigned long long). If rlim_cur exceeded UINT_MAX, this would
create an infinite loop.

Fix: Use rlim_t type for the loop variable and cap at INT_MAX.
  for (rlim_t fd_rlim = 3; fd_rlim < nlimit.rlim_cur && fd_rlim <= INT_MAX; fd_rlim++)

Bug 3: close_range() Detection Logic (High)
------------------------------------------
The original detection logic had two problems:
1. Executed close_range syscall twice on first successful call
2. Incorrectly cached availability on transient failures (EINTR),
   leaving file descriptors open without fallback

Fix: Reordered logic to only cache on success, allow retry on
transient failures. Only cache as "not available" on ENOSYS.
For other errors (EBADF, EINVAL, etc.), don't cache - might be transient.

Files Modified
--------------
- include/proxysql_utils.h
- lib/proxysql_utils.cpp
pull/5276/head
Rene Cannao 4 months ago
parent 2448b12a56
commit b39e193f4f

@ -263,7 +263,21 @@ inline void replace_checksum_zeros(char* checksum) {
*/
std::string get_checksum_from_hash(uint64_t hash);
void close_all_non_term_fd(std::vector<int> excludeFDs);
/**
* @brief Closes all open file descriptors except stdin (0), stdout (1), stderr (2), and a specified exclusion list
*
* This function is typically called after fork() in the child process before exec() to ensure that
* the child process does not inherit unintended file descriptors from the parent.
*
* CRITICAL: This function is designed to be called between fork() and execve() in the child process.
* To avoid deadlocks in multi-threaded programs, it must NOT allocate on the heap.
*
* @param excludeFDs Vector of file descriptors to preserve (in addition to 0, 1, 2)
* Passed by const reference to avoid heap allocation during copy.
*
* Thread-safety: Safe to call in child process after fork() before execve()
*/
void close_all_non_term_fd(const std::vector<int>& excludeFDs);
/**
* @brief Returns the expected error for query 'SELECT $$'.

@ -587,23 +587,31 @@ std::string get_checksum_from_hash(uint64_t hash) {
* @param excludeFDs Vector of file descriptors to preserve (in addition to 0, 1, 2)
* @return void
*/
void close_all_non_term_fd(std::vector<int> excludeFDs) {
void close_all_non_term_fd(const std::vector<int>& excludeFDs) {
// Try close_range() syscall first (Linux 5.9+) - most efficient and safe
// We use syscall directly with runtime detection to avoid hard dependency on kernel version
// close_range() can ONLY be used when excludeFDs is empty, because it closes all fds >= 3
#ifdef __NR_close_range
if (excludeFDs.empty()) {
static int close_range_available = -1; // -1 = unknown, 0 = not available, 1 = available
if (close_range_available == -1) {
// First call: check if close_range is available
long ret = syscall(__NR_close_range, 3, ~0U, 0);
close_range_available = (ret == 0 || errno != ENOSYS) ? 1 : 0;
}
if (close_range_available == 1) {
// close_range is available, use it to close all fds >= 3
syscall(__NR_close_range, 3, ~0U, 0);
return;
}
if (close_range_available == -1) {
// First call: check if close_range is available
long ret = syscall(__NR_close_range, 3, ~0U, 0);
if (ret == 0) {
close_range_available = 1;
return;
}
// Only cache as "not available" on ENOSYS
// For other errors (EBADF, EINVAL, etc.), don't cache - might be transient
if (errno == ENOSYS) {
close_range_available = 0;
}
}
}
#endif
@ -642,11 +650,14 @@ void close_all_non_term_fd(std::vector<int> excludeFDs) {
struct rlimit nlimit;
int rc = getrlimit(RLIMIT_NOFILE, &nlimit);
if (rc == 0) {
for (unsigned int fd = 3; fd < nlimit.rlim_cur; fd++) {
// Use rlim_t for the loop variable to avoid infinite loop when rlim_cur > UINT_MAX
// Cap at INT_MAX since file descriptors are signed ints
for (rlim_t fd_rlim = 3; fd_rlim < nlimit.rlim_cur && fd_rlim <= INT_MAX; fd_rlim++) {
int fd = (int)fd_rlim;
// Check if fd is in exclusion list
bool exclude = false;
for (size_t i = 0; i < excludeFDs.size(); i++) {
if (excludeFDs[i] == (int)fd) {
if (excludeFDs[i] == fd) {
exclude = true;
break;
}

Loading…
Cancel
Save