Page MenuHomePhabricator
Files F13969803

D15675.diff
No OneTemporary
Actions

D15675.diff
View Options

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);
+ }
+ }
}

File Metadata

Mime Type
text/plain
Expires
Fri, Oct 18, 3:56 AM (3 w, 22 m ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
6724565
Default Alt Text
D15675.diff (13 KB)

Event Timeline

Phabricator · Built by Phacility