From 9f522988b8b12c59131e394875cc2ecefc89a28d Mon Sep 17 00:00:00 2001
From: Max Semenik <maxsem.wiki@gmail.com>
Date: Fri, 18 Jul 2014 13:03:06 -0700
Subject: [PATCH] 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
---
 includes/GlobalFunctions.php                  | 23 +++++++++++++------
 .../GlobalFunctions/wfShellExecTest.php       | 20 ++++++++++++++++
 2 files changed, 36 insertions(+), 7 deletions(-)
 create mode 100644 tests/phpunit/includes/GlobalFunctions/wfShellExecTest.php

diff --git a/includes/GlobalFunctions.php b/includes/GlobalFunctions.php
index b41ec9ff74..fe12a078e2 100644
--- 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
index 0000000000..fcd26f541f
--- /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 ) );
+		}
+	}
+}
-- 
2.20.1