From 4a751a2ceb59d0a386cb59f6bbf9b4a34bfd77e6 Mon Sep 17 00:00:00 2001 From: Stuart Inglis Date: Wed, 1 Jul 2026 08:03:13 +1200 Subject: [PATCH 1/2] io: use poll() instead of select() to avoid an FD_SETSIZE hang (issue #231) rsync's I/O loops (safe_read, safe_write, and the main perform_io multiplexer) waited for readiness with select() and fd_set bitmaps. An fd_set can only represent descriptors below FD_SETSIZE (1024 with glibc). When rsync is started with many descriptors already open -- e.g. inherited from a parent process that leaked fds, a high "ulimit -n", or a busy daemon -- its own socket and pipe fds get allocated at or above 1024. FD_SET() and FD_ISSET() then index past the end of the fixed-size fd_set, which is undefined behavior: select() reports the fd as ready, but FD_ISSET() reads the out-of-bounds bit as 0, so the read or write never happens and rsync spins at 100% CPU forever with no progress. This is the long-standing "rsync hangs at 100% CPU on large systems" report, and it matches the MemorySanitizer use-of-uninitialized-value seen in perform_io. Convert the three loops to poll(), which identifies descriptors by value in a small array and has no FD_SETSIZE ceiling, so a high-numbered fd works fine. rsync only ever waits on a handful of fds (at most three in perform_io: in_fd, out_fd, and the files-from forward fd), so poll() is as fast as -- or faster than -- select() here; the select()-vs-poll() cost gap only appears when watching thousands of descriptors, which rsync never does. The remaining select(0, ...) call is a pure timed sleep with no fds and is unaffected. The conversion is behavior-preserving: the same max_fd bookkeeping decides when there is nothing to wait on, the per-fd readiness checks map to the matching pollfd revents, and the timeout is the same (now expressed in milliseconds). testsuite/highfd-hang_test.py reproduces the hang deterministically by opening enough inheritable dummy fds to push rsync's descriptors past FD_SETSIZE before an ordinary transfer; it hangs (caught by a timeout) on the select() code and passes instantly with poll(). --- io.c | 94 +++++++++++++++++++---------------- testsuite/highfd-hang_test.py | 84 +++++++++++++++++++++++++++++++ 2 files changed, 134 insertions(+), 44 deletions(-) create mode 100644 testsuite/highfd-hang_test.py diff --git a/io.c b/io.c index 2dfeeaa34..e9de0b713 100644 --- a/io.c +++ b/io.c @@ -31,7 +31,9 @@ #include "ifuncs.h" #include "inums.h" -/** If no timeout is specified then use a 60 second select timeout */ +#include + +/** If no timeout is specified then use a 60 second I/O timeout */ #define SELECT_TIMEOUT 60 extern int bwlimit; @@ -243,31 +245,26 @@ static size_t safe_read(int fd, char *buf, size_t len) assert(fd != iobuf.in_fd); while (1) { - struct timeval tv; - fd_set r_fds, e_fds; + struct pollfd pfd; int cnt; - FD_ZERO(&r_fds); - FD_SET(fd, &r_fds); - FD_ZERO(&e_fds); - FD_SET(fd, &e_fds); - tv.tv_sec = select_timeout; - tv.tv_usec = 0; + /* We use poll() rather than select() so that a high-numbered fd + * (>= FD_SETSIZE) cannot overflow an fd_set bitmap. */ + pfd.fd = fd; + pfd.events = POLLIN; + pfd.revents = 0; - cnt = select(fd+1, &r_fds, NULL, &e_fds, &tv); + cnt = poll(&pfd, 1, select_timeout * 1000); if (cnt <= 0) { if (cnt < 0 && errno == EBADF) { - rsyserr(FERROR, errno, "safe_read select failed"); + rsyserr(FERROR, errno, "safe_read poll failed"); exit_cleanup(RERR_FILEIO); } check_timeout(1, MSK_ALLOW_FLUSH); continue; } - /*if (FD_ISSET(fd, &e_fds)) - rprintf(FINFO, "select exception on fd %d\n", fd); */ - - if (FD_ISSET(fd, &r_fds)) { + if (pfd.revents) { ssize_t n = read(fd, buf + got, len - got); if (DEBUG_GTE(IO, 2)) { rprintf(FINFO, "[%s] safe_read(%d)=%" SIZE_T_FMT_MOD "d\n", @@ -332,19 +329,18 @@ static void safe_write(int fd, const char *buf, size_t len) } while (len) { - struct timeval tv; - fd_set w_fds; + struct pollfd pfd; int cnt; - FD_ZERO(&w_fds); - FD_SET(fd, &w_fds); - tv.tv_sec = select_timeout; - tv.tv_usec = 0; + /* poll() avoids the FD_SETSIZE limit that select() imposes. */ + pfd.fd = fd; + pfd.events = POLLOUT; + pfd.revents = 0; - cnt = select(fd + 1, NULL, &w_fds, NULL, &tv); + cnt = poll(&pfd, 1, select_timeout * 1000); if (cnt <= 0) { if (cnt < 0 && errno == EBADF) { - rsyserr(FERROR, errno, "safe_write select failed on %s", what_fd_is(fd)); + rsyserr(FERROR, errno, "safe_write poll failed on %s", what_fd_is(fd)); exit_cleanup(RERR_FILEIO); } if (io_timeout) @@ -352,7 +348,7 @@ static void safe_write(int fd, const char *buf, size_t len) continue; } - if (FD_ISSET(fd, &w_fds)) { + if (pfd.revents) { n = write(fd, buf, len); if (n < 0) { if (errno == EINTR) @@ -561,9 +557,8 @@ static void handle_kill_signal(BOOL flush_ok) * unused raw data in the buf would prevent the reading of socket data. */ static char *perform_io(size_t needed, int flags) { - fd_set r_fds, e_fds, w_fds; - struct timeval tv; - int cnt, max_fd; + struct pollfd pfds[3]; + int cnt, max_fd, npfds, poll_timeout, in_pollpos, out_pollpos, ff_pollpos; size_t empty_buf_len = 0; xbuf *out; char *data; @@ -656,13 +651,15 @@ static char *perform_io(size_t needed, int flags) } max_fd = -1; + npfds = 0; + in_pollpos = out_pollpos = ff_pollpos = -1; - FD_ZERO(&r_fds); - FD_ZERO(&e_fds); if (iobuf.in_fd >= 0 && iobuf.in.size - iobuf.in.len) { if (!read_batch || batch_fd >= 0) { - FD_SET(iobuf.in_fd, &r_fds); - FD_SET(iobuf.in_fd, &e_fds); + pfds[npfds].fd = iobuf.in_fd; + pfds[npfds].events = POLLIN; + pfds[npfds].revents = 0; + in_pollpos = npfds++; } if (iobuf.in_fd > max_fd) max_fd = iobuf.in_fd; @@ -670,12 +667,14 @@ static char *perform_io(size_t needed, int flags) /* Only do more filesfrom processing if there is enough room in the out buffer. */ if (ff_forward_fd >= 0 && iobuf.out.size - iobuf.out.len > FILESFROM_BUFLEN*2) { - FD_SET(ff_forward_fd, &r_fds); + pfds[npfds].fd = ff_forward_fd; + pfds[npfds].events = POLLIN; + pfds[npfds].revents = 0; + ff_pollpos = npfds++; if (ff_forward_fd > max_fd) max_fd = ff_forward_fd; } - FD_ZERO(&w_fds); if (iobuf.out_fd >= 0) { if (iobuf.raw_flushing_ends_before || (!iobuf.msg.len && iobuf.out.len > iobuf.out_empty_len && !(flags & PIO_NEED_MSGROOM))) { @@ -715,7 +714,10 @@ static char *perform_io(size_t needed, int flags) } else out = NULL; if (out) { - FD_SET(iobuf.out_fd, &w_fds); + pfds[npfds].fd = iobuf.out_fd; + pfds[npfds].events = POLLOUT; + pfds[npfds].revents = 0; + out_pollpos = npfds++; if (iobuf.out_fd > max_fd) max_fd = iobuf.out_fd; } @@ -752,16 +754,15 @@ static char *perform_io(size_t needed, int flags) if (extra_flist_sending_enabled) { if (file_total - file_old_total < MAX_FILECNT_LOOKAHEAD && IN_MULTIPLEXED_AND_READY) - tv.tv_sec = 0; + poll_timeout = 0; else { extra_flist_sending_enabled = False; - tv.tv_sec = select_timeout; + poll_timeout = select_timeout * 1000; } } else - tv.tv_sec = select_timeout; - tv.tv_usec = 0; + poll_timeout = select_timeout * 1000; - cnt = select(max_fd + 1, &r_fds, &w_fds, &e_fds, &tv); + cnt = poll(pfds, npfds, poll_timeout); if (cnt <= 0) { if (cnt < 0 && errno == EBADF) { @@ -774,11 +775,16 @@ static char *perform_io(size_t needed, int flags) extra_flist_sending_enabled = !flist_eof; } else check_timeout((flags & PIO_NEED_INPUT) != 0, 0); - FD_ZERO(&r_fds); /* Just in case... */ - FD_ZERO(&w_fds); + /* Just in case... */ + if (in_pollpos >= 0) + pfds[in_pollpos].revents = 0; + if (ff_pollpos >= 0) + pfds[ff_pollpos].revents = 0; + if (out_pollpos >= 0) + pfds[out_pollpos].revents = 0; } - if (iobuf.in_fd >= 0 && FD_ISSET(iobuf.in_fd, &r_fds)) { + if (iobuf.in_fd >= 0 && in_pollpos >= 0 && pfds[in_pollpos].revents) { size_t len, pos = iobuf.in.pos + iobuf.in.len; ssize_t n; if (pos >= iobuf.in.size) { @@ -827,7 +833,7 @@ static char *perform_io(size_t needed, int flags) exit_cleanup(RERR_TIMEOUT); } - if (out && FD_ISSET(iobuf.out_fd, &w_fds)) { + if (out && out_pollpos >= 0 && pfds[out_pollpos].revents) { size_t len = iobuf.raw_flushing_ends_before ? iobuf.raw_flushing_ends_before - out->pos : out->len; ssize_t n; @@ -888,7 +894,7 @@ static char *perform_io(size_t needed, int flags) wait_for_receiver(); /* generator only */ } - if (ff_forward_fd >= 0 && FD_ISSET(ff_forward_fd, &r_fds)) { + if (ff_forward_fd >= 0 && ff_pollpos >= 0 && pfds[ff_pollpos].revents) { /* This can potentially flush all output and enable * multiplexed output, so keep this last in the loop * and be sure to not cache anything that would break diff --git a/testsuite/highfd-hang_test.py b/testsuite/highfd-hang_test.py new file mode 100644 index 000000000..d86837e2b --- /dev/null +++ b/testsuite/highfd-hang_test.py @@ -0,0 +1,84 @@ +#!/usr/bin/env python3 +"""Regression test for issue #231: rsync must not hang when its I/O file +descriptors are numbered at/above FD_SETSIZE. + +rsync's I/O loops in io.c used to call select() with fd_set bitmaps, which can +only represent descriptors below FD_SETSIZE (1024 with glibc). If rsync is +started with many descriptors already open -- e.g. inherited from a parent that +leaked fds -- its own socket/pipe fds get allocated above 1024. FD_SET() and +FD_ISSET() then index past the end of the fixed-size fd_set, which is undefined +behavior: select() reports the fd ready, but FD_ISSET() reads the out-of-bounds +bit as 0, so the read/write never happens and rsync spins at 100% CPU forever +("rsync hangs, 100% of one CPU, no progress"). + +We reproduce that deterministically by opening enough inheritable dummy fds to +push rsync's descriptors past FD_SETSIZE, then running an ordinary transfer +with close_fds=False so the child inherits them. With the poll()-based io.c +the transfer finishes instantly; with the old select()-based code it never +finishes and is caught by the timeout. The cross-over is binary (instant vs. +infinite), so the timeout is a robust signal rather than a timing race. +""" + +import os +import resource +import subprocess + +from rsyncfns import ( + FROMDIR, TODIR, rmtree, rsync_argv, test_fail, test_skipped, +) + +# select()'s fd_set holds FD_SETSIZE bits; glibc and most libcs use 1024. +FD_SETSIZE = 1024 +NDUMMY = FD_SETSIZE + 80 # force rsync's fds comfortably past the limit +TIMEOUT = 30 # poll build: ~instant; select build: hangs forever + +# We must be able to open enough descriptors to cross FD_SETSIZE. +soft, hard = resource.getrlimit(resource.RLIMIT_NOFILE) +want = NDUMMY + 64 +if soft < want: + if hard != resource.RLIM_INFINITY and hard < want: + test_skipped(f"RLIMIT_NOFILE hard cap {hard} < {want}; cannot place " + "fds above FD_SETSIZE to exercise issue #231") + resource.setrlimit(resource.RLIMIT_NOFILE, (want, hard)) + +# A small tree to transfer. +rmtree(FROMDIR) +rmtree(TODIR) +FROMDIR.mkdir(parents=True, exist_ok=True) +payload = {f"f{i}": os.urandom(1000) for i in range(20)} +for name, data in payload.items(): + (FROMDIR / name).write_bytes(data) + +# Occupy the low fd numbers with inheritable dummy descriptors so that the +# rsync child's socket/pipe fds are forced above FD_SETSIZE. Python opens fds +# O_CLOEXEC by default, so mark each inheritable and use close_fds=False. +dummies = [] +try: + while True: + fd = os.open(os.devnull, os.O_RDONLY) + os.set_inheritable(fd, True) + dummies.append(fd) + if fd >= NDUMMY: + break + + argv = rsync_argv('-a', f'{FROMDIR}/', f'{TODIR}/') + try: + proc = subprocess.run(argv, timeout=TIMEOUT, close_fds=False) + except subprocess.TimeoutExpired: + test_fail("rsync hung with high-numbered fds -- select()/fd_set " + "overflow (issue #231 regression)") +finally: + for fd in dummies: + os.close(fd) + +if proc.returncode != 0: + test_fail(f"rsync exited {proc.returncode}: {' '.join(argv)}") + +# The cap only matters if it stayed correct: verify every file transferred. +for name, data in payload.items(): + got = (TODIR / name).read_bytes() + if got != data: + test_fail(f"{name} differs after a high-fd transfer") + +print(f"issue #231: transfer with fds >= {NDUMMY} (above FD_SETSIZE) " + "completed correctly") From 7ef165dd4540ab5d2db8aa89f74a7286f99e27cd Mon Sep 17 00:00:00 2001 From: Stuart Inglis Date: Mon, 20 Jul 2026 12:39:09 +1200 Subject: [PATCH 2/2] io/socket: address review of the poll() conversion Follow-up to the FD_SETSIZE fix, covering the points raised in review. Negative/overflowing I/O timeouts. set_io_timeout() could produce a negative select_timeout (a peer-supplied MSG_IO_TIMEOUT value was applied unchecked), and every wait now passes select_timeout * 1000 to poll(), where a negative millisecond count means "wait forever" -- so a hostile or buggy peer could stall the other side and bypass keepalives entirely. select() used to reject that with EINVAL, which kept the loop and check_timeout() running. Clamp a negative argument to 0, compute allowed_lull without overflowing near INT_MAX (secs / 2 + secs % 2), ignore a non-positive MSG_IO_TIMEOUT value, and funnel all three waits through poll_timeout_ms(), which keeps the count positive and bounded. The daemon accept loop had the same fd_set overflow. start_accept_loop() still stored listening sockets in an fd_set, so a daemon started with enough descriptors already open got listener fds >= FD_SETSIZE and hit the same undefined behaviour at startup -- verified: with the old code a transfer through such a daemon yields nothing, with this change it succeeds. Converted it to poll() as well. Readiness testing. Treating any non-zero revents as ordinary readiness was wrong: poll() reports POLLERR/POLLHUP/POLLNVAL unrequested, and an invalid fd shows up as POLLNVAL on a successful poll() rather than -1/EBADF, which left the EBADF branches dead and let an invalid ff_forward_fd reach forward_filesfrom_data() (where EBADF reads as EOF). Use role-specific masks (POLL_RD_BITS / POLL_WR_BITS), handle POLLNVAL explicitly in all three loops, and request POLLPRI so select()'s old exception set is not silently dropped. A bidirectional fd is no longer entered twice. A direct daemon connection uses one fd for both directions; it now occupies a single pollfd row with OR-ed events instead of two rows carrying different masks, which also avoids the Cygwin < 3.3.6 duplicate-entry readiness bug. poll() is now a declared requirement: configure.ac checks for poll.h and poll(), failing with a clear message rather than leaving it implicit. The test no longer hardcodes FD_SETSIZE (1024 on glibc but 65536 on 64-bit Solaris, where it would have opened too few fds and passed vacuously); it asks the C library for the real value via a small compiled probe and skips if that is unavailable. Its description now also covers the fortified-libc case, where the pre-fix result is an abort rather than a hang. --- configure.ac | 8 ++- io.c | 96 +++++++++++++++++++++++------ socket.c | 35 +++++------ testsuite/highfd-hang_test.py | 110 ++++++++++++++++++++++------------ 4 files changed, 175 insertions(+), 74 deletions(-) diff --git a/configure.ac b/configure.ac index cda60405b..9003797a7 100644 --- a/configure.ac +++ b/configure.ac @@ -5,7 +5,7 @@ AC_INIT([rsync],[ ],[https://rsync.samba.org/bug-tracking.html]) AC_C_BIGENDIAN AC_HEADER_DIRENT AC_HEADER_SYS_WAIT -AC_CHECK_HEADERS(sys/fcntl.h sys/select.h fcntl.h sys/time.h sys/unistd.h \ +AC_CHECK_HEADERS(poll.h sys/fcntl.h sys/select.h fcntl.h sys/time.h sys/unistd.h \ unistd.h utime.h compat.h sys/param.h ctype.h sys/wait.h sys/stat.h \ sys/ioctl.h sys/filio.h string.h stdlib.h sys/socket.h sys/mode.h grp.h \ sys/un.h sys/attr.h arpa/inet.h arpa/nameser.h locale.h sys/types.h \ @@ -909,6 +909,12 @@ AC_HAVE_TYPE([struct stat64], [#include # if we can't find strcasecmp, look in -lresolv (for Unixware at least) # +dnl rsync's I/O readiness loops use poll() rather than select() so that a +dnl file descriptor at or above FD_SETSIZE cannot overflow an fd_set (which +dnl is undefined behaviour and could hang the transfer). poll() is in +dnl POSIX.1-2001; fail early and clearly if this target lacks it. +AC_CHECK_FUNCS([poll], , [AC_MSG_ERROR([rsync requires poll(); please report the platform to the rsync developers])]) + AC_CHECK_FUNCS(strcasecmp) if test x"$ac_cv_func_strcasecmp" = x"no"; then AC_CHECK_LIB(resolv, strcasecmp) diff --git a/io.c b/io.c index e9de0b713..0ab1962cd 100644 --- a/io.c +++ b/io.c @@ -33,6 +33,12 @@ #include +/* Readiness bits we act on. poll() can report POLLERR/POLLHUP/POLLNVAL even + * when they were not requested, and POLLPRI stands in for select()'s old + * exception set. */ +#define POLL_RD_BITS (POLLIN | POLLPRI | POLLERR | POLLHUP) +#define POLL_WR_BITS (POLLOUT | POLLERR | POLLHUP) + /** If no timeout is specified then use a 60 second I/O timeout */ #define SELECT_TIMEOUT 60 @@ -115,6 +121,19 @@ static xbuf ff_xb = EMPTY_XBUF; static xbuf iconv_buf = EMPTY_XBUF; #endif static int select_timeout = SELECT_TIMEOUT; + +/* Turn select_timeout (in seconds) into a poll() millisecond count, keeping it + * positive and bounded. A negative count means "wait forever" to poll(), which + * would bypass our keepalives and timeout enforcement entirely. */ +static int poll_timeout_ms(void) +{ + int secs = select_timeout; + + if (secs <= 0 || secs > SELECT_TIMEOUT) + secs = SELECT_TIMEOUT; + return secs * 1000; +} + static int active_filecnt = 0; static OFF_T active_bytecnt = 0; static int first_message = 1; @@ -251,12 +270,12 @@ static size_t safe_read(int fd, char *buf, size_t len) /* We use poll() rather than select() so that a high-numbered fd * (>= FD_SETSIZE) cannot overflow an fd_set bitmap. */ pfd.fd = fd; - pfd.events = POLLIN; + pfd.events = POLLIN | POLLPRI; pfd.revents = 0; - cnt = poll(&pfd, 1, select_timeout * 1000); + cnt = poll(&pfd, 1, poll_timeout_ms()); if (cnt <= 0) { - if (cnt < 0 && errno == EBADF) { + if (cnt < 0 && errno != EINTR && errno != EAGAIN) { rsyserr(FERROR, errno, "safe_read poll failed"); exit_cleanup(RERR_FILEIO); } @@ -264,7 +283,13 @@ static size_t safe_read(int fd, char *buf, size_t len) continue; } - if (pfd.revents) { + /* An invalid fd is reported here rather than via poll()'s return. */ + if (pfd.revents & POLLNVAL) { + rsyserr(FERROR, EBADF, "safe_read poll failed"); + exit_cleanup(RERR_FILEIO); + } + + if (pfd.revents & POLL_RD_BITS) { ssize_t n = read(fd, buf + got, len - got); if (DEBUG_GTE(IO, 2)) { rprintf(FINFO, "[%s] safe_read(%d)=%" SIZE_T_FMT_MOD "d\n", @@ -337,9 +362,9 @@ static void safe_write(int fd, const char *buf, size_t len) pfd.events = POLLOUT; pfd.revents = 0; - cnt = poll(&pfd, 1, select_timeout * 1000); + cnt = poll(&pfd, 1, poll_timeout_ms()); if (cnt <= 0) { - if (cnt < 0 && errno == EBADF) { + if (cnt < 0 && errno != EINTR && errno != EAGAIN) { rsyserr(FERROR, errno, "safe_write poll failed on %s", what_fd_is(fd)); exit_cleanup(RERR_FILEIO); } @@ -348,7 +373,12 @@ static void safe_write(int fd, const char *buf, size_t len) continue; } - if (pfd.revents) { + if (pfd.revents & POLLNVAL) { + rsyserr(FERROR, EBADF, "safe_write poll failed on %s", what_fd_is(fd)); + exit_cleanup(RERR_FILEIO); + } + + if (pfd.revents & POLL_WR_BITS) { n = write(fd, buf, len); if (n < 0) { if (errno == EINTR) @@ -657,7 +687,7 @@ static char *perform_io(size_t needed, int flags) if (iobuf.in_fd >= 0 && iobuf.in.size - iobuf.in.len) { if (!read_batch || batch_fd >= 0) { pfds[npfds].fd = iobuf.in_fd; - pfds[npfds].events = POLLIN; + pfds[npfds].events = POLLIN | POLLPRI; pfds[npfds].revents = 0; in_pollpos = npfds++; } @@ -714,10 +744,18 @@ static char *perform_io(size_t needed, int flags) } else out = NULL; if (out) { - pfds[npfds].fd = iobuf.out_fd; - pfds[npfds].events = POLLOUT; - pfds[npfds].revents = 0; - out_pollpos = npfds++; + /* A direct daemon connection uses one fd for both + * directions; give it a single row with both events + * rather than two rows carrying different masks. */ + if (in_pollpos >= 0 && iobuf.out_fd == iobuf.in_fd) { + pfds[in_pollpos].events |= POLLOUT; + out_pollpos = in_pollpos; + } else { + pfds[npfds].fd = iobuf.out_fd; + pfds[npfds].events = POLLOUT; + pfds[npfds].revents = 0; + out_pollpos = npfds++; + } if (iobuf.out_fd > max_fd) max_fd = iobuf.out_fd; } @@ -757,10 +795,10 @@ static char *perform_io(size_t needed, int flags) poll_timeout = 0; else { extra_flist_sending_enabled = False; - poll_timeout = select_timeout * 1000; + poll_timeout = poll_timeout_ms(); } } else - poll_timeout = select_timeout * 1000; + poll_timeout = poll_timeout_ms(); cnt = poll(pfds, npfds, poll_timeout); @@ -784,7 +822,20 @@ static char *perform_io(size_t needed, int flags) pfds[out_pollpos].revents = 0; } - if (iobuf.in_fd >= 0 && in_pollpos >= 0 && pfds[in_pollpos].revents) { + if (cnt > 0) { + /* poll() reports a bad fd here, not via its return value. */ + int p; + for (p = 0; p < npfds; p++) { + if (pfds[p].revents & POLLNVAL) { + msgs2stderr = 1; + rsyserr(FERROR, EBADF, "perform_io: poll reported an invalid fd"); + exit_cleanup(RERR_SOCKETIO); + } + } + } + + if (iobuf.in_fd >= 0 && in_pollpos >= 0 + && pfds[in_pollpos].revents & POLL_RD_BITS) { size_t len, pos = iobuf.in.pos + iobuf.in.len; ssize_t n; if (pos >= iobuf.in.size) { @@ -833,7 +884,7 @@ static char *perform_io(size_t needed, int flags) exit_cleanup(RERR_TIMEOUT); } - if (out && out_pollpos >= 0 && pfds[out_pollpos].revents) { + if (out && out_pollpos >= 0 && pfds[out_pollpos].revents & POLL_WR_BITS) { size_t len = iobuf.raw_flushing_ends_before ? iobuf.raw_flushing_ends_before - out->pos : out->len; ssize_t n; @@ -894,7 +945,8 @@ static char *perform_io(size_t needed, int flags) wait_for_receiver(); /* generator only */ } - if (ff_forward_fd >= 0 && ff_pollpos >= 0 && pfds[ff_pollpos].revents) { + if (ff_forward_fd >= 0 && ff_pollpos >= 0 + && pfds[ff_pollpos].revents & POLL_RD_BITS) { /* This can potentially flush all output and enable * multiplexed output, so keep this last in the loop * and be sure to not cache anything that would break @@ -1153,8 +1205,11 @@ void io_set_sock_fds(int f_in, int f_out) void set_io_timeout(int secs) { + if (secs < 0) + secs = 0; io_timeout = secs; - allowed_lull = (io_timeout + 1) / 2; + /* Round up without overflowing when secs is near INT_MAX. */ + allowed_lull = secs / 2 + secs % 2; if (!io_timeout || allowed_lull > SELECT_TIMEOUT) select_timeout = SELECT_TIMEOUT; @@ -1559,7 +1614,10 @@ static void read_a_msg(void) goto invalid_msg; val = raw_read_int(); iobuf.in_multiplexed = 1; - if (!io_timeout || io_timeout > val) { + /* Ignore a non-positive value: it would otherwise disable our + * timeout entirely (and a negative one used to be rejected by + * select(), but would make poll() wait forever). */ + if (val > 0 && (!io_timeout || io_timeout > val)) { if (INFO_GTE(MISC, 2)) rprintf(FINFO, "Setting --timeout=%d to match server\n", val); set_io_timeout(val); diff --git a/socket.c b/socket.c index 4ac79ec76..85f86bc5b 100644 --- a/socket.c +++ b/socket.c @@ -25,6 +25,7 @@ * emulate it using the KAME implementation. */ #include "rsync.h" +#include #include "itypes.h" #include "ifuncs.h" #ifdef HAVE_NETINET_IN_SYSTM_H @@ -536,8 +537,8 @@ static void sigchld_handler(UNUSED(int val)) void start_accept_loop(int port, int (*fn)(int, int)) { - fd_set deffds; - int *sp, maxfd, i; + struct pollfd *pfds; + int *sp, nsp, i; #ifdef HAVE_SIGACTION sigact.sa_flags = SA_NOCLDSTOP; @@ -549,8 +550,12 @@ void start_accept_loop(int port, int (*fn)(int, int)) exit_cleanup(RERR_SOCKETIO); /* ready to listen */ - FD_ZERO(&deffds); - for (i = 0, maxfd = -1; sp[i] >= 0; i++) { + for (nsp = 0; sp[nsp] >= 0; nsp++) {} + /* poll() rather than select(): a listening fd at or above FD_SETSIZE + * would overflow an fd_set, which is undefined behaviour (issue #231). */ + if (!(pfds = new_array(struct pollfd, nsp ? nsp : 1))) + out_of_memory("start_accept_loop"); + for (i = 0; sp[i] >= 0; i++) { if (listen(sp[i], lp_listen_backlog()) < 0) { rsyserr(FERROR, errno, "listen() on socket failed"); #ifdef INET6 @@ -560,36 +565,32 @@ void start_accept_loop(int port, int (*fn)(int, int)) #endif exit_cleanup(RERR_SOCKETIO); } - FD_SET(sp[i], &deffds); - if (maxfd < sp[i]) - maxfd = sp[i]; + pfds[i].fd = sp[i]; + pfds[i].events = POLLIN; + pfds[i].revents = 0; } /* now accept incoming connections - forking a new process * for each incoming connection */ while (1) { - fd_set fds; pid_t pid; int fd; struct sockaddr_storage addr; socklen_t addrlen = sizeof addr; - /* close log file before the potentially very long select so + /* close log file before the potentially very long wait so the * file can be trimmed by another process instead of growing * forever */ logfile_close(); -#ifdef FD_COPY - FD_COPY(&deffds, &fds); -#else - fds = deffds; -#endif + for (i = 0; i < nsp; i++) + pfds[i].revents = 0; - if (select(maxfd + 1, &fds, NULL, NULL, NULL) < 1) + if (poll(pfds, nsp, -1) < 1) continue; - for (i = 0, fd = -1; sp[i] >= 0; i++) { - if (FD_ISSET(sp[i], &fds)) { + for (i = 0, fd = -1; i < nsp; i++) { + if (pfds[i].revents & (POLLIN | POLLERR | POLLHUP)) { fd = accept(sp[i], (struct sockaddr *)&addr, &addrlen); break; } diff --git a/testsuite/highfd-hang_test.py b/testsuite/highfd-hang_test.py index d86837e2b..d49db900d 100644 --- a/testsuite/highfd-hang_test.py +++ b/testsuite/highfd-hang_test.py @@ -1,84 +1,120 @@ #!/usr/bin/env python3 -"""Regression test for issue #231: rsync must not hang when its I/O file -descriptors are numbered at/above FD_SETSIZE. +"""Regression test for issue #231: high-numbered I/O descriptors. -rsync's I/O loops in io.c used to call select() with fd_set bitmaps, which can -only represent descriptors below FD_SETSIZE (1024 with glibc). If rsync is -started with many descriptors already open -- e.g. inherited from a parent that -leaked fds -- its own socket/pipe fds get allocated above 1024. FD_SET() and +rsync's I/O readiness loops used to call select() with fd_set bitmaps, which can +only represent descriptors below FD_SETSIZE. If rsync is started with many +descriptors already open -- e.g. inherited from a parent that leaked fds -- its +own socket and pipe fds get allocated at or above that limit. FD_SET() and FD_ISSET() then index past the end of the fixed-size fd_set, which is undefined -behavior: select() reports the fd ready, but FD_ISSET() reads the out-of-bounds -bit as 0, so the read/write never happens and rsync spins at 100% CPU forever -("rsync hangs, 100% of one CPU, no progress"). - -We reproduce that deterministically by opening enough inheritable dummy fds to -push rsync's descriptors past FD_SETSIZE, then running an ordinary transfer -with close_fds=False so the child inherits them. With the poll()-based io.c -the transfer finishes instantly; with the old select()-based code it never -finishes and is caught by the timeout. The cross-over is binary (instant vs. -infinite), so the timeout is a robust signal rather than a timing race. +behaviour: with a plain build select() reports the fd ready while FD_ISSET() +reads the out-of-bounds bit as 0, so the read/write never happens and rsync +spins at 100% CPU forever; with a fortified libc the process instead aborts +("bit out of range 0 - FD_SETSIZE in fd_set"). Either way the transfer never +completes. + +We reproduce that by opening enough inheritable dummy fds to push rsync's +descriptors past FD_SETSIZE, then running an ordinary transfer with +close_fds=False so the child inherits them. With the poll()-based I/O the +transfer finishes immediately; without it, it hangs or aborts. + +FD_SETSIZE is *not* hardcoded: it is 1024 with glibc but 65536 on 64-bit +Solaris, and assuming 1024 there would open too few fds to cross the limit and +pass vacuously. We ask the C library for the real value instead, and skip when +we cannot (no compiler, or the fd limit cannot be raised far enough). """ import os import resource +import shutil import subprocess +import tempfile from rsyncfns import ( FROMDIR, TODIR, rmtree, rsync_argv, test_fail, test_skipped, ) -# select()'s fd_set holds FD_SETSIZE bits; glibc and most libcs use 1024. -FD_SETSIZE = 1024 -NDUMMY = FD_SETSIZE + 80 # force rsync's fds comfortably past the limit -TIMEOUT = 30 # poll build: ~instant; select build: hangs forever +TIMEOUT = 30 # poll() build: ~instant; the bug: hangs (or aborts) + + +def probe_fd_setsize(): + """Ask the C library for FD_SETSIZE rather than assuming a value.""" + cc = os.environ.get('CC') or shutil.which('cc') or shutil.which('gcc') + if not cc: + return None + with tempfile.TemporaryDirectory() as td: + src = os.path.join(td, 'fdss.c') + exe = os.path.join(td, 'fdss') + with open(src, 'w') as f: + f.write('#include \n' + '#include \n' + 'int main(void){ printf("%d\\n", (int)FD_SETSIZE); return 0; }\n') + if subprocess.run([cc, src, '-o', exe], + capture_output=True).returncode != 0: + return None + proc = subprocess.run([exe], capture_output=True, text=True) + if proc.returncode != 0: + return None + try: + return int(proc.stdout.strip()) + except ValueError: + return None + + +fd_setsize = probe_fd_setsize() +if not fd_setsize: + test_skipped("could not determine FD_SETSIZE (no usable C compiler)") + +# Push rsync's descriptors comfortably past the limit. +ndummy = fd_setsize + 80 +want = ndummy + 64 -# We must be able to open enough descriptors to cross FD_SETSIZE. soft, hard = resource.getrlimit(resource.RLIMIT_NOFILE) -want = NDUMMY + 64 if soft < want: if hard != resource.RLIM_INFINITY and hard < want: - test_skipped(f"RLIMIT_NOFILE hard cap {hard} < {want}; cannot place " - "fds above FD_SETSIZE to exercise issue #231") + test_skipped(f"RLIMIT_NOFILE hard cap {hard} < {want}; cannot place fds " + f"above FD_SETSIZE ({fd_setsize}) to exercise issue #231") resource.setrlimit(resource.RLIMIT_NOFILE, (want, hard)) -# A small tree to transfer. rmtree(FROMDIR) rmtree(TODIR) FROMDIR.mkdir(parents=True, exist_ok=True) -payload = {f"f{i}": os.urandom(1000) for i in range(20)} +TODIR.mkdir(parents=True, exist_ok=True) + +payload = {f'f{i}': os.urandom(1000) for i in range(20)} for name, data in payload.items(): (FROMDIR / name).write_bytes(data) -# Occupy the low fd numbers with inheritable dummy descriptors so that the -# rsync child's socket/pipe fds are forced above FD_SETSIZE. Python opens fds -# O_CLOEXEC by default, so mark each inheritable and use close_fds=False. +# Occupy the low fd numbers with inheritable dummy descriptors so the rsync +# child's socket/pipe fds land above FD_SETSIZE. Python opens fds O_CLOEXEC by +# default, so mark each inheritable and use close_fds=False. dummies = [] try: while True: fd = os.open(os.devnull, os.O_RDONLY) os.set_inheritable(fd, True) dummies.append(fd) - if fd >= NDUMMY: + if fd >= ndummy: break argv = rsync_argv('-a', f'{FROMDIR}/', f'{TODIR}/') try: proc = subprocess.run(argv, timeout=TIMEOUT, close_fds=False) except subprocess.TimeoutExpired: - test_fail("rsync hung with high-numbered fds -- select()/fd_set " - "overflow (issue #231 regression)") + test_fail(f"rsync did not finish within {TIMEOUT}s with fds above " + f"FD_SETSIZE ({fd_setsize}) -- select()/fd_set overflow " + "(issue #231 regression)") finally: for fd in dummies: os.close(fd) if proc.returncode != 0: - test_fail(f"rsync exited {proc.returncode}: {' '.join(argv)}") + test_fail(f"rsync exited {proc.returncode} with fds above FD_SETSIZE " + f"({fd_setsize}); a fortified libc aborts on the fd_set overflow " + "(issue #231 regression)") -# The cap only matters if it stayed correct: verify every file transferred. for name, data in payload.items(): - got = (TODIR / name).read_bytes() - if got != data: + if (TODIR / name).read_bytes() != data: test_fail(f"{name} differs after a high-fd transfer") -print(f"issue #231: transfer with fds >= {NDUMMY} (above FD_SETSIZE) " +print(f"issue #231: transfer with fds above FD_SETSIZE ({fd_setsize}) " "completed correctly")