From 6f42d900ded62683465eecf3b662e4361c81fd81 Mon Sep 17 00:00:00 2001 From: "Christoph M. Becker" Date: Sun, 24 Nov 2024 15:45:52 +0100 Subject: [PATCH 1/4] 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. --- ext/standard/tests/streams/gh16889.phpt | 26 +++++++++++++++++++++++++ win32/select.c | 11 ++++++++--- 2 files changed, 34 insertions(+), 3 deletions(-) create mode 100644 ext/standard/tests/streams/gh16889.phpt 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 6bdd4b7018a18..0f14a8a25ef53 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,20 @@ 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 (!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++; } } } From bab29d55114d0e98d31f45b3f38d3cde9bb1a8f4 Mon Sep 17 00:00:00 2001 From: "Christoph M. Becker" Date: Sun, 24 Nov 2024 16:59:45 +0100 Subject: [PATCH 2/4] Fix bug64770.phpt The test assumes 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. --- ext/standard/tests/streams/bug64770.phpt | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) 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; From 2a034d33f776ea8100db28da486546abddb98fc9 Mon Sep 17 00:00:00 2001 From: "Christoph M. Becker" Date: Sun, 24 Nov 2024 18:43:59 +0100 Subject: [PATCH 3/4] Fix further test cases * bug60602.phpt has the same issue as bug64770.phpt (copy&paste) * bug49936_win32.phpt should suppress the warnings[1], or be dropped altogether[2] [1] [2] --- ext/standard/tests/streams/bug49936_win32.phpt | 11 ++--------- ext/standard/tests/streams/bug60602.phpt | 5 +++-- 2 files changed, 5 insertions(+), 11 deletions(-) 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; From ed9ca7be145344a039a3ed7750e1d6f5295d1bcf Mon Sep 17 00:00:00 2001 From: "Christoph M. Becker" Date: Sun, 24 Nov 2024 19:06:01 +0100 Subject: [PATCH 4/4] Only call `PeekNamedPipe` on pipes `FILE_TYPE_PIPE` will also be returned for sockets, but these have been filtered out already. --- win32/select.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/win32/select.c b/win32/select.c index 0f14a8a25ef53..b1e55d5f18519 100644 --- a/win32/select.c +++ b/win32/select.c @@ -136,7 +136,10 @@ PHPAPI int php_select(php_socket_t max_fd, fd_set *rfds, fd_set *wfds, fd_set *e if (WAIT_OBJECT_0 == WaitForSingleObject(handles[i], 0)) { if (SAFE_FD_ISSET(handle_slot_to_fd[i], rfds)) { DWORD avail_read = 0; - if (!PeekNamedPipe(handles[i], NULL, 0, NULL, &avail_read, NULL) || 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++; }