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 @@ -341,6 +341,8 @@ * @task config */ public function setTimeout($seconds) { + // Timeout requires non-blocking streams + $this->setUseWindowsFileStreams(true); $this->terminateTimeout = $seconds; $this->killTimeout = $seconds + min($seconds, 60); return $this; @@ -426,7 +428,14 @@ public function resolveKill() { if (!$this->result) { $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, @@ -596,18 +605,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 { @@ -766,11 +763,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 @@ -86,6 +86,10 @@ // 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_PASSTHRU); + } } $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) { @@ -106,7 +115,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 { @@ -123,7 +132,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,16 +6,14 @@ private $escapingMode = false; const MODE_DEFAULT = 'default'; + const MODE_WIN_PASSTHRU = 'winargv'; + const MODE_WIN_CMD = 'wincmd'; const MODE_POWERSHELL = 'powershell'; public function __construct(array $argv) { $this->argv = $argv; $this->escapingMode = self::MODE_DEFAULT; - - // This makes sure we throw immediately if there are errors in the - // parameters. - $this->getMaskedString(); } public function __toString() { @@ -48,14 +46,90 @@ public static function escapeArgument($value, $mode) { switch ($mode) { case self::MODE_DEFAULT: - return escapeshellarg($value); + return phutil_is_windows() ? self::escapeWindowsCMD($value) : escapeshellarg($value); case self::MODE_POWERSHELL: return self::escapePowershell($value); + case self::MODE_WIN_PASSTHRU: + return self::escapeWindowsArgv($value); + case self::MODE_WIN_CMD: + return self::escapeWindowsCMD($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) { + if (strpos($value, "\0") !== false) { + throw new UnexpectedValueException(pht("Can't pass NULL BYTE in command arguments!")); + } + + // 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; + } else if ($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; + } + + /** + * Escapes all CMD metacharacters with a `^`. + * + * 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 CMD.exe + */ + private static function escapeWindowsCMD($value) { + if (preg_match('/[\n\r]/', $value)) { + throw new UnexpectedValueException(pht("Can't pass line breaks to CMD.exe!")); + } + + // Make sure this is CreateProcess-safe first + $value = self::escapeWindowsArgv($value); + + // Now prefix all CMD meta characters with a `^` to escape them + return preg_replace('/[()%!^"<>&|]/', '^$0', $value); + } + 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,8 @@ 'http://domain.com/path/' => true, 'svn+ssh://domain.com/path/' => true, '`rm -rf`' => false, - '$VALUE' => false, + '$VALUE' => phutil_is_windows(), + '%VALUE%' => !phutil_is_windows(), ); foreach ($inputs as $input => $expect_same) { @@ -39,14 +40,12 @@ } 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() { @@ -57,7 +56,7 @@ // "%P" takes a PhutilOpaqueEnvelope. $caught = null; try { - csprintf('echo %P', 'hunter2trustno1'); + csprintf('echo %P', 'hunter2trustno1')->getMaskedString(); } catch (Exception $ex) { $caught = $ex; } @@ -76,6 +75,12 @@ public function testEscapingIsRobust() { if (phutil_is_windows()) { + // NOTE: The reason we can't run this test on Windows is two fold: + // 1. We need to use both `argv` escaping and `cmd` escaping + // when running commands on Windows because of the CMD proxy + // 2. After the first `argv` escaping, you only need CMD escaping + // but we need a new `%x` thing to signal this which is probably + // not worth the added complexity. $this->assertSkipped(pht("This test doesn't work on Windows.")); } @@ -93,4 +98,51 @@ $this->assertTrue(strpos($out, '!@#$%^&*()') !== false); } + public function testEdgeCases() { + $edgeCases = array( + '\\', + '%', + '%%', + ' ', // space + '', // empty string + '-', + '/flag', + '\\\^\%\\"\ \\', + '%PATH%', + '%XYZ%', + '%%HOMEDIR%', + '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); + } + } + + public function testThrowingEdgeCases() { + $edgeCases = array( + "\0", + "\n", + "\r", + "\n\r\n", + ); + + foreach ($edgeCases as $edgeCase) { + $caught = null; + try { + $cmd = csprintf('echo %s', $edgeCase); + $cmd->setEscapingMode(PhutilCommandString::MODE_WIN_CMD); + $cmd->getMaskedString(); + } catch (Exception $ex) { + $caught = $ex; + } + $this->assertTrue($caught instanceof UnexpectedValueException); + } + } }