I guess the author of the article I referenced is @dancol ?

@epriestley, @hach-que - I think this is in a very good state for a review now. The only thing I'm unsure is whether to enable bypass_shell by default since I don't see a reason to use the cmd.exe proxy. Its commands are not universal and it requires another layer of escaping which is confusing and probably prone to errors, especially when things get nested.

what could go wrong

lgtm (but I haven't tested it)

lgtm (but I haven't tested it)

Note that this patch should go hand in hand with D15693 otherwise we'll break arc for all Windows users.

The flag was initially added to minimize the impact of fixing the broken streaming that Windows has by default. Without this flag on, you can't actually stream data from a process's standard output to PHP in realtime; you have to wait for the output to be fully buffered.

The flag was initially added to minimize the impact of fixing the broken streaming that Windows has by default. Without this flag on, you can't actually stream data from a process's standard output to PHP in realtime; you have to wait for the output to be fully buffered.

Yup, I had the misfortune of tracing and debugging this to get the tests passing for this patch :D

Yeah, that's why I think it's probably time to just turn it on by default.

(I'm looking forward to reviewing this but probably won't get to it for at least a few days since I want to test it thoroughly myself.)

(I'm looking forward to reviewing this but probably won't get to it for at least a few days since I want to test it thoroughly myself.)

It's okay. I realized you're busy with the scalability stuff :) Thanks for the note though.

Bump

@epriestley if you are willing to give this another shot I'll try to fix the test error. Otherwise all my hard work seems to be going into a blackhole.

If there's no way to escape it, we should make it throw, and have a test case to make sure it throws. It's OK if we don't handle it, I just want to have no cases where we silently do the wrong thing.

If there's no way to escape it, we should make it throw, and have a test case to make sure it throws. It's OK if we don't handle it, I just want to have no cases where we silently do the wrong thing.

Makes sense. This is not possible on Unix either so we may wanna address that case separately since not sure if the current implementation throws or now.

I've decided to just drop the CMD.exe support since it is crazy and unreliable when it comes to escaping edge cases. Also it needs double escaping anyways (ArgV + Crazy-ass CMD escaping).

It's unclear to me the value of this change if it doesn't handle CMD escaping.

As far as I'm also aware, it would be fine to throw an exception when the CMD encoding encounters a value that it can't escape - indeed this is the behavior that @epriestley is explicitly asking for. You'd be better off doing that than saying it's unreliable, not implementing it, and then having things break in various other subtle ways.

I'd love to get this merged or concluded instead of trying to keep up with rebases and push it forward. I have been using this branch for quite a while locally and as far as I can tell, there are no problems.

@BYK Thanks for the patch! I feel your pain on this -- I had to do something similar at work, namely get a sequence of arguments passed through two levels of escaping hell (CreateProcess + cmd.exe).

One corner case we ran into that you'll want to handle wrt cmd.exe escaping is that the space character needs the same ^-escaping as the documented cmd.exe meta-characters, but only if it's in the first argument. This doesn't seem to be documented in that MSDN article. (I observed this on a fully-patched Windows 7 x64 -- never tried with other versions.)

So if you're trying to launch: C:\Program Files\foo.exe, bar baz
The string you pass to CreateProcess needs to be: cmd /C ^"C:\Program^ Files\foo.exe^" ^"bar baz^"

If you don't do this, the process might appear to launch successfully but be unable to tokenise its own arguments properly because cmd.exe itself called CreateProcess with a malformed string. (You can verify what's actually being passed to CreateProcess using Sysinternals Process Monitor.)

One thing I noticed is that arc browse is still broken on Windows, because it tries to run start http://SOME_URL where start is a cmd.exe builtin, not an executable, and PhutilExecPassthru sets bypass_shell to true.

I don't know much about the internals of Phabricator, but I wonder if PhutilExecPassthru.php should really be forcing bypass_shell=true on Windows, the scary comment notwithstanding. Other than cmd.exe's shorter command limit, inability to accept certain characters (e.g. line breaks) in arguments, and the psychic pain it causes you when you try to figure out its escaping rules, is there a reason not to use it?

@epriestley is the best course of action for this diff to abandon it?

Probably, yes. Our modern roadmap is almost entirely driven by paying customers, and no customers have expressed interest in this.