Fixes shell edge-cases in Windows

Fixes executable paths with spaces in them, argument escapes,
and other strange behavior in Windows.

Also, fixes some shell tests on Windows. This is done by using
PHP scripts instead of native POSIX executables like "cat".

Behavior should be exactly the same on non-Windows servers.

Bug: T183759
Change-Id: I2367a6c47e3774bf4fabfa8c66e4bc4c5c8a714a

includes/shell/Command.php

@@ -348,6 +348,10 @@ class Command {
 			$cmd .= ' 2>&1';
 		}
 
+		if ( wfIsWindows() ) {
+			$cmd = 'cmd.exe /c "' . $cmd . '"';
+		}
+
 		return [ $cmd, $useLogPipe ];
 	}
 
@@ -388,7 +392,16 @@ class Command {
 		}
 		$pipes = null;
 		$scoped = Profiler::instance()->scopedProfileIn( __FUNCTION__ . '-' . $profileMethod );
-		$proc = proc_open( $cmd, $desc, $pipes );
+		$proc = null;
+
+		if ( wfIsWindows() ) {
+			// Windows Shell bypassed, but command run is "cmd.exe /C "{$cmd}"
+			// This solves some shell parsing issues, see T207248
+			$proc = proc_open( $cmd, $desc, $pipes, null, null, [ 'bypass_shell' => true ] );
+		} else {
+			$proc = proc_open( $cmd, $desc, $pipes );
+		}
+
 		if ( !$proc ) {
 			$this->logger->error( "proc_open() failed: {command}", [ 'command' => $cmd ] );
 			throw new ProcOpenError();

tests/phpunit/includes/shell/CommandTest.php

@@ -21,12 +21,10 @@ class CommandTest extends PHPUnit\Framework\TestCase {
 	/**
 	 * @dataProvider provideExecute
 	 */
-	public function testExecute( $commandInput, $expectedExitCode, $expectedOutput ) {
-		$this->requirePosix();
-
-		$command = new Command();
+	public function testExecute( $command, $args, $expectedExitCode, $expectedOutput ) {
+		$command = $this->getPhpCommand( $command );
 		$result = $command
-			->params( $commandInput )
+			->params( $args )
 			->execute();
 
 		$this->assertSame( $expectedExitCode, $result->getExitCode() );
@@ -35,37 +33,34 @@ class CommandTest extends PHPUnit\Framework\TestCase {
 
 	public function provideExecute() {
 		return [
-			'success status' => [ 'true', 0, '' ],
-			'failure status' => [ 'false', 1, '' ],
-			'output' => [ [ 'echo', '-n', 'x', '>', 'y' ], 0, 'x > y' ],
+			'success status' => [ 'success_status.php', [], 0, '' ],
+			'failure status' => [ 'failure_status.php', [], 1, '' ],
+			'output' => [ 'echo_args.php', [ 'x', '>', 'y' ], 0, 'x > y' ],
 		];
 	}
 
 	public function testEnvironment() {
-		$this->requirePosix();
-
-		$command = new Command();
+		$command = $this->getPhpCommand( 'echo_env.php' );
 		$result = $command
-			->params( [ 'printenv', 'FOO' ] )
+			->params( [ 'FOO' ] )
 			->environment( [ 'FOO' => 'bar' ] )
 			->execute();
-		$this->assertSame( "bar\n", $result->getStdout() );
+		$this->assertSame( "bar", $result->getStdout() );
 	}
 
 	public function testStdout() {
-		$this->requirePosix();
-
-		$command = new Command();
+		$command = $this->getPhpCommand( 'echo_args.php' );
 
 		$result = $command
-			->params( 'bash', '-c', 'echo ThisIsStderr 1>&2' )
+			->unsafeParams( 'ThisIsStderr', '1>&2' )
 			->execute();
 
 		$this->assertStringNotContainsString( 'ThisIsStderr', $result->getStdout() );
-		$this->assertEquals( "ThisIsStderr\n", $result->getStderr() );
+		$this->assertEquals( "ThisIsStderr", $result->getStderr() );
 	}
 
 	public function testStdoutRedirection() {
+		// The double redirection doesn't work on Windows
 		$this->requirePosix();
 
 		$command = new Command();
@@ -80,33 +75,33 @@ class CommandTest extends PHPUnit\Framework\TestCase {
 	}
 
 	public function testOutput() {
-		global $IP;
-
-		$this->requirePosix();
-		chdir( $IP );
-
-		$command = new Command();
-		$result = $command
-			->params( [ 'ls', 'index.php' ] )
-			->execute();
-		$this->assertRegExp( '/^index.php$/m', $result->getStdout() );
+		$command = $this->getPhpCommand(
+			'stdout_stderr.php',
+			[ 'correct stdout' ]
+		);
+		$result = $command->execute();
+		$this->assertSame( 'correct stdout', $result->getStdout() );
 		$this->assertSame( null, $result->getStderr() );
 
-		$command = new Command();
+		$command = $this->getPhpCommand(
+			'stdout_stderr.php',
+			[ 'correct stdout ', 'correct stderr ' ]
+		);
 		$result = $command
-			->params( [ 'ls', 'index.php', 'no-such-file' ] )
 			->includeStderr()
 			->execute();
-		$this->assertRegExp( '/^index.php$/m', $result->getStdout() );
-		$this->assertRegExp( '/^.+no-such-file.*$/m', $result->getStdout() );
+		$this->assertRegExp( '/correct stdout/m', $result->getStdout() );
+		$this->assertRegExp( '/correct stderr/m', $result->getStdout() );
 		$this->assertSame( null, $result->getStderr() );
 
-		$command = new Command();
+		$command = $this->getPhpCommand(
+			'stdout_stderr.php',
+			[ 'correct stdout', 'correct stderr' ]
+		);
 		$result = $command
-			->params( [ 'ls', 'index.php', 'no-such-file' ] )
 			->execute();
-		$this->assertRegExp( '/^index.php$/m', $result->getStdout() );
-		$this->assertRegExp( '/^.+no-such-file.*$/m', $result->getStderr() );
+		$this->assertSame( 'correct stdout', $result->getStdout() );
+		$this->assertSame( 'correct stderr', $result->getStderr() );
 	}
 
 	/**
@@ -116,22 +111,25 @@ class CommandTest extends PHPUnit\Framework\TestCase {
 		$command = TestingAccessWrapper::newFromObject( new Command );
 		$command->params( 'echo', 'a', null, 'b' );
 		$command->unsafeParams( 'c', null, 'd' );
-		$this->assertEquals( "'echo' 'a' 'b' c d", $command->command );
+
+		if ( wfIsWindows() ) {
+			$this->assertEquals( '"echo" "a" "b" c d', $command->command );
+		} else {
+			$this->assertEquals( "'echo' 'a' 'b' c d", $command->command );
+		}
 	}
 
 	public function testT69870() {
-		if ( wfIsWindows() ) {
-			// T209159: Anonymous pipe under Windows does not support asynchronous read and write,
-			// and the default buffer is too small (~4K), it is easy to be blocked.
-			$this->markTestSkipped(
-				'T209159: Anonymous pipe under Windows cannot withstand such a large amount of data'
-			);
-		}
+		// Testing for Bug T69870
+		//     wfShellExec() cuts off stdout at multiples of 8192 bytes.
+
+		// hangs on Windows, see Bug T199989, non-blocking pipes
+		$this->requirePosix();
 
 		// Test several times because it involves a race condition that may randomly succeed or fail
 		for ( $i = 0; $i < 10; $i++ ) {
-			$command = new Command();
-			$output = $command->unsafeParams( 'printf "%-333333s" "*"' )
+			$command = $this->getPhpCommand( 'echo_333333_stars.php' );
+			$output = $command
 				->execute()
 				->getStdout();
 			$this->assertEquals( 333333, strlen( $output ) );
@@ -139,45 +137,41 @@ class CommandTest extends PHPUnit\Framework\TestCase {
 	}
 
 	public function testLogStderr() {
-		$this->requirePosix();
-
 		$logger = new TestLogger( true, function ( $message, $level, $context ) {
 			return $level === Psr\Log\LogLevel::ERROR ? '1' : null;
 		}, true );
-		$command = new Command();
+		$command = $this->getPhpCommand( 'echo_args.php' );
 		$command->setLogger( $logger );
-		$command->params( 'bash', '-c', 'echo ThisIsStderr 1>&2' );
+		$command->unsafeParams( 'ThisIsStderr', '1>&2' );
 		$command->execute();
 		$this->assertSame( [], $logger->getBuffer() );
 
-		$command = new Command();
+		$command = $this->getPhpCommand( 'echo_args.php' );
 		$command->setLogger( $logger );
 		$command->logStderr();
-		$command->params( 'bash', '-c', 'echo ThisIsStderr 1>&2' );
+		$command->unsafeParams( 'ThisIsStderr', '1>&2' );
 		$command->execute();
 		$this->assertCount( 1, $logger->getBuffer() );
 		$this->assertSame( trim( $logger->getBuffer()[0][2]['error'] ), 'ThisIsStderr' );
 	}
 
 	public function testInput() {
+		// hangs on Windows, see Bug T199989, non-blocking pipes
 		$this->requirePosix();
 
-		$command = new Command();
-		$command->params( 'cat' );
+		$command = $this->getPhpCommand( 'echo_stdin.php' );
 		$command->input( 'abc' );
 		$result = $command->execute();
 		$this->assertSame( 'abc', $result->getStdout() );
 
 		// now try it with something that does not fit into a single block
-		$command = new Command();
-		$command->params( 'cat' );
+		$command = $this->getPhpCommand( 'echo_stdin.php' );
 		$command->input( str_repeat( '!', 1000000 ) );
 		$result = $command->execute();
 		$this->assertSame( 1000000, strlen( $result->getStdout() ) );
 
 		// And try it with empty input
-		$command = new Command();
-		$command->params( 'cat' );
+		$command = $this->getPhpCommand( 'echo_stdin.php' );
 		$command->input( '' );
 		$result = $command->execute();
 		$this->assertSame( '', $result->getStdout() );
@@ -195,4 +189,33 @@ class CommandTest extends PHPUnit\Framework\TestCase {
 		$command->restrict( Shell::RESTRICT_NONE );
 		$this->assertSame( 0, $command->restrictions );
 	}
+
+	/**
+	 * Creates a command that will execute one of the PHP test scripts by its
+	 * file name, using the current PHP_BIN binary.
+	 *
+	 * NOTE: the PHP test scripts are located in the sub directory
+	 * "bin".
+	 *
+	 * @param string $fileName  a file name in the "bin" sub-directory
+	 * @param array $args       an array of arguments to pass to the PHP script
+	 *
+	 * @return Command  a command instance pointing to the right script
+	 */
+	private function getPhpCommand( $fileName, array $args = [] ) {
+		$command = new Command;
+		$params = [
+			PHP_BINARY,
+			__DIR__
+				 . DIRECTORY_SEPARATOR
+				 . 'bin'
+				 . DIRECTORY_SEPARATOR
+				 . $fileName
+		];
+		$params = array_merge( $params, $args );
+
+		$command->params( $params );
+		$command->limits( [ 'memory' => 0 ] );
+		return $command;
+	}
 }

tests/phpunit/includes/shell/ShellTest.php

@@ -37,7 +37,8 @@ class ShellTest extends MediaWikiIntegrationTestCase {
 	 * @covers \MediaWiki\Shell\Shell::makeScriptCommand
 	 * @dataProvider provideMakeScriptCommand
 	 *
-	 * @param string $expected
+	 * @param string $expected    expected in POSIX
+	 * @param string $expectedWin expected in Windows
 	 * @param string $script
 	 * @param string[] $parameters
 	 * @param string[] $options
@@ -45,6 +46,7 @@ class ShellTest extends MediaWikiIntegrationTestCase {
 	 */
 	public function testMakeScriptCommand(
 		$expected,
+		$expectedWin,
 		$script,
 		$parameters,
 		$options = [],
@@ -64,7 +66,12 @@ class ShellTest extends MediaWikiIntegrationTestCase {
 		$this->assertInstanceOf( Command::class, $command );
 
 		$wrapper = TestingAccessWrapper::newFromObject( $command );
-		$this->assertEquals( $expected, $wrapper->command );
+
+		if ( wfIsWindows() ) {
+			$this->assertEquals( $expectedWin, $wrapper->command );
+		} else {
+			$this->assertEquals( $expected, $wrapper->command );
+		}
 		$this->assertSame( 0, $wrapper->restrictions & Shell::NO_LOCALSETTINGS );
 	}
 
@@ -74,11 +81,13 @@ class ShellTest extends MediaWikiIntegrationTestCase {
 		return [
 			[
 				"'$wgPhpCli' 'maintenance/foobar.php' 'bar'\\''\"baz' 'safe' unsafe",
+				'"' . $wgPhpCli . '" "maintenance/foobar.php" "bar\'\\"baz" "safe" unsafe',
 				'maintenance/foobar.php',
 				[ 'bar\'"baz' ],
 			],
 			[
 				"'$wgPhpCli' 'changed.php' '--wiki=somewiki' 'bar'\\''\"baz' 'safe' unsafe",
+				'"' . $wgPhpCli . '" "changed.php" "--wiki=somewiki" "bar\'\\"baz" "safe" unsafe',
 				'maintenance/foobar.php',
 				[ 'bar\'"baz' ],
 				[],
@@ -89,12 +98,14 @@ class ShellTest extends MediaWikiIntegrationTestCase {
 			],
 			[
 				"'/bin/perl' 'maintenance/foobar.php' 'bar'\\''\"baz' 'safe' unsafe",
+				'"/bin/perl" "maintenance/foobar.php" "bar\'\\"baz" "safe" unsafe',
 				'maintenance/foobar.php',
 				[ 'bar\'"baz' ],
 				[ 'php' => '/bin/perl' ],
 			],
 			[
 				"'$wgPhpCli' 'foobinize' 'maintenance/foobar.php' 'bar'\\''\"baz' 'safe' unsafe",
+				'"' . $wgPhpCli . '" "foobinize" "maintenance/foobar.php" "bar\'\\"baz" "safe" unsafe',
 				'maintenance/foobar.php',
 				[ 'bar\'"baz' ],
 				[ 'wrapper' => 'foobinize' ],

tests/phpunit/includes/shell/bin/echo_333333_stars.php

@@ -0,0 +1,7 @@
+<?php
+
+if ( PHP_SAPI !== 'cli' ) {
+	exit( 1 );
+}
+
+echo str_repeat( '*', 333333 );

tests/phpunit/includes/shell/bin/echo_args.php

@@ -0,0 +1,12 @@
+<?php
+
+if ( PHP_SAPI !== 'cli' ) {
+	exit( 1 );
+}
+
+for ( $i = 1; $i < count( $argv ); $i++ ) {
+	fprintf( STDOUT, "%s", $argv[$i] );
+
+	if ( $i + 1 < count( $argv ) )
+		fprintf( STDOUT, " " );
+}

tests/phpunit/includes/shell/bin/echo_env.php

@@ -0,0 +1,12 @@
+<?php
+
+if ( PHP_SAPI !== 'cli' ) {
+	exit( 1 );
+}
+
+for ( $i = 1; $i < count( $argv ); $i++ ) {
+	fprintf( STDOUT, "%s", getenv( $argv[$i] ) );
+
+	if ( $i + 1 < count( $argv ) )
+		fprintf( STDOUT, "\n" );
+}

tests/phpunit/includes/shell/bin/echo_stdin.php

@@ -0,0 +1,8 @@
+<?php
+
+if ( PHP_SAPI !== 'cli' ) {
+	exit( 1 );
+}
+
+$input_data = stream_get_contents( STDIN );
+echo $input_data;

tests/phpunit/includes/shell/bin/failure_status.php

@@ -0,0 +1,3 @@
+<?php
+
+exit( 1 );

tests/phpunit/includes/shell/bin/stdout_stderr.php

@@ -0,0 +1,10 @@
+<?php
+
+if ( PHP_SAPI !== 'cli' ) {
+	exit( 1 );
+}
+
+file_put_contents( "php://stdout", $argv[1] );
+if ( isset( $argv[2] ) ) {
+	file_put_contents( "php://stderr", $argv[2] );
+}

tests/phpunit/includes/shell/bin/success_status.php

@@ -0,0 +1 @@
+<?php