Fix race condition in wfShellExec()

Especially when executing commands that return a relatively lot of data in stdout
quickly, proc_get_status() may return that command has terminated before everything
has been read from pipes. Handle this case by continuing to perform
non-blocking select on the process's streams until all remaining data has been
read.

Bug: 67870
Change-Id: I050292dbb76821f66a15f937bf3aaf4defe67687

diff --git a/includes/GlobalFunctions.php b/includes/GlobalFunctions.php
index b41ec9f..fe12a07 100644 (file)
--- a/includes/GlobalFunctions.php
+++ b/includes/GlobalFunctions.php
@@ -2910,19 +2910,28 @@ function wfShellExec( $cmd, &$retval = null, $environ = array(),
                $fds[(int)$pipe] = $fd;
        }
 
-       while ( true ) {
-               $status = proc_get_status( $proc );
-               if ( !$status['running'] ) {
-                       break;
+       $running = true;
+       $timeout = null;
+       $numReadyPipes = 0;
+
+       while ( $running === true || $numReadyPipes !== 0 ) {
+               if ( $running ) {
+                       $status = proc_get_status( $proc );
+                       // If the process has terminated, switch to nonblocking selects
+                       // for getting any data still waiting to be read.
+                       if ( !$status['running'] ) {
+                               $running = false;
+                               $timeout = 0;
+                       }
                }
-               $status = false;
 
                $readyPipes = $pipes;
 
                // Clear last error
                // @codingStandardsIgnoreStart Generic.PHP.NoSilencedErrors.Discouraged
                @trigger_error( '' );
-               if ( @stream_select( $readyPipes, $emptyArray, $emptyArray, null ) === false ) {
+               $numReadyPipes = @stream_select( $readyPipes, $emptyArray, $emptyArray, $timeout );
+               if ( $numReadyPipes === false ) {
                        // @codingStandardsIgnoreEnd
                        $error = error_get_last();
                        if ( strncmp( $error['message'], $eintrMessage, strlen( $eintrMessage ) ) == 0 ) {
@@ -2970,7 +2979,7 @@ function wfShellExec( $cmd, &$retval = null, $environ = array(),
 
        // Use the status previously collected if possible, since proc_get_status()
        // just calls waitpid() which will not return anything useful the second time.
-       if ( $status === false ) {
+       if ( $running ) {
                $status = proc_get_status( $proc );
        }
 

diff --git a/tests/phpunit/includes/GlobalFunctions/wfShellExecTest.php b/tests/phpunit/includes/GlobalFunctions/wfShellExecTest.php
new file mode 100644 (file)
index 0000000..fcd26f5
--- /dev/null
+++ b/tests/phpunit/includes/GlobalFunctions/wfShellExecTest.php
@@ -0,0 +1,20 @@
+<?php
+
+/**
+ * @group GlobalFunctions
+ * @covers ::wfShellExec
+ */
+class WfShellExecTest extends MediaWikiTestCase {
+       public function testBug67870() {
+               $command = wfIsWindows()
+                       // 333 = 331 + CRLF
+                       ? ( 'for /l %i in (1, 1, 1001) do @echo ' . str_repeat( '*', 331 ) )
+                       : 'printf "%-333333s" "*"';
+
+               // Test several times because it involves a race condition that may randomly succeed or fail
+               for ( $i = 0; $i < 10; $i++ ) {
+                       $output = wfShellExec( $command );
+                       $this->assertEquals( 333333, strlen( $output ) );
+               }
+       }
+}