revi-archive/phorge-arcanist
1
0
Fork 0
mirror of https://we.phorge.it/source/arcanist.git synced 2024-11-08 16:02:39 +01:00
Code Issues Releases Wiki Activity

[Wilds] On Windows: always use stdout/stderr files and always "bypass_shell"

Browse source 
Summary:
Ref T13209. I'll walk through this a bit inline, but two major ideas here:

---

First, always use `bypass_shell` when calling `proc_open()`. The effect of this parameter is to use a "raw" `CreateProcessW()` Windows API call to spawn the subprocess instead of wrapping it in `cmd.exe /c ...`, which is roughly equivalent to `sh -c`.

The major advantage of adding `cmd.exe /c ...` is that you can use shell features like `> outfile.txt`.

The major disadvantage of adding `cmd.exe /c ...` is that `cmd.exe` is a terrible shell and escaping becomes absurdly complicated and position-dependent. It does not appear to be possible to escape some characters, like "\n", in argument lists passed through `cmd.exe`. It is unclear if we can safely escape environmental variables ("%APPDATA%"). PHP gives up on this and Python defaults to a `bypass_shell` equivalent with warnings about how using `shell=True` is dangerous, although they appear to understate the danger.

Since we don't currently rely on any shell features, don't plan to rely on shell features, and making program behavior generally sane should be easier without needing to fight against `cmd.exe`, always bypass it for now. We may later find that we need it in some cases (e.g. remote execution, or `arc alias` shell commands, or whatever else). If so, we can support it at that time with an elective mode on `CommandString`, but bypassing it seems to clearly be the far saner default.

---

Second, Windows doesn't have nonblocking pipes so if you try to read output from a subprocess you block until it exits. Most of the time that's not great but ends up working out okay, but when it's not fine you likely just deadlocked and hung and that's the end of you.

We can avoid this by using files instead of pipes. We've supported this mode since D11688, but never used it in the upstream. Ostensibly, it works. Make it the only mode, then rearrange it a bit so that resource management is a little tighter (for example, previously we could try to use `$pipes[0]` even after `proc_open()` failed, which meant that `$pipes` would not be populated).

It appears to //actually// work, in the sense that all the tests pass, which is promising.

Test Plan: With further changes, no tests fail on Windows, which is at least promising.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13209

Differential Revision: https://secure.phabricator.com/D19727
This commit is contained in:
epriestley 2018-10-02 10:46:37 -07:00
parent 5a4c489916
commit 0140a0e990
1 changed files with 82 additions and 70 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

152
src/future/exec/ExecFuture.php
View file

@ -42,7 +42,6 @@ final class ExecFuture extends PhutilExecutableFuture {
private $profilerCallID;
private $killedByTimeout;
private $useWindowsFileStreams = false;
private $windowsStdoutTempFile = null;
private $windowsStderrTempFile = null;
@ -181,21 +180,6 @@ final class ExecFuture extends PhutilExecutableFuture {
}
/**
* Set whether to use non-blocking streams on Windows.
*
* @param bool Whether to use non-blocking streams.
* @return this
* @task config
*/
public function setUseWindowsFileStreams($use_streams) {
if (phutil_is_windows()) {
$this->useWindowsFileStreams = $use_streams;
}
return $this;
}
/* -( Interacting With Commands )------------------------------------------ */
@ -576,6 +560,7 @@ final class ExecFuture extends PhutilExecutableFuture {
// classes are always available.
if (!$this->pipes) {
$is_windows = phutil_is_windows();
// NOTE: See note above about Phage.
if (class_exists('PhutilServiceProfiler')) {
@ -599,18 +584,6 @@ final class ExecFuture extends PhutilExecutableFuture {
$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 {
@ -627,21 +600,31 @@ final class ExecFuture extends PhutilExecutableFuture {
}
$spec = self::$descriptorSpec;
if ($this->useWindowsFileStreams) {
$this->windowsStdoutTempFile = new TempFile();
$this->windowsStderrTempFile = new TempFile();
if ($is_windows) {
$stdout_file = new TempFile();
$stderr_file = new TempFile();
$stdout_handle = fopen($stdout_file, 'wb');
if (!$stdout_handle) {
throw new Exception(
pht(
'Unable to open stdout temporary file ("%s") for writing.',
$stdout_file));
}
$stderr_handle = fopen($stderr_file, 'wb');
if (!$stderr_handle) {
throw new Exception(
pht(
'Unable to open stderr temporary file ("%s") for writing.',
$stderr_file));
}
$spec = array(
0 => self::$descriptorSpec[0], // stdin
1 => fopen($this->windowsStdoutTempFile, 'wb'), // stdout
2 => fopen($this->windowsStderrTempFile, 'wb'), // stderr
0 => self::$descriptorSpec[0],
1 => $stdout_handle,
2 => $stderr_handle,
);
if (!$spec[1] || !$spec[2]) {
throw new Exception(pht(
'Unable to create temporary files for '.
'Windows stdout / stderr streams'));
}
}
$proc = @proc_open(
@ -649,23 +632,10 @@ final class ExecFuture extends PhutilExecutableFuture {
$spec,
$pipes,
$cwd,
$env);
if ($this->useWindowsFileStreams) {
fclose($spec[1]);
fclose($spec[2]);
$pipes = array(
0 => head($pipes), // stdin
1 => fopen($this->windowsStdoutTempFile, 'rb'), // stdout
2 => fopen($this->windowsStderrTempFile, 'rb'), // stderr
);
if (!$pipes[1] || !$pipes[2]) {
throw new Exception(pht(
'Unable to open temporary files for '.
'reading Windows stdout / stderr streams'));
}
}
$env,
array(
'bypass_shell' => true,
));
if ($trap) {
$err = $trap->getErrorsAsString();
@ -674,12 +644,56 @@ final class ExecFuture extends PhutilExecutableFuture {
$err = error_get_last();
}
if ($is_windows) {
fclose($stdout_handle);
fclose($stderr_handle);
}
if (!is_resource($proc)) {
throw new Exception(
// When you run an invalid command on a Linux system, the "proc_open()"
// works and then the process (really a "/bin/sh -c ...") exits after
// it fails to resolve the command.
// When you run an invalid command on a Windows system, we bypass the
// shell and the "proc_open()" itself fails. Throw a "CommandException"
// here for consistency with the Linux behavior in this common failure
// case.
throw new CommandException(
pht(
'Failed to `%s`: %s',
'proc_open()',
$err));
'Call to "proc_open()" to open a subprocess failed: %s',
$err),
$this->command,
1,
'',
'');
}
if ($is_windows) {
$stdout_handle = fopen($stdout_file, 'rb');
if (!$stdout_handle) {
throw new Exception(
pht(
'Unable to open stdout temporary file ("%s") for reading.',
$stdout_file));
}
$stderr_handle = fopen($stderr_file, 'rb');
if (!$stderr_handle) {
throw new Exception(
pht(
'Unable to open stderr temporary file ("%s") for reading.',
$stderr_file));
}
$pipes = array(
0 => $pipes[0],
1 => $stdout_handle,
2 => $stderr_handle,
);
$this->windowsStdoutTempFile = $stdout_file;
$this->windowsStderrTempFile = $stderr_file;
}
$this->pipes = $pipes;
@ -687,11 +701,11 @@ final class ExecFuture extends PhutilExecutableFuture {
list($stdin, $stdout, $stderr) = $pipes;
if (!phutil_is_windows()) {
if (!$is_windows) {
// On Windows, we redirect process standard output and standard error
// through temporary files, and then use stream_select to determine
// if there's more data to read.
// through temporary files. Files don't block, so we don't need to make
// these streams nonblocking.
if ((!stream_set_blocking($stdout, false)) ||
(!stream_set_blocking($stderr, false)) ||
@ -769,11 +783,6 @@ final class ExecFuture extends PhutilExecutableFuture {
}
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.
@ -853,7 +862,10 @@ final class ExecFuture extends PhutilExecutableFuture {
@proc_close($this->proc);
$this->proc = null;
}
$this->stdin = null;
$this->stdin = null;
unset($this->windowsStdoutTempFile);
unset($this->windowsStderrTempFile);
if ($this->profilerCallID !== null) {
$profiler = PhutilServiceProfiler::getInstance();