Skip to content

Fix stream_select() failing unpredictably with many or large descriptors (backport of #14452 to PHP-8.4) - #25072

Open
frodeborli wants to merge 1 commit into
php:PHP-8.4from
frodeborli:stream-select-fd-setsize-8.4
Open

frodeborli wants to merge 1 commit into
php:PHP-8.4from
frodeborli:stream-select-fd-setsize-8.4

Conversation

@frodeborli

Copy link
Copy Markdown

Backport of #14452 (23bb1c9) to PHP-8.4, since it is a bug fix.

What it fixes

  • POSIX: once any watched stream's descriptor is at or above FD_SETSIZE, stream_select() emits the "You MUST recompile PHP with a larger value of FD_SETSIZE" warning and the whole call returns false. A server with many open connections can't select on them without rebuilding PHP. (In the original PR I described this as stack corruption on POSIX; that was wrong. PHP_SAFE_FD_SET guards it, so it fails cleanly.)
  • Windows: sockets beyond winsock's FD_SETSIZE are silently dropped, and more than 64 pipe or file handles overflow the fixed handles[MAXIMUM_WAIT_OBJECTS] stack array in win32/select.c. That is a stack buffer overrun; it now fails with a warning instead.

Differences from the master commit

  • stream_select() keeps 8.4's signature and code. Master's $context parameter, the stream error operation API and the HashTable * refactor are not part of this backport.
  • win32/select.c: master's version builds on "Improve fix for stream_select() timeout useless for pipes on Windows #16889" (6972612), which is not in PHP-8.4. I left that out: no PeekNamedPipe() readiness check, no usleep(), and a ready pipe or file handle is still counted once, as 8.4 does today. The rest of the Windows change (growable socket sets, a single pass over the input arrays, de-duplication, and the bounds check on the handle array) is the same as in master.
  • main/php_network.h only gains the new php_growable_fd_set type and static inline helpers. No existing macro, struct or function signature changes, so already-built extensions are unaffected. php_select() keeps its signature.
  • UPGRADING is not touched. The NEWS entries are under 8.4.28.

Merging up

The 8.5 resolution is on frodeborli:stream-select-fd-setsize-8.5 (d0412de). There win32/select.c applies exactly as in master, since 8.5 has the GH-16889 change; only stream_select() drops the master-only $context/error API lines. Master already has the fix.

Testing

Linux debug builds of 8.4 and 8.5: all stream tests pass, including gh9590-001/002/003, which go past FD_SETSIZE in the read, write and except sets. I haven't been able to build on Windows for this branch, so the stream_select_win32_* tests rely on CI here; the 8.4 win32/select.c is the part that differs from what was reviewed in #14452.

I'm not proposing this for 8.2/8.3: on POSIX it's a clean failure, and the Windows overflow needs the script itself to pass more than 64 pipes or files, so I don't think it qualifies as a security fix.

…ors (php#14452)

Updated stream_select() so that its not limited by FD_SETSIZE anymore
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant