Page Menu
Home
Phabricator
Search
Configure Global Search
Log In
Files
F14076514
D15675.id38850.diff
No One
Temporary
Actions
View File
Edit File
Delete File
View Transforms
Subscribe
Mute Notifications
Award Token
Flag For Later
Size
12 KB
Referenced Files
None
Subscribers
None
D15675.id38850.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
@@ -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 {
@@ -762,11 +759,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) {
@@ -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_PASSTHRU = 'winargv';
const MODE_POWERSHELL = 'powershell';
public function __construct(array $argv) {
@@ -48,14 +49,80 @@
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);
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;
+ }
+
+ /**
+ * 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) {
+ // 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
@@ -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,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,33 @@
$this->assertTrue(strpos($out, '!@#$%^&*()') !== false);
}
+ public function testEdgeCases() {
+ $edgeCases = array(
+ '\0', // null byte
+ '\\',
+ '%',
+ '%%',
+ ' ', // space
+ '', // empty string
+ '-',
+ '/flag',
+ '\\\^\%\\\'\ \\',
+ '%PATH%',
+ '%XYZ%',
+ '%%HOMEDIR%',
+ '\n', // newline
+ '\r', // 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);
+ }
+ }
}
File Metadata
Details
Attached
Mime Type
text/plain
Expires
Fri, Nov 22, 5:49 PM (15 h, 38 m)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
6775508
Default Alt Text
D15675.id38850.diff (12 KB)
Attached To
Mode
D15675: Fix Windows escaping
Attached
Detach File
Event Timeline
Log In to Comment