Differential D15675
Fix Windows escaping Authored by BYK on Apr 10 2016, 3:05 PM. Tags None Referenced Files
Details
Default escaping mode in PhutilCommandString uses escapeshellarg which does non-sensical things on Windows such as replacing % with a space. This patch adds proper ArgV escaping for Windows using the information from https://blogs.msdn.microsoft.com/twistylittlepassagesallalike/2011/04/23/everyone-quotes-command-line-arguments-the-wrong-way/ Fixes T8298. Included unit tests.
Diff Detail
Event TimelineThere are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions I've noticed an issue with passthru mode. I'll investigate and update the patch accordingly. Comment Actions Okay, the patch was good. It was arc trying to overcompensate for Windows mania so another patch to arc is needed and it is on its way. Without that patch, arc gets broken on Windows with only this patch. Comment Actions
Comment Actions @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. Comment Actions what could go wrong lgtm (but I haven't tested it)
Comment Actions
Note that this patch should go hand in hand with D15693 otherwise we'll break arc for all Windows users.
Comment Actions 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. Comment Actions
Yup, I had the misfortune of tracing and debugging this to get the tests passing for this patch :D Comment Actions (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.) Comment Actions
It's okay. I realized you're busy with the scalability stuff :) Thanks for the note though. Comment Actions @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.
Comment Actions 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. Comment Actions
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.
Comment Actions 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). Comment Actions 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.
Comment Actions 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. Comment Actions @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 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.) Comment Actions 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? Comment Actions Probably, yes. Our modern roadmap is almost entirely driven by paying customers, and no customers have expressed interest in this. Comment Actions Alright! You know where to find me or this patch if interest arises :) Thanks again for the earlier reviews and guidance. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||