diff --git a/src/future/exec/ExecFuture.php b/src/future/exec/ExecFuture.php --- a/src/future/exec/ExecFuture.php +++ b/src/future/exec/ExecFuture.php @@ -334,6 +334,8 @@ * @task config */ public function setTimeout($seconds) { + // Timeout requires non-blocking streams + $this->setUseWindowsFileStreams(TRUE); $this->timeout = $seconds; return $this; } @@ -423,7 +425,14 @@ $signal = 9; } - proc_terminate($this->proc, $signal); + if (phutil_is_windows()) { + // NOTE: See http://stackoverflow.com/a/28045959/90297 + $status = proc_get_status($this->proc); + exec('taskkill /F /T /PID '.$status['pid']); + } else { + proc_terminate($this->proc, $signal); + } + $this->result = array( 128 + $signal, $this->stdout, @@ -592,18 +601,6 @@ $pipes = array(); - if (phutil_is_windows()) { - // See T4395. proc_open under Windows uses "cmd /C [cmd]", which will - // strip the first and last quote when there aren't exactly two quotes - // (and some other conditions as well). This results in a command that - // looks like `command" "path to my file" "something something` which is - // clearly wrong. By surrounding the command string with quotes we can - // be sure this process is harmless. - if (strpos($unmasked_command, '"') !== false) { - $unmasked_command = '"'.$unmasked_command.'"'; - } - } - if ($this->hasEnv()) { $env = $this->getEnv(); } else { @@ -642,7 +639,8 @@ $spec, $pipes, $cwd, - $env); + $env, + array('bypass_shell' => true)); if ($this->useWindowsFileStreams) { fclose($spec[1]); @@ -762,11 +760,6 @@ } if ($is_done) { - if ($this->useWindowsFileStreams) { - fclose($stdout); - fclose($stderr); - } - // If the subprocess got nuked with `kill -9`, we get a -1 exitcode. // Upgrade this to a slightly more informative value by examining the // terminating signal code. diff --git a/src/future/exec/PhutilExecPassthru.php b/src/future/exec/PhutilExecPassthru.php --- a/src/future/exec/PhutilExecPassthru.php +++ b/src/future/exec/PhutilExecPassthru.php @@ -65,12 +65,6 @@ $spec = array(STDIN, STDOUT, STDERR); $pipes = array(); - if ($command instanceof PhutilCommandString) { - $unmasked_command = $command->getUnmaskedString(); - } else { - $unmasked_command = $command; - } - if ($this->hasEnv()) { $env = $this->getEnv(); } else { @@ -86,6 +80,16 @@ // invoked from git bash fail horridly, and everything is a mess in // general. $options['bypass_shell'] = true; + // Since we are not using CMD anymore, set the escaping mode accordingly + if ($command instanceof PhutilCommandString) { + $command->setEscapingMode(PhutilCommandString::MODE_WIN_ARGV); + } + } + + if ($command instanceof PhutilCommandString) { + $unmasked_command = $command->getUnmaskedString(); + } else { + $unmasked_command = $command; } $trap = new PhutilErrorTrap(); diff --git a/src/future/exec/__tests__/ExecFutureTestCase.php b/src/future/exec/__tests__/ExecFutureTestCase.php --- a/src/future/exec/__tests__/ExecFutureTestCase.php +++ b/src/future/exec/__tests__/ExecFutureTestCase.php @@ -2,11 +2,22 @@ final class ExecFutureTestCase extends PhutilTestCase { + // Do not use `cat` since on Windows, cat chokes after 8kb of STDIN input + // Also `cat` is only available through Git for Windows package + private static function cat() { + return new ExecFuture('php -r %s', 'while($f=fgets(STDIN)){echo $f;}'); + } + + // `sleep` is only available through Git for Windows package on Windows + private static function sleeper($seconds) { + return new ExecFuture('php -r %s', "sleep($seconds);"); + } + public function testEmptyWrite() { // NOTE: This is mostly testing that we don't hang while doing an empty // write. - list($stdout) = id(new ExecFuture('cat'))->write('')->resolvex(); + list($stdout) = self::cat()->write('')->resolvex(); $this->assertEqual('', $stdout); } @@ -14,7 +25,7 @@ public function testKeepPipe() { // NOTE: This is mostly testing the semantics of $keep_pipe in write(). - list($stdout) = id(new ExecFuture('cat')) + list($stdout) = self::cat() ->write('', true) ->start() ->write('x', true) @@ -30,14 +41,14 @@ // flushing a buffer. $data = str_repeat('x', 1024 * 1024 * 4); - list($stdout) = id(new ExecFuture('cat'))->write($data)->resolvex(); + list($stdout) = self::cat()->write($data)->resolvex(); - $this->assertEqual($data, $stdout); + $this->assertEqual($data, rtrim($stdout)); } public function testBufferLimit() { $data = str_repeat('x', 1024 * 1024); - list($stdout) = id(new ExecFuture('cat')) + list($stdout) = self::cat() ->setStdoutSizeLimit(1024) ->write($data) ->resolvex(); @@ -49,8 +60,7 @@ // NOTE: This tests interactions between the resolve() timeout and the // ExecFuture timeout, which are similar but not identical. - $future = id(new ExecFuture('sleep 32000'))->start(); - $future->setTimeout(32000); + $future = self::sleeper(32000)->setTimeout(32000)->start(); // We expect this to return in 0.01s. $result = $future->resolve(0.01); @@ -63,12 +73,11 @@ $future->resolve(); } - public function testTimeoutTestShouldRunLessThan1Sec() { // NOTE: This is partly testing that we choose appropriate select wait // times; this test should run for significantly less than 1 second. - $future = new ExecFuture('sleep 32000'); + $future = self::sleeper(32000); list($err) = $future->setTimeout(0.01)->resolve(); $this->assertTrue($err > 0); @@ -78,7 +87,7 @@ public function testMultipleTimeoutsTestShouldRunLessThan1Sec() { $futures = array(); for ($ii = 0; $ii < 4; $ii++) { - $futures[] = id(new ExecFuture('sleep 32000'))->setTimeout(0.01); + $futures[] = self::sleeper(32000)->setTimeout(0.01); } foreach (new FutureIterator($futures) as $future) { @@ -91,7 +100,7 @@ public function testNoHangOnExecFutureDestructionWithRunningChild() { $start = microtime(true); - $future = new ExecFuture('sleep 30'); + $future = self::sleeper(30); $future->start(); unset($future); $end = microtime(true); @@ -118,7 +127,7 @@ $str_len_4 = 'abcd'; // This is a write/read with no read buffer. - $future = new ExecFuture('cat'); + $future = self::cat(); $future->write($str_len_8); do { @@ -135,7 +144,7 @@ // This is a write/read with a read buffer. - $future = new ExecFuture('cat'); + $future = self::cat(); $future->write($str_len_8); // Set the read buffer size. diff --git a/src/future/exec/__tests__/ExecPassthruTestCase.php b/src/future/exec/__tests__/ExecPassthruTestCase.php --- a/src/future/exec/__tests__/ExecPassthruTestCase.php +++ b/src/future/exec/__tests__/ExecPassthruTestCase.php @@ -8,7 +8,7 @@ // the terminal, which is undesirable). This makes crafting effective unit // tests a fairly involved process. - $exec = new PhutilExecPassthru('exit'); + $exec = new PhutilExecPassthru('php -r "exit();"'); $err = $exec->execute(); $this->assertEqual(0, $err); } diff --git a/src/xsprintf/PhutilCommandString.php b/src/xsprintf/PhutilCommandString.php --- a/src/xsprintf/PhutilCommandString.php +++ b/src/xsprintf/PhutilCommandString.php @@ -6,6 +6,7 @@ private $escapingMode = false; const MODE_DEFAULT = 'default'; + const MODE_WIN_ARGV = 'winargv'; const MODE_POWERSHELL = 'powershell'; public function __construct(array $argv) { @@ -48,14 +49,64 @@ public static function escapeArgument($value, $mode) { switch ($mode) { case self::MODE_DEFAULT: - return escapeshellarg($value); + return phutil_is_windows() ? self::escapeWindowsArgv($value) : escapeshellarg($value); case self::MODE_POWERSHELL: return self::escapePowershell($value); + case self::MODE_WIN_ARGV: + return self::escapeWindowsArgv($value); default: throw new Exception(pht('Unknown escaping mode!')); } } + /** + * Escapes a single argument to be glued together and passed into + * CreateProcess on Windows through `proc_open`. + * + * Adapted from https://blogs.msdn.microsoft.com/twistylittlepassagesallalike/2011/04/23/everyone-quotes-command-line-arguments-the-wrong-way/ + * + * @param string The argument to be escaped + * @result string Escaped argument that can be used in a CreateProcess call + */ + public static function escapeWindowsArgv($value) { + // Don't quote unless we actually need to do so hopefully + // avoid problems if programs won't parse quotes properly + if ($value && !preg_match('/["[:space:]]/', $value)) { + return $value; + } + + $result = '"'; + $len = strlen($value); + for ($i = 0; $i < $len; $i++) { + $numBackslashes = 0; + + while ($i < $len && $value[$i] == '\\') { + $i++; + $numBackslashes++; + } + + if ($i == $len) { + // Escape all backslashes, but let the terminating + // double quotation mark we add below be interpreted + // as a metacharacter. + $result .= str_repeat('\\', $numBackslashes * 2); + break; + } elseif ($value[$i] == '"') { + // Escape all backslashes and the following double quotation mark. + $result .= str_repeat('\\', $numBackslashes * 2 + 1); + $result .= $value[$i]; + } else { + // Backslashes aren't special here. + $result .= str_repeat('\\', $numBackslashes); + $result .= $value[$i]; + } + } + + $result .= '"'; + + return $result; + } + private static function escapePowershell($value) { // These escape sequences are from http://ss64.com/ps/syntax-esc.html diff --git a/src/xsprintf/__tests__/PhutilCsprintfTestCase.php b/src/xsprintf/__tests__/PhutilCsprintfTestCase.php --- a/src/xsprintf/__tests__/PhutilCsprintfTestCase.php +++ b/src/xsprintf/__tests__/PhutilCsprintfTestCase.php @@ -16,7 +16,7 @@ 'http://domain.com/path/' => true, 'svn+ssh://domain.com/path/' => true, '`rm -rf`' => false, - '$VALUE' => false, + '$VALUE' => phutil_is_windows(), ); foreach ($inputs as $input => $expect_same) { @@ -39,14 +39,13 @@ } public function testNoPowershell() { - if (!phutil_is_windows()) { - $cmd = csprintf('%s', '#'); - $cmd->setEscapingMode(PhutilCommandString::MODE_DEFAULT); + $cmd = csprintf('%s', '#"'); + $cmd->setEscapingMode(PhutilCommandString::MODE_DEFAULT); - $this->assertEqual( - '\'#\'', - (string)$cmd); - } + $this->assertEqual( + phutil_is_windows() ? '"#\""' : '\'#"\'', + (string)$cmd + ); } public function testPasswords() { @@ -76,6 +75,9 @@ public function testEscapingIsRobust() { if (phutil_is_windows()) { + // NOTE: The reason we can't run this test on Windows is two fold: + // 1. We don't have a sane process starter in Windows + // 2. We can't use CMD because its escaping is unreliable $this->assertSkipped(pht("This test doesn't work on Windows.")); } @@ -93,4 +95,33 @@ $this->assertTrue(strpos($out, '!@#$%^&*()') !== false); } + public function testEdgeCases() { + $edgeCases = array( + "\\", + "%", + "%%", + " ", // space + "", // empty string + "-", + "/flag", + "\\\^\%\\\'\ \\", + '%PATH%', + '%XYZ%', + '%%HOMEDIR%', + "a\nb", // newline + "a\rb", // newline + "a\r\n\n\rb", // newline + "a b", + '"a b"', + '"%%$HOMEDIR%^^"', + '\'a b\'', + '^%HO ^"M\'EDIR^%^%\'', + '"\'a\0\r\nb%PATH%%`\'"\'`\'`\'', + ); + + foreach ($edgeCases as $edgeCase) { + list($output) = execx('php -r %s %s', 'echo $argv[1];', $edgeCase); + $this->assertEqual($edgeCase, $output); + } + } }