Repository navigation
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
Open
frodeborli wants to merge 1 commit into
frodeborli wants to merge 1 commit into
Conversation
…ors (php#14452) Updated stream_select() so that its not limited by FD_SETSIZE anymore
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Backport of #14452 (23bb1c9) to PHP-8.4, since it is a bug fix.
What it fixes
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_SETguards it, so it fails cleanly.)FD_SETSIZEare silently dropped, and more than 64 pipe or file handles overflow the fixedhandles[MAXIMUM_WAIT_OBJECTS]stack array inwin32/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$contextparameter, the stream error operation API and theHashTable *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: noPeekNamedPipe()readiness check, nousleep(), 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.honly gains the newphp_growable_fd_settype and static inline helpers. No existing macro, struct or function signature changes, so already-built extensions are unaffected.php_select()keeps its signature.UPGRADINGis not touched. TheNEWSentries are under 8.4.28.Merging up
The 8.5 resolution is on
frodeborli:stream-select-fd-setsize-8.5(d0412de). Therewin32/select.capplies exactly as in master, since 8.5 has the GH-16889 change; onlystream_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_SETSIZEin the read, write and except sets. I haven't been able to build on Windows for this branch, so thestream_select_win32_*tests rely on CI here; the 8.4win32/select.cis 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.