Fix stream_select() failing unpredictably (and crashing on Windows) with many descriptors

[frodeborli]

stream_select() has a correctness bug: once any watched stream's descriptor exceeds FD_SETSIZE, the call emits a warning and returns false — aborting the entire select, including the low-numbered descriptors that would have worked. PHP userland has no control over or visibility into which integer a stream's descriptor gets, so this failure is effectively nondeterministic from the script's perspective: the same code works or fails depending on incidental descriptor numbering. The only escape was recompiling with --enable-fd-setsize.

This fixes the root cause: stream_select() no longer uses a fixed-size fd_set and is no longer bounded by FD_SETSIZE. No recompilation needed.

POSIX: builds a heap bitset sized to the descriptors in use, rounded to the word granularity select() reads/writes.

Windows: the winsock fd_set is a packed SOCKET array bounded by FD_SETSIZE; the socket path now uses a growable fd_set that doubles on demand. php_select()'s exported fd_set * ABI is unchanged.

Also fixes a Windows crash: on the non-socket (pipe/file) path, php_select() pushed each handle into a fixed 64-entry stack array with no bounds check, so more than 64 handles overran it. That path is bounded by WaitForMultipleObjects()'s MAXIMUM_WAIT_OBJECTS (64) limit anyway, so it now returns an error (caller warns and returns false) instead. Fixed inside php_select(), so sapi/cli/php_cli_server.c is covered too.

Scope: stream_select() only. --enable-fd-setsize remains meaningful for socket_select(), mysqlnd, the FPM select backend, and other consumers, which are unchanged.

Tests: ext/standard/tests/streams passes on Linux, FreeBSD, macOS, Windows. gh9590-001/002.phpt updated to assert correct selection past the old limit. Two Windows-only tests added.

---

This is a bug fix, not a behavior change, and I'd like it considered for all supported branches. The differences an existing script can observe — the return value, the removed E_WARNING, and blocking instead of returning immediately — occur only on calls that previously emitted an E_WARNING and returned false, i.e. calls PHP was already reporting as failures. Any call that was selecting successfully (all descriptors below FD_SETSIZE) behaves identically. There is no warning-free, working call whose behavior changes. The Windows >64-handle case is an outright crash fix. On that basis I believe it qualifies for backport to the active stable branches rather than master only.

The patch does not apply cleanly to the older branches as-is (stream_select() was refactored on master, and the Windows pipe handling differs across versions), so I'm happy to provide adapted patches per branch if backporting is wanted.

[frodeborli]

The testing complained that I had left behind two functions which I didn't dare to remove in case somebody else was using them. I have updated the pr.

[frodeborli]

I'm not sure why the tests timed out or failed; it does not seem to be related to my patch.

[arnaud-lb]

Big +1 on the general idea. This fixes a long standing issue with stream_select() not supporting file descriptors above 1024.

@bukka @devnexen wdyt?

Maybe we can remove the --enable-fd-setsize configure flag as well. I believe it's a no-op in glibc for some time now.

[mvorisek]

Can this be tested from useland when more than 1024 file descriptors are created/used?

[arnaud-lb]

Also when there is less than 1024, but at least one has a number > 1024

[frodeborli]

@mvorisek I used this code to test:

<?php
// Create a large number of file descriptors
$files = [];
$r = $w = $e = [];

for ($i = 0; $i < 20000; $i++) {
    $fileName = tempnam(sys_get_temp_dir(), 'testfile');
    $fp = fopen($fileName, 'r');
    if ($fp === false) {
        die("Failed to open file: $fileName\n");
    }
    $files[] = ['name' => $fileName, 'handle' => $fp];
    $r[] = $fp;
}

// Check the number of file descriptors
$res = stream_select($r, $w, $e, 0, 0);
var_dump($res, $r);
// Clean up
foreach ($files as $file) {
    fclose($file['handle']);
    unlink($file['name']);
}

[TimWolla]

A function doing dup2() can probably be added to zend_test, allowing to zend_test_dup2(1, 9999).

[frodeborli]

@TimWolla Is zend_test a mechanism to add special functions to the PHP runtime during testing? I think it would be nice to have zend_test_get_fd($resource), but perhaps that already exists?

[TimWolla]

@frodeborli

Is zend_test a mechanism to add special functions to the PHP runtime during testing?

Yes, exactly. It is meant to provide helpers to test stuff that would otherwise be untestable. You can find it in https://github.com/php/php-src/tree/master/ext/zend_test and can enable it at build time using --enable-zend-test.

[frodeborli]

I will look over the PR thoroughly later today or tomorrow morning.

Can anybody tell me how to build this in a stricter mode? Several commits could have been avoided by me if only my environment would let me know about unused functions etc, instead of waiting for the github CI process to check for me.

[devnexen]

I will look over the PR thoroughly later today or tomorrow morning.

Can anybody tell me how to build this in a stricter mode? Several commits could have been avoided by me if only my environment would let me know about unused functions etc, instead of waiting for the github CI process to check for me.

Speaking of CI, you can get inspiration from it if you like.

[ramsey]

The only feedback I would give is to please update your editor to respect the .editorconfig file provided with PHP. https://editorconfig.org

It looks like you've converted a lot of tab characters to spaces, so the diff is much bigger than it should be.

[frodeborli]

I am having trouble because stream wrappers emit warnings without respecting the silent cast argument of php_stream_cast...

So a phpt test does not generate the exact same output anymore after my patch.

[arnaud-lb]

The cause of the extra warnings in ext/standard/tests/file/userstreams_002.phpt appear to be that max_fd is never equal to -1 here.

[frodeborli]

Now it seems all tests succeeded, except Windows 64 bit. Not sure how to proceed. I think the php_select macro refers to a win32 implementation which is meant to be compatible with select() in Linux and therefore should work.

[withinboredom]

Are we missing FD_BIGSET_FREE before returning?

[frodeborli]

I'm not sure where you mean; I think FD_BIGSET_FREE is everywhere it is needed now, but there was a few places they were missing before the last push.

[withinboredom]

We've already called FD_BIGSET_ZERO by this point, so the memory is allocated. On this line, we are about to return without freeing that memory; thus I think we need to free it.

That being said, I'm not quite sure how we would get here.

[frodeborli]

in any case it would fall back to the current behavior Sendt fra Outlook for iOS<https://aka.ms/o0ukef>

________________________________ Fra: Rob Landers ***@***.***> Sendt: Friday, June 7, 2024 4:36:36 PM Til: php/php-src ***@***.***> Kopi: Frode Børli ***@***.***>; Author ***@***.***> Emne: Re: [php/php-src] Updated the stream_select() function so that it supports bigger file … (PR #14452) @withinboredom commented on this pull request.

________________________________ In ext/standard/streamsfuncs.c<#14452 (comment)>:

+ FD_BIGSET_ZERO(&rfds, max_fds);

+ FD_BIGSET_ZERO(&wfds, max_fds); + FD_BIGSET_ZERO(&efds, max_fds); See man sysconf<https://www.man7.org/linux/man-pages/man3/sysconf.3.html>: The return value of sysconf() is one of the following: On error, -1 is returned and errno<https://www.man7.org/linux/man-pages/man3/errno.3.html> is set to indicate the error (for example, EINVAL, indicating that name is invalid). If name corresponds to a maximum or minimum limit, and that limit is indeterminate, -1 is returned and errno<https://www.man7.org/linux/man-pages/man3/errno.3.html> is not changed. (To distinguish an indeterminate limit from an error, set errno<https://www.man7.org/linux/man-pages/man3/errno.3.html> to zero before the call, and then check whether errno<https://www.man7.org/linux/man-pages/man3/errno.3.html> is nonzero when -1 is returned.) If name corresponds to an option, a positive value is returned if the option is supported, and -1 is returned if the option is not supported. Otherwise, the current value of the option or limit is returned. This value will not be more restrictive than the corresponding value that was described to the application in <unistd.h> or <limits.h> when the application was compiled. So, theoretically, if uname -n unlimited were possible, you could also receive a -1 too. AFAIK, it isn't possible to set the open file limit to unlimited though. (I could be wrong, but I'm 95% sure). — Reply to this email directly, view it on GitHub<#14452 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AARRLUDB2NEIVS4KIIK75JTZGHAPJAVCNFSM6AAAAABIWFGFUGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCMBUG42TENJUGQ>. You are receiving this because you authored the thread.Message ID: ***@***.***>

[frodeborli]

I think it will be very rare with file descriptors above 131k, at least for the next couple of years. Perhaps the best solution is to reallocate on the fly if we see fds above 131k and use a single pass. 131k is extremely high for a single process so I think it will be rare to encounter it.

[arnaud-lb]

I think we can use max_fd to directly compute the largest size?

[frodeborli]

Something like:

while (max_fd > rfds.size) fd_bigset_double_size(&rfds);

I was just not confident about possibly off by one error here...

[arnaud-lb]

We can use the same condition as in FD_BIGSET_ENSURE_CAPACITY(), or pre-compute the largest size with:

size_t num_fds = max_fd + 1;
size_t largest = (num_fds + 7) / 8;

[frodeborli]

@arnaud-lb Thank you for your detailed review. I'm merging the latest php-src and pushing an updated PR with all your suggestions.

I was looking into Windows which apparently only has FD_SETSIZE 64 by default, and it uses an entirely different fd_set structure. Not sure how this is handled inside PHP and if my approach breaks it? I've not got a Windows C development environment, but I can look into it if you can give some hints perhaps?

[arnaud-lb]

Oh right, it looks like this will break on Windows. We use WinSock select() according to this comment, so fd_set is as defined here: https://learn.microsoft.com/en-us/windows/win32/api/winsock/ns-winsock-fd_set on Windows. It represents an array of handles rather than a bitset, and the limit is the number of watched fds rather than the max fd value.

We can use the same strategy as for other platforms, but use the fd count to decide whether to grow the set. We have to define the FD_BIGSET_ macros differently on windows:

#ifdef PHP_WIN32

typedef struct {
    uint32_t capacity;
    fd_set set;
} fd_bigset;

#define FD_BIGSET_ENSURE_CAPACITY(fd, set) \
    if (UNEXPECTED(fd.set.count == fd.capacity)) { \
        fd_bigset_double_size(set); \
    }

// other FD_BIGSET macros for windows

#else 

// FD_BIGSET macros for non-windows

#endif

However there is a (non-enforced) hard limit of MAXIMUM_WAIT_OBJECTS (64) non-socket fds in win32/select.c. I would suggest to return a failure from php_select() when there are more than MAXIMUM_WAIT_OBJECTS non-socket fds.

php_select() copies fd_set structs by assignment (fd_set sock_read, aread; ...; aread = sock_read;). This will not work with custom sized sets, so we need to replace these with memcpy()s.

It would make sense to do part of this work later in a separate PR, as long as this PR doesn't break windows. E.g. don't support more than FD_SETSIZE fds for now on Windows.

I've not got a Windows C development environment, but I can look into it if you can give some hints perhaps?

I sometimes use a Windows VM from https://developer.microsoft.com/en-us/windows/downloads/virtual-machines/ (The VirtualBox ones work well. If you use virt-manager you may be able to convert them to KVM but that's not trivial.). This page describes how to build on windows: https://wiki.php.net/internals/windows/stepbystepbuild_sdk_2

[bukka]

I will take a proper look later but could you please in the meantime fix the coding style.

[bukka]

btw I have a real Windows env so I can test it then too.

[frodeborli]

The WINDOWS_X64_ZTS failure here is unrelated to this change. The build succeeds (vs18, /WX), and the only failing test out of ~23.7k is ext/standard/tests/file/GHSA-9f67-6fw4-hpfp-win32.phpt, which fails during its own cleanup (Warning: rmdir(): Directory not empty). That test is currently failing on master too — e.g. the Windows job in run 35732538919 — so it's a pre-existing issue independent of this PR. The stream_select() changes compile cleanly and the new stream_select_win32_* tests pass.

[frodeborli]

Excited to pick this back up — apologies for the long silence since last year, some personal circumstances kept me away from this for a while. Good to be back on it.

Rebased and pushed 2 days ago (test run 35733334346) — CI is green across Linux (x64/x32, ASAN/DEBUG), Alpine, macOS ARM64, FreeBSD, and CircleCI arm. The one red job, WINDOWS_X64_ZTS, is ghsa-9f67-6fw4-hpfp-win32.phpt failing during its own cleanup (rmdir(): Directory not empty) — unrelated to this change and also currently red on master itself (e.g. run 35732538919). The stream_select_win32_* tests this PR adds pass.

Re the ABI break label: I believe this is the path-based labeler matching main/*.h (this PR touches main/php_network.h), not a reflection of an actual break — the diff to that file is a pure addition (php_growable_fd_set and its static-inline helpers), nothing existing is resized, relocated, or given a different signature. php_select()'s exported fd_set * signature is unchanged, as noted in the PR description. Happy to be corrected if I'm missing something.

@bukka — thanks for taking another look. @arnaud-lb — tagging you too since you did most of the substantive review back in 2024 (the FD_BIGSET design, the Windows fd_set-as-array constraints) and the Windows growable-fd_set approach here follows the shape you suggested. Would appreciate your eyes on the final form if you have time.

[arnaud-lb]

@frodeborli Is this still worth the effort now that we have https://wiki.php.net/rfc/poll_api to replace stream_select()?

[frodeborli]

Thanks @arnaud-lb. I checked master: the poll API added Io\Poll as a separate API and left stream_select() untouched — it's still the select()-based code with fixed-size fd_set rfds, wfds, efds on the stack. So this isn't superseded; the bug is still live in master.

And it is a real bug, not just a limit. Past FD_SETSIZE, FD_SET() writes past those stack fd_sets — stack corruption, i.e. a crash (and worse than a clean failure). Because fd numbers grow with the number of open connections, a server that accepts enough of them can be pushed over the edge, so this is a plausible DoS in any app using stream_select() at scale. Io\Poll existing doesn't help any of that already-written code — including the default pure-PHP event loops in ReactPHP, Revolt/amphp and phasync, which fall back to stream_select() when ext-event/ev/uv aren't installed (the common case).

I'd also argue the two are complementary rather than redundant. Io\Poll (epoll/kqueue) wins for large numbers of long-lived connections; select()/poll() semantics rebuild the set per call with no per-fd registration syscall, which is cheaper under high connection churn (typical request/response servers). Keeping stream_select() correct keeps that regime viable.

The fix is implemented and green across platforms, and the Windows growable fd_set follows the design you outlined in 2024. Personally, I really want this feature in PHP, even though Io\Poll is coming.

[frodeborli]

@bukka The tabs-vs-spaces issue from your review is fixed; the diff now uses tabs throughout. I've also added tests for descriptors beyond FD_SETSIZE in the write and except sets, on both POSIX (gh9590-003) and Windows (stream_select_win32_multi_set, verified on the Windows CI build). Until now only the read set was covered.

Since this is a bug fix (stack corruption past FD_SETSIZE), I think it's still needed alongside Io\Poll, and it doesn't conflict with it. I looked at implementing stream_select() on top of epoll instead, but it's a poor fit: stream_select() is stateless, since every call passes fresh arrays. So each call would need epoll_create1(), one epoll_ctl() per descriptor, epoll_wait() and close(). That's n+2 syscalls where select() needs one. poll() changes the semantics in small ways: select() counts a descriptor once per set it's ready in while poll() counts it once, an invalid fd becomes POLLNVAL instead of EBADF, and timeouts drop from microseconds to milliseconds. I ran into exactly those differences in a poll()-based stream_select() in an extension. Keeping select() with a growable fd_set is the smallest change that fixes the bug while keeping stream_select()'s behaviour identical. Io\Poll remains the right tool for new code.

Could you take another look when you have time?