From b614b4a69ae7bab13c39af2f4a01dea846dfa307 Mon Sep 17 00:00:00 2001 From: "Christoph M. Becker" Date: Sun, 24 Nov 2024 19:06:01 +0100 Subject: [PATCH] GH-16889: stream_select() timeout useless for pipes on Windows Pipes are blocking on Windows, but `php_select()` always returns them as ready for read/write. This renders the `stream_select()` timeout useless, what can cause a following read to block for a very long time. While there is no general fix (and least not within reach for a stable version), we can at least cater to the important case of read pipes by peeking the pipe to check whether data is available. If there is none, we do not add the handle to the read set. We need to fix a couple of tests cases: * bug60692.phpt and bug64770.phpt assume that at least the stdin and stdout pipes are always selected as readable, and that the select call will not change their order. We're being more defensive now. * the potentials warnings of bug49936_win32.phpt need to be suppressed, like it has been done earlier for the POSIX variant of this test case[1]. Possibly this test case should be dropped altogether[2]. [1] [2] Closes GH-16917. --- NEWS | 4 +++ .../tests/streams/bug49936_win32.phpt | 11 ++------ ext/standard/tests/streams/bug60602.phpt | 5 ++-- ext/standard/tests/streams/bug64770.phpt | 5 ++-- ext/standard/tests/streams/gh16889.phpt | 26 +++++++++++++++++++ win32/select.c | 14 +++++++--- 6 files changed, 49 insertions(+), 16 deletions(-) create mode 100644 ext/standard/tests/streams/gh16889.phpt diff --git a/NEWS b/NEWS index 8663f30c99dab..171a437f53c06 100644 --- a/NEWS +++ b/NEWS @@ -74,6 +74,10 @@ PHP NEWS . Fixed crypt() tests on musl when using --with-external-libcrypt (Michael Orlitzky). +- Streams: + . Fixed bug GH-16889 (stream_select() timeout useless for pipes on Windows). + (cmb) + - Windows: . Fixed bug GH-10992 (Improper long path support for relative paths). (cmb, nielsdos) diff --git a/ext/standard/tests/streams/bug49936_win32.phpt b/ext/standard/tests/streams/bug49936_win32.phpt index 6984d61b36aef..30189c3d589fe 100644 --- a/ext/standard/tests/streams/bug49936_win32.phpt +++ b/ext/standard/tests/streams/bug49936_win32.phpt @@ -12,17 +12,10 @@ default_socket_timeout=2 $dir = 'ftp://your:self@localhost/'; -var_dump(opendir($dir)); -var_dump(opendir($dir)); +var_dump(@opendir($dir)); +var_dump(@opendir($dir)); ?> --EXPECTF-- -Warning: opendir(): connect() failed: %s in %s on line %d - -Warning: opendir(ftp://...@localhost/): Failed to open directory: operation failed in %s on line %d bool(false) - -Warning: opendir(): connect() failed: %s in %s on line %d - -Warning: opendir(ftp://...@localhost/): Failed to open directory: operation failed in %s on line %d bool(false) diff --git a/ext/standard/tests/streams/bug60602.phpt b/ext/standard/tests/streams/bug60602.phpt index b97f6f877a42d..b3795414e4f19 100644 --- a/ext/standard/tests/streams/bug60602.phpt +++ b/ext/standard/tests/streams/bug60602.phpt @@ -18,8 +18,9 @@ if (is_resource($p)) { $data = ''; while (1) { + $r = [$pipes[1]]; $w = $e = NULL; - $n = stream_select($pipes, $w, $e, 300); + $n = stream_select($r, $w, $e, 300); if ($n === false) { echo "no streams \n"; @@ -29,7 +30,7 @@ if (is_resource($p)) { proc_terminate($p, 9); break; } else if ($n > 0) { - $line = fread($pipes[1], 8192); + $line = fread($r[0], 8192); if (strlen($line) == 0) { /* EOF */ break; diff --git a/ext/standard/tests/streams/bug64770.phpt b/ext/standard/tests/streams/bug64770.phpt index ba39bb5f84f68..46c1ffa3061d8 100644 --- a/ext/standard/tests/streams/bug64770.phpt +++ b/ext/standard/tests/streams/bug64770.phpt @@ -18,8 +18,9 @@ if (is_resource($p)) { $data = ''; while (1) { + $r = [$pipes[1]]; $w = $e = NULL; - $n = stream_select($pipes, $w, $e, 300); + $n = stream_select($r, $w, $e, 300); if ($n === false) { echo "no streams \n"; @@ -29,7 +30,7 @@ if (is_resource($p)) { proc_terminate($p, 9); break; } else if ($n > 0) { - $line = fread($pipes[1], 8192); + $line = fread($r[0], 8192); if (strlen($line) == 0) { /* EOF */ break; diff --git a/ext/standard/tests/streams/gh16889.phpt b/ext/standard/tests/streams/gh16889.phpt new file mode 100644 index 0000000000000..2788db52bbcdf --- /dev/null +++ b/ext/standard/tests/streams/gh16889.phpt @@ -0,0 +1,26 @@ +--TEST-- +GH-16889 (stream_select() timeout useless for pipes on Windows) +--FILE-- + +--EXPECT-- +bool(true) diff --git a/win32/select.c b/win32/select.c index f443325cf0456..76d5ef1ee5be9 100644 --- a/win32/select.c +++ b/win32/select.c @@ -21,7 +21,7 @@ * - If you supply only sockets, this simply passes through to winsock select(). * - If you supply file handles, there is no way to distinguish between * ready for read/write or OOB, so any set in which the handle is found will - * be marked as ready. + * be marked as ready. Pipes will be checked if they are ready for read, though. * - If you supply a mixture of handles and sockets, the system will interleave * calls between select() and WaitForMultipleObjects(). The time slicing may * cause this function call to take up to 100 ms longer than you specified. @@ -135,15 +135,23 @@ PHPAPI int php_select(php_socket_t max_fd, fd_set *rfds, fd_set *wfds, fd_set *e for (i = 0; i < n_handles; i++) { if (WAIT_OBJECT_0 == WaitForSingleObject(handles[i], 0)) { if (SAFE_FD_ISSET(handle_slot_to_fd[i], rfds)) { - FD_SET((uint32_t)handle_slot_to_fd[i], &aread); + DWORD avail_read = 0; + if (GetFileType(handles[i]) != FILE_TYPE_PIPE + || !PeekNamedPipe(handles[i], NULL, 0, NULL, &avail_read, NULL) + || avail_read > 0 + ) { + FD_SET((uint32_t)handle_slot_to_fd[i], &aread); + retcode++; + } } if (SAFE_FD_ISSET(handle_slot_to_fd[i], wfds)) { FD_SET((uint32_t)handle_slot_to_fd[i], &awrite); + retcode++; } if (SAFE_FD_ISSET(handle_slot_to_fd[i], efds)) { FD_SET((uint32_t)handle_slot_to_fd[i], &aexcept); + retcode++; } - retcode++; } } }