Page MenuHomePhabricator

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

Authored by epriestley on Oct 2 2018, 6:25 PM.
Tags
None
Referenced Files
F13088066: D19727.diff
Thu, Apr 25, 1:16 AM
Unknown Object (File)
Sun, Apr 21, 1:42 PM
Unknown Object (File)
Fri, Apr 19, 2:37 AM
Unknown Object (File)
Tue, Apr 16, 1:55 AM
Unknown Object (File)
Thu, Apr 11, 8:50 AM
Unknown Object (File)
Tue, Apr 2, 8:47 AM
Unknown Object (File)
Tue, Apr 2, 8:47 AM
Unknown Object (File)
Sat, Mar 30, 6:56 AM
Subscribers
None

Details

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.

Diff Detail

Repository
rARC Arcanist
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

epriestley created this revision.
epriestley added inline comments.
src/future/exec/ExecFuture.php
602–612

We no longer need this since we're now bypassing cmd.exe and all its magical behaviors.

636–638

On Windows: Skip the horror that is cmd.exe.

Elsewhere: no effect, we still run sh -c ... internally.

662–669

This is mostly for consistency and also fixes one unit test which expects a CommandException from new ExecFuture('qlwkenflkqwn').

706–709

The stream_select() call in D11688 was removed before it landed, this comment was just never updated.

772–775

These are closed by closeProcess() below, so we don't need to close them here.

This revision is now accepted and ready to land.Oct 2 2018, 7:34 PM
This revision was automatically updated to reflect the committed changes.