io: use poll() instead of select() to avoid an FD_SETSIZE hang (fixes #231)

[dr-who]

Problem (#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 — inherited from a parent that leaked fds, a high ulimit -n, or a busy daemon — its own socket/pipe fds get allocated at or above 1024. FD_SET()/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 with no progress.

This is the long-standing "rsync hangs at 100% CPU" report, and it explains the MemorySanitizer use-of-uninitialized-value in perform_io in the issue thread (the OOB FD_ISSET read) and Wayne's own hypothesis there ("maybe iobuf.in is greater than the default FD_SETSIZE"). The -vvv correlation was a red herring — the message-backlog deadlock is separately mitigated by the dynamic iobuf.msg growth.

Deterministic reproduction

Pre-open ~1100 inheritable fds so rsync's descriptors land above FD_SETSIZE, then run any transfer:

• < ~1015 fds: fine.
• ≥ ~1015 fds (socket fd crosses 1024): hangs. strace shows pselect6(1106, [1105], …) = 1 returning "ready" instantly in a tight loop; one process pegged at 100% CPU in state R; 0 files transferred.

Fix

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.

• Not slower here. rsync only ever waits on a handful of fds (at most three in perform_io: in_fd, out_fd, the files-from forward fd). The "poll is slower than select" effect only appears when watching thousands of descriptors; at N≤3 poll() is equal-to or faster than select() (it skips the FD_ZERO bitmap clears). epoll/kqueue would be more code, unportable, and actually slower for such a tiny, changing fd set.
• Behavior-preserving. The same max_fd bookkeeping decides when there's nothing to wait on; each FD_ISSET maps to the matching pollfd.revents; the timeout is unchanged (now in milliseconds). The lone remaining select(0, …) is a pure timed sleep with no fds and is left as-is.

Verified: with the fix, transfers complete correctly with 1100 and 5000 pre-opened fds (and the data matches); a normal low-fd transfer is unchanged.

Test

testsuite/highfd-hang_test.py opens enough inheritable dummy fds to push rsync's descriptors past FD_SETSIZE, then runs an ordinary transfer with close_fds=False. The hang is an infinite spin, so the instant-pass / never-finish cross-over is binary rather than a timing race. It hangs (caught by a timeout) on the select() code and passes instantly with poll(), and also verifies the transferred files are correct. It skips cleanly if RLIMIT_NOFILE can't be raised above FD_SETSIZE.

Full suite: 107 passed, 6 skipped, 0 failed.

[tridge]

Automated review — generated by OpenAI Codex. Findings were spot-checked
against the code but have not been hand-verified in full; please treat them as
review input rather than a final verdict.

Thanks for tackling #231 — moving the I/O readiness loops off select()/fd_set
is the right direction, and the transfer-path conversion is largely sound: the
fixed pollfd[3] set can't overflow, the message-before-data priority and
partial-progress / short read-write / EINTR / EAGAIN semantics are preserved,
it builds cleanly, and the new highfd-hang test is RED on master (aborts with
a fortify "bit out of range … FD_SETSIZE") and GREEN with the patch.

A few things should be addressed before this lands, though — two of them
significant:

1. High — a negative I/O timeout becomes an infinite poll() wait

set_io_timeout() can yield a negative select_timeout (e.g. secs == -3 →
allowed_lull == -1 → select_timeout == -1), and MSG_IO_TIMEOUT accepts an
arbitrary signed peer value (it calls set_io_timeout(val) whenever
val < io_timeout). All three waits now pass select_timeout * 1000 to
poll(); a negative millisecond count means wait forever on Linux, which
bypasses keepalives and timeout enforcement entirely. Previously select()
rejected the negative timeval with EINVAL, so the loop and check_timeout()
still ran. A hostile or buggy peer can therefore stall the other side
indefinitely. ((io_timeout + 1) is also a signed-overflow path for
INT_MAX.) Suggest rejecting peer values <= 0, computing allowed_lull
overflow-safe (secs / 2 + secs % 2), and clamping the final poll timeout to
0..60000.

2. High — the same FD_SETSIZE bug remains in the daemon accept loop

start_accept_loop() in socket.c still stores listening sockets in an
fd_set and calls select(). If rsync --daemon itself starts with enough
descriptors open, open_socket_in() returns listener fds >= FD_SETSIZE and the
same out-of-bounds FD_SET() occurs at daemon startup. This PR fixes high
accepted-connection fds once the transfer reaches perform_io, but not the
listener path, and the new test only runs a local transfer so it doesn't cover
it. Either convert the accept loop too, or narrow the PR's claim and track the
remaining path as a companion fix.

3. Medium — revents != 0 isn't the requested-readiness test; invalid-fd handling drifts

safe_read, safe_write, and the three multiplexer checks treat any non-zero
revents as ordinary readiness. poll() reports POLLERR/POLLHUP/POLLNVAL
even when unrequested, and an invalid fd yields a successful poll with
POLLNVAL — not poll() == -1 && errno == EBADF — so the new EBADF branches
are effectively dead. Consequences: safe_read/safe_write shift from
RERR_FILEIO at the readiness call to RERR_STREAMIO during the I/O attempt;
perform_io may service other ready fds before noticing the bad one; and an
invalid ff_forward_fd is passed to forward_filesfrom_data(), where EBADF is
read as EOF and an EOF marker is sent instead of failing. Suggest handling
POLLNVAL explicitly and using role-specific masks
(POLLIN|POLLERR|POLLHUP, POLLOUT|POLLERR|POLLHUP). The old exception sets
(POLLPRI) are also silently dropped, so the "behavior-preserving" claim isn't
exact.

4. Medium — in_fd == out_fd is entered twice

Direct daemon connections use one fd bidirectionally (client_run(fd, fd, …)),
and the patch adds two pollfd rows for it (one POLLIN, one POLLOUT). This
is fine on current systems, but Cygwin through 3.3.5 had a bug that copied
readiness between duplicate poll entries with different masks (fixed in 3.3.6);
combined with the any-revents test, a permanently-writable socket can spuriously
trigger the read path and cause EAGAIN loops. Merging the identical fd into one
row with OR-ed events (or role-specific revents masks) avoids this and also
satisfies "no fd double-counted."

5. Medium — poll() is now an undeclared platform requirement

The patch adds <poll.h> but configure.ac checks neither the header nor the
function. Modern Cygwin/Solaris have poll(), but rsync still carries support for
older/proprietary systems, and poll() is absent on some targets (e.g. native
MinGW/MSVC, HPE NonStop) with behavioral quirks on some older AIX/macOS special
files. Suggest adding AC_CHECK_HEADERS([poll.h]) / AC_CHECK_FUNCS([poll]) and
either failing configure with a clear minimum-platform message, using a
replacement, or keeping a bounded select() fallback that rejects
fd >= FD_SETSIZE rather than invoking UB.

6. Test — valid on Linux but not portable as written

highfd-hang_test.py hardcodes FD_SETSIZE = 1024. On 64-bit Solaris that value
is 65536, so opening up to fd 1104 doesn't cross the limit and the test passes
vacuously (false green). Obtaining the build's actual FD_SETSIZE from a small
compiled helper would make it robust; it's also worth describing the pre-fix
result as "hang or abort due to UB," since on a fortified glibc master aborts
rather than hangs. The fd-construction approach itself (opening continuously,
close_fds=False) is sound on Linux.

Happy to see this land once the timeout handling (#1) and the readiness/POLLNVAL
semantics (#3) are sorted; #2 is a scope call worth deciding explicitly.

[tridge]

thanks for looking at this. I agree we should move to poll. I did a codex review which found a few issues, see comment above. I'd like to get this into 3.5.0 if I can, sorry it took me a while to get to this.

[dr-who]

Thanks — that's a genuinely useful review, and all six were real. Pushed as a follow-up commit (io/socket: address review of the poll() conversion), rebased onto current master. Point by point:

1 (High) — negative timeout became an infinite poll() wait. Confirmed, and the analysis is right: select() used to reject the negative timeval with EINVAL so the loop and check_timeout() still ran, whereas poll() treats it as wait-forever. Fixed on all the paths you identified:

• set_io_timeout() clamps a negative argument to 0 and computes the lull overflow-safe as secs / 2 + secs % 2 (the (io_timeout + 1) overflow you spotted).
• MSG_IO_TIMEOUT now ignores a non-positive peer value rather than applying it — a peer shouldn't be able to disable our timeout.
• All three waits go through a new poll_timeout_ms() that keeps the count positive and bounded (0 < secs <= SELECT_TIMEOUT), so the belt-and-braces clamp is there even if some future path produces a bad value.

2 (High) — same bug in the daemon accept loop. You're right, and I'd wrongly scoped the PR. Rather than narrow the claim I converted start_accept_loop() too. I verified it rather than assuming: starting a daemon with ~1100 descriptors already open (so the listener lands above FD_SETSIZE), a transfer through it returns nothing with the old code and succeeds with this change.

3 (Medium) — revents != 0 isn't the readiness test. Agreed on all counts, including that the EBADF branches were effectively dead and that an invalid ff_forward_fd would reach forward_filesfrom_data() and be read as EOF. Now using role-specific masks (POLL_RD_BITS = POLLIN|POLLPRI|POLLERR|POLLHUP, POLL_WR_BITS = POLLOUT|POLLERR|POLLHUP), with POLLNVAL handled explicitly in all three loops. POLLPRI is requested so select()'s old exception set isn't silently dropped — and you're right that my "behavior-preserving" wording was not exact; it shouldn't have claimed that.

4 (Medium) — in_fd == out_fd entered twice. Fixed: a bidirectional fd now occupies a single pollfd row with OR-ed events instead of two rows with differing masks, which sidesteps the Cygwin < 3.3.6 duplicate-entry behaviour as well as satisfying "no fd double-counted."

5 (Medium) — undeclared platform requirement. Added AC_CHECK_HEADERS([poll.h]) and AC_CHECK_FUNCS([poll]). This one is a judgement call I'd like your steer on: I went with failing configure with an explicit message, on the grounds that the code now hard-requires poll() and a silent mis-build is worse. If you'd rather keep older/proprietary targets building, I'm happy to switch to a bounded select() fallback that refuses fd >= FD_SETSIZE instead of invoking UB — say the word and I'll respin.

6 (Test) — not portable as written. Good catch on the vacuous green; hardcoding 1024 would indeed have opened too few fds on 64-bit Solaris. The test now obtains the real FD_SETSIZE from a small compiled probe (skipping cleanly if there's no usable compiler) and sizes the fd run from that. The description now also covers the fortified-glibc case where the pre-fix result is an abort rather than a hang.

Full suite: 108 passed, 0 failed. The high-fd test still fails on master and passes with the change.