Skip to content

Commit

Permalink
GH-16889: stream_select() timeout useless for pipes on Windows
Browse files Browse the repository at this point in the history
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] <c884d37>
[2] <2c6b85f>

Closes GH-16917.
  • Loading branch information
cmb69 committed Dec 7, 2024
1 parent d98e191 commit b614b4a
Show file tree
Hide file tree
Showing 6 changed files with 49 additions and 16 deletions.
4 changes: 4 additions & 0 deletions NEWS
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
11 changes: 2 additions & 9 deletions ext/standard/tests/streams/bug49936_win32.phpt
Original file line number Diff line number Diff line change
Expand Up @@ -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)
5 changes: 3 additions & 2 deletions ext/standard/tests/streams/bug60602.phpt
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand All @@ -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;
Expand Down
5 changes: 3 additions & 2 deletions ext/standard/tests/streams/bug64770.phpt
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand All @@ -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;
Expand Down
26 changes: 26 additions & 0 deletions ext/standard/tests/streams/gh16889.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
--TEST--
GH-16889 (stream_select() timeout useless for pipes on Windows)
--FILE--
<?php
$desc = [
["pipe", "r"],
["pipe", "w"],
["pipe", "w"],
];
// open process which won't produce output for 10s
$proc = proc_open([PHP_BINARY, "-r", "sleep(10); echo 'finish';"], $desc, $pipes);
$read = [$pipes[1]];
$write = null;
$except = null;
$time0 = microtime(true);
// select STDOUT pipe of process for 1ms
if (stream_select($read, $write, $except, 0, 1000)) {
var_dump(fread($read[0], 1));
}
// avoid blocking of finishing the test process
proc_terminate($proc);
$time1 = microtime(true);
var_dump($time1 - $time0 < 1);
?>
--EXPECT--
bool(true)
14 changes: 11 additions & 3 deletions win32/select.c
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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++;
}
}
}
Expand Down

0 comments on commit b614b4a

Please sign in to comment.