]> granicus.if.org Git - php/commitdiff
Fixed bug #51800 proc_open on Windows hangs forever
authorAnatol Belski <ab@php.net>
Mon, 29 Sep 2014 14:24:34 +0000 (16:24 +0200)
committerAnatol Belski <ab@php.net>
Mon, 29 Sep 2014 14:24:34 +0000 (16:24 +0200)
This loop can block for some minutes, theoretically. Practially
however, this is a 99% non issue for a normal use case. This is
required because read() is synchronous. The PHP streams API wants
to fill its internal buffers, therefore it might try to read some
more data than user has demanded. Also, for a case where we want
to read X bytes, but neither enough data nor EOF arrives, read()
will block until it could fill the buffer. If a counterpart station
runs slowly or delivers not all the data at once, read() would
still be waiting. If we quit too early, we possibly could loose
some data from the pipe. Thus it has to emulate the read()
behaviour, but obviously not completely, just to some grade.

Reading big data amount is for sure an issue on any platforms, it
depends on the pipe buffer size, which is controlled by the system.
On Windows, the buffer size seems to be way too small, which causes
buffer congestion and a dead lock. It is essential to read the pipe
descriptors simultaneously and possibly in the same order as the
opposite writes them.

Thus, this will work with smaller buffer data sizes passed through
pipes. As MSDN states, anonymous pipes don't support asynchronous
operations. Neither anonymous pipes do support select() as they are
not SOCKETs but file descriptors. Consequently - bigger data sizes
will need a better solution based on threads. However it is much
more expencive. Maybe a better solution could be exporting a part
of the internal doing as a userspace function which could perform
some kind of lookahead operation on the pipe descriptor.

This is just the first stone, depending on the user feedback we
might go for further improvements in this area.

NEWS
ext/standard/tests/streams/proc_open_bug51800.phpt [new file with mode: 0644]
ext/standard/tests/streams/proc_open_bug51800_right.phpt [new file with mode: 0644]
ext/standard/tests/streams/proc_open_bug60120.phpt [new file with mode: 0644]
ext/standard/tests/streams/proc_open_bug64438.phpt [new file with mode: 0644]
main/streams/plain_wrapper.c

diff --git a/NEWS b/NEWS
index 2942c720edc74d20b2d96698360eb7f1c0c77ca4..50d997875cc3a4c3b62190d44e60d203f7c30516 100644 (file)
--- a/NEWS
+++ b/NEWS
@@ -9,6 +9,7 @@ PHP                                                                        NEWS
     as 6.2 (instead of 6.3)). (Christian Wenz)
   . Fixed bug #67633 (A foreach on an array returned from a function not doing
     copy-on-write). (Nikita)
+  . Fixed bug #51800 (proc_open on Windows hangs forever). (Anatol)
 
 - FPM:
   . Fixed bug #65641 (PHP-FPM incorrectly defines the SCRIPT_NAME variable
diff --git a/ext/standard/tests/streams/proc_open_bug51800.phpt b/ext/standard/tests/streams/proc_open_bug51800.phpt
new file mode 100644 (file)
index 0000000..024d9cd
--- /dev/null
@@ -0,0 +1,91 @@
+--TEST--
+Bug #51800 proc_open on Windows hangs forever
+--SKIPIF--
+<?php
+       if (getenv("SKIP_SLOW_TESTS")) {
+               die("skip slow test");
+       }
+?>
+--XFAIL--
+pipes have to be read/written simultaneously
+--FILE--
+<?php
+/* This is the wrong way to do it. The parent will block till it has read all the STDIN.
+The smaller the pipe buffer is, the longer it will take. It might even pass at the end,
+after taking inappropriately long. Pipes have to be read simultaneously in smaller chunks,
+so then the pipe buffer is emptied more often and the child has chance to continue its
+write. The behaviour might look some better if write/read in a separate thread, however
+this is much more resource greedy and complexer to integrate into the user script. */
+
+$callee = dirname(__FILE__) . "/process" . md5(uniqid()) . ".php";
+$php = PHP_BINARY;
+$cmd = "$php $callee";
+
+$status;
+$stdout = "";
+$stderr = "";
+$pipes = array();
+
+$descriptors = array(
+        0 => array("pipe", "rb"),       // stdin
+        1 => array("pipe", "wb"),       // stdout
+        2 => array("pipe", "wb")                // stderr
+        );
+
+/* create the proc file */
+$r = file_put_contents($callee, '<?php
+
+$how_much = 10000;
+
+$data0 = str_repeat("a", $how_much);
+$data1 = str_repeat("b", $how_much);
+fwrite(STDOUT, $data0);
+fwrite(STDERR, $data1);
+
+exit(0);
+');
+
+if (!$r) {
+       die("couldn't create helper script '$callee'");
+}
+
+$process = proc_open($cmd, $descriptors, $pipes);
+
+if (is_resource($process))
+{
+        fclose($pipes[0]);
+
+       while (!feof($pipes[1]))
+               $stdout .= fread($pipes[1], 1024);
+       fclose($pipes[1]);
+
+       while (!feof($pipes[2]))
+               $stderr .= fread($pipes[2], 1024);
+       fclose($pipes[2]);
+
+        $status = proc_close($process);
+}
+
+var_dump(array(
+        "status" => $status,
+        "stdout" => $stdout,
+        "stderr" => $stderr,
+), strlen($stdout), strlen($stderr));
+
+unlink($callee);
+
+?>
+===DONE===
+--EXPECTF--
+array(3) {
+  ["status"]=>
+  int(0)
+  ["stdout"]=>
+  string(10000) "a%s"
+  ["stderr"]=>
+  string(10000) "b%s"
+}
+int(10000)
+int(10000)
+===DONE===
+
diff --git a/ext/standard/tests/streams/proc_open_bug51800_right.phpt b/ext/standard/tests/streams/proc_open_bug51800_right.phpt
new file mode 100644 (file)
index 0000000..bab37a8
--- /dev/null
@@ -0,0 +1,75 @@
+--TEST--
+Bug #51800 proc_open on Windows hangs forever, the right way to do it
+--FILE--
+<?php
+$callee = dirname(__FILE__) . "/process" . md5(uniqid()) . ".php";
+$php = PHP_BINARY;
+$cmd = "$php $callee";
+
+$status;
+$stdout = "";
+$stderr = "";
+$pipes = array();
+
+$descriptors = array(
+        0 => array("pipe", "rb"),       // stdin
+        1 => array("pipe", "wb"),       // stdout
+        2 => array("pipe", "wb")                // stderr
+        );
+
+/* create the proc file */
+$r = file_put_contents($callee, '<?php
+
+$how_much = 10000;
+
+$data0 = str_repeat("a", $how_much);
+$data1 = str_repeat("b", $how_much);
+fwrite(STDOUT, $data0);
+fwrite(STDERR, $data1);
+
+exit(0);
+');
+
+if (!$r) {
+       die("couldn't create helper script '$callee'");
+}
+
+$process = proc_open($cmd, $descriptors, $pipes);
+
+if (is_resource($process))
+{
+        fclose($pipes[0]);
+
+        while (!feof($pipes[1]) || !feof($pipes[2])) {
+                $stdout .= fread($pipes[1], 1024);
+                $stderr .= fread($pipes[2], 1024);
+        }
+        fclose($pipes[1]);
+        fclose($pipes[2]);
+
+        $status = proc_close($process);
+}
+
+var_dump(array(
+        "status" => $status,
+        "stdout" => $stdout,
+        "stderr" => $stderr,
+), strlen($stdout), strlen($stderr));
+
+unlink($callee);
+
+?>
+===DONE===
+--EXPECTF--
+array(3) {
+  ["status"]=>
+  int(0)
+  ["stdout"]=>
+  string(10000) "a%s"
+  ["stderr"]=>
+  string(10000) "b%s"
+}
+int(10000)
+int(10000)
+===DONE===
+
diff --git a/ext/standard/tests/streams/proc_open_bug60120.phpt b/ext/standard/tests/streams/proc_open_bug60120.phpt
new file mode 100644 (file)
index 0000000..978cbe6
--- /dev/null
@@ -0,0 +1,67 @@
+--TEST--
+Bug #60120 proc_open hangs with stdin/out with 2048+ bytes
+--FILE--
+<?php
+error_reporting(E_ALL);
+
+$cmd = PHP_BINARY . ' -n -r "fwrite(STDOUT, $in = file_get_contents(\'php://stdin\')); fwrite(STDERR, $in);"';
+$descriptors = array(array('pipe', 'r'), array('pipe', 'w'), array('pipe', 'w'));
+$stdin = str_repeat('*', 1024 * 16) . '!';
+$stdin = str_repeat('*', 2049 );
+
+$options = array_merge(array('suppress_errors' => true, 'binary_pipes' => true, 'bypass_shell' => false));
+$process = proc_open($cmd, $descriptors, $pipes, getcwd(), array(), $options);
+
+foreach ($pipes as $pipe) {
+    stream_set_blocking($pipe, false);
+}
+$writePipes = array($pipes[0]);
+$stdinLen = strlen($stdin);
+$stdinOffset = 0;
+
+unset($pipes[0]);
+
+while ($pipes || $writePipes) {
+    $r = $pipes;
+    $w = $writePipes;
+    $e = null;
+    $n = stream_select($r, $w, $e, 60);
+
+    if (false === $n) {
+        break;
+    } elseif ($n === 0) {
+        proc_terminate($process);
+
+    }
+    if ($w) {
+        $written = fwrite($writePipes[0], (binary)substr($stdin, $stdinOffset), 8192);
+        if (false !== $written) {
+            $stdinOffset += $written;
+        }
+        if ($stdinOffset >= $stdinLen) {
+            fclose($writePipes[0]);
+            $writePipes = null;
+        }
+    }
+
+    foreach ($r as $pipe) {
+        $type = array_search($pipe, $pipes);
+        $data = fread($pipe, 8192);
+        var_dump($data);
+        if (false === $data || feof($pipe)) {
+            fclose($pipe);
+            unset($pipes[$type]);
+        }
+    }
+}
+
+
+?>
+===DONE===
+--EXPECTF--
+string(2049) "%s"
+string(2049) "%s"
+string(0) ""
+string(0) ""
+===DONE===
+
diff --git a/ext/standard/tests/streams/proc_open_bug64438.phpt b/ext/standard/tests/streams/proc_open_bug64438.phpt
new file mode 100644 (file)
index 0000000..e328851
--- /dev/null
@@ -0,0 +1,66 @@
+--TEST--
+Bug #64438 proc_open hangs with stdin/out with 4097+ bytes
+--FILE--
+<?php
+
+error_reporting(E_ALL);
+
+$cmd = PHP_BINARY . ' -n -r "fwrite(STDOUT, $in = file_get_contents(\'php://stdin\')); fwrite(STDERR, $in);"';
+$descriptors = array(array('pipe', 'r'), array('pipe', 'w'), array('pipe', 'w'));
+$stdin = str_repeat('*', 4097);
+
+$options = array_merge(array('suppress_errors' => true, 'binary_pipes' => true, 'bypass_shell' => false));
+$process = proc_open($cmd, $descriptors, $pipes, getcwd(), array(), $options);
+
+foreach ($pipes as $pipe) {
+    stream_set_blocking($pipe, false);
+}
+$writePipes = array($pipes[0]);
+$stdinLen = strlen($stdin);
+$stdinOffset = 0;
+
+unset($pipes[0]);
+
+while ($pipes || $writePipes) {
+    $r = $pipes;
+    $w = $writePipes;
+    $e = null;
+    $n = stream_select($r, $w, $e, 60);
+
+    if (false === $n) {
+        break;
+    } elseif ($n === 0) {
+        proc_terminate($process);
+
+    }
+    if ($w) {
+        $written = fwrite($writePipes[0], (binary)substr($stdin, $stdinOffset), 8192);
+        if (false !== $written) {
+            $stdinOffset += $written;
+        }
+        if ($stdinOffset >= $stdinLen) {
+            fclose($writePipes[0]);
+            $writePipes = null;
+        }
+    }
+
+    foreach ($r as $pipe) {
+        $type = array_search($pipe, $pipes);
+        $data = fread($pipe, 8192);
+        var_dump($data);
+        if (false === $data || feof($pipe)) {
+            fclose($pipe);
+            unset($pipes[$type]);
+        }
+    }
+}
+
+?>
+===DONE===
+--EXPECTF--
+string(4097) "%s"
+string(4097) "%s"
+string(0) ""
+string(0) ""
+===DONE===
+
index a50662b78e4cc093cba8a548578bd1874e9f7a8f..20ec7c2423cce54a895007253e523f1da5f1d44c 100644 (file)
@@ -348,6 +348,34 @@ static size_t php_stdiop_read(php_stream *stream, char *buf, size_t count TSRMLS
        assert(data != NULL);
 
        if (data->fd >= 0) {
+#ifdef PHP_WIN32
+               php_stdio_stream_data *self = (php_stdio_stream_data*)stream->abstract;
+
+               if (self->is_pipe || self->is_process_pipe) {
+                       HANDLE ph = (HANDLE)_get_osfhandle(data->fd);
+                       int retry = 0;
+                       DWORD avail_read = 0;
+
+                       do {
+                               /* Look ahead to get the available data amount to read. Do the same
+                                       as read() does, however not blocking forever. In case it failed,
+                                       no data will be read (better than block). */
+                               if (!PeekNamedPipe(ph, NULL, 0, NULL, &avail_read, NULL)) {
+                                       break;
+                               }
+                               /* If there's nothing to read, wait in 100ms periods. */
+                               if (0 == avail_read) {
+                                       usleep(100000);
+                               }
+                       } while (0 == avail_read && retry++ < 180);
+
+                       /* Reduce the required data amount to what is available, otherwise read()
+                               will block.*/
+                       if (avail_read < count) {
+                               count = avail_read;
+                       }
+               }
+#endif
                ret = read(data->fd, buf, count);
 
                if (ret == (size_t)-1 && errno == EINTR) {