Page MenuHomePhabricator

Fix Windows escaping
AbandonedPublic

Authored by BYK on Apr 10 2016, 3:05 PM.

Details

Summary

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.

Test Plan

Included unit tests.

Diff Detail

Repository
rPHU libphutil
Branch
windows-escapes
Lint
Lint WarningsExcuse: will fix
SeverityLocationCodeMessage
Warningsrc\xsprintf\PhutilCommandString.php:49TXT3Line Too Long
Warningsrc\xsprintf\PhutilCommandString.php:65TXT3Line Too Long
Warningsrc\xsprintf\PhutilCommandString.php:72TXT3Line Too Long
Warningsrc\xsprintf\PhutilCommandString.php:84XHP9Naming Conventions
Warningsrc\xsprintf\PhutilCommandString.php:88XHP9Naming Conventions
Warningsrc\xsprintf\PhutilCommandString.php:95XHP9Naming Conventions
Warningsrc\xsprintf\PhutilCommandString.php:99XHP9Naming Conventions
Warningsrc\xsprintf\PhutilCommandString.php:103XHP9Naming Conventions
Warningsrc\xsprintf\PhutilCommandString.php:116TXT3Line Too Long
Warningsrc\xsprintf\PhutilCommandString.php:123TXT3Line Too Long
Warningsrc\xsprintf\__tests__\PhutilCsprintfTestCase.php:82TXT3Line Too Long
Warningsrc\xsprintf\__tests__\PhutilCsprintfTestCase.php:102XHP9Naming Conventions
Warningsrc\xsprintf\__tests__\PhutilCsprintfTestCase.php:122XHP9Naming Conventions
Warningsrc\xsprintf\__tests__\PhutilCsprintfTestCase.php:122XHP9Naming Conventions
Warningsrc\xsprintf\__tests__\PhutilCsprintfTestCase.php:123XHP9Naming Conventions
Warningsrc\xsprintf\__tests__\PhutilCsprintfTestCase.php:124XHP9Naming Conventions
Warningsrc\xsprintf\__tests__\PhutilCsprintfTestCase.php:129XHP9Naming Conventions
Warningsrc\xsprintf\__tests__\PhutilCsprintfTestCase.php:136XHP9Naming Conventions
Warningsrc\xsprintf\__tests__\PhutilCsprintfTestCase.php:136XHP9Naming Conventions
Warningsrc\xsprintf\__tests__\PhutilCsprintfTestCase.php:139XHP9Naming Conventions
Unit
Unit Tests OK
Build Status
Buildable 15171
Build 19931: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
BYK retitled this revision from Default to PowerShell escape mode on Windows to Fix Windows escaping.Apr 12 2016, 2:10 PM
BYK updated this object.
BYK edited the test plan for this revision. (Show Details)
BYK updated this object.
BYK added inline comments.Apr 12 2016, 2:16 PM
src/future/exec/ExecFuture.php
344–345

This fix should probably be in its own patch but I needed it to get my tests passing.

431–440

Same as my previous inline comment. This is a separate issue which I needed to fix to get tests running properly on Windows.

769–772

closeProcess already closes all pipes so this is redundant.

src/future/exec/PhutilExecPassthru.php
91

We need this mode for all 'bypass_shell' => true. The current implementation may not be the greatest way to achieve this.

95

This change of order was needed because of the mode change above. Could have kept the ordering and added another phutil_is_windows but that seemed wasteful.

src/future/exec/__tests__/ExecFutureTestCase.php
8

I think I should replace these with php -r %s. LMK if you agree and I'll make the change.

src/xsprintf/__tests__/PhutilCsprintfTestCase.php
47

Well, this is not really accurate. I should probably branch and use the expected Windows version at this point. What do you think?

BYK added inline comments.Apr 12 2016, 2:18 PM
src/future/exec/ExecFuture.php
431–440

Btw. context on this change: http://stackoverflow.com/a/28045959/90297

BYK added inline comments.Apr 12 2016, 2:20 PM
src/xsprintf/__tests__/PhutilCsprintfTestCase.php
126

Should probably do

list($output) = execx('php -r %s -- %s', 'echo $argv[1];', $edgeCase);

instead.

BYK added a subscriber: dancol.Apr 12 2016, 2:36 PM

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

BYK planned changes to this revision.Apr 12 2016, 9:01 PM

I've noticed an issue with passthru mode. I'll investigate and update the patch accordingly.

BYK requested a review of this revision.Apr 13 2016, 6:09 AM

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.

BYK updated this revision to Diff 37876.Apr 15 2016, 8:51 AM
  • Rebase
  • Add comment about taskkill change
  • Slightly better php invocations in tests
  • Enable one more test for Windows
BYK added a comment.Apr 15 2016, 8:53 AM

@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.

BYK updated this revision to Diff 37877.Apr 15 2016, 8:56 AM
  • Actually add comment about taskkill change
  • Safer PHP invocation for edge case test

what could go wrong

whatcouldgowrongindoorpooledition

lgtm (but I haven't tested it)

src/future/exec/ExecFuture.php
345

We can probably enable these by default now (i.e. turn on Windows file streams by default instead of explicitly turning it on here).

We've been using them for ages and they work fine.

BYK added a comment.Apr 15 2016, 9:09 AM

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.

src/future/exec/ExecFuture.php
345

Not turning them on unless necessary avoid temporary file creation etc. so I'm unsure. That said I'm okay if you think that's a good idea.

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.

BYK added a comment.Apr 15 2016, 9:58 AM

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.)

BYK added a comment.Apr 15 2016, 1:10 PM

(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.

BYK updated this revision to Diff 38112.Apr 29 2016, 3:58 PM

rebase

mwwade added a subscriber: mwwade.May 10 2016, 9:07 PM
hach-que resigned from this revision.
BYK updated this revision to Diff 38850.Jun 20 2016, 10:35 AM

rebase

BYK added a comment.Jun 20 2016, 10:36 AM

@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.

epriestley edited edge metadata.Jun 20 2016, 12:48 PM
epriestley requested changes to this revision.
epriestley added inline comments.
src/xsprintf/__tests__/PhutilCsprintfTestCase.php
101

These test cases do not test what they intend to test.

In PHP, '\0' is the string literal "backslash, zero", not a null byte.

Likewise, '\n' and '\r' are not newline characters.

When these strings are changed to use double quotes, the test cases fail.

This revision now requires changes to proceed.Jun 20 2016, 12:48 PM
BYK added inline comments.Jun 20 2016, 12:54 PM
src/xsprintf/__tests__/PhutilCsprintfTestCase.php
101

Ah, okay I see. Will fix.

BYK added inline comments.Jun 20 2016, 6:55 PM
src/xsprintf/__tests__/PhutilCsprintfTestCase.php
101

Turns out it is not possible to pass a NULL BYTE through the command line anyways so gonna drop that case. Working on the new lines which are the other two problems.

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.

BYK added a comment.Jun 20 2016, 6:58 PM

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.

hach-que added inline comments.Jun 20 2016, 10:59 PM
src/xsprintf/PhutilCommandString.php
49

There should be MODE_WIN_CMD to force Windows CMD escaping for e.g. when Drydock is executing a command on a remote Windows host (in this scenario, phutil_is_windows returns false because the Phabricator host is Linux).

BYK added inline comments.Jun 21 2016, 6:10 AM
src/xsprintf/PhutilCommandString.php
49

Good point. That said where should I change to make Drydock use that mode? Also in that case does MODE_POWERSHELL become obsolete?

hach-que added inline comments.Jun 21 2016, 6:34 AM
src/xsprintf/PhutilCommandString.php
49

No need to change Drydock. Adding a specific mode will just future proof this code.

BYK edited edge metadata.Jun 22 2016, 10:23 PM
BYK updated this revision to Diff 38899.
  • Remove CMD.exe support and fix edge cases
BYK updated this object.Jun 22 2016, 10:23 PM
BYK edited edge metadata.

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.

src/future/exec/ExecFuture.php
647

This will break anything that doesn't specify an absolute path to the binary program, and will break any invocations that make use of shell features such as &&. This is not the correct fix.

src/xsprintf/PhutilCommandString.php
49

Dropping CMD now (apart from the other breakage) will mean we have to come back and add it later, because you can't remotely execute without cmd.

hach-que requested changes to this revision.

(requesting changes because the proc_open is clearly wrong)

This revision now requires changes to proceed.Jun 22 2016, 10:42 PM
BYK added inline comments.Jun 23 2016, 10:28 AM
src/future/exec/ExecFuture.php
647

It actually didn't break things that are not specifying an absolute path (see my tests below using just php) that said you have a valid point with &&. I just don't want to rely on CMD.exe's horribly broken stuff but looks like the solution you want is to simply throw an exception when it sees a new line.

Technically LF should work where as all CRs are removed automatically but I can't even get LF working for some reason.

src/xsprintf/PhutilCommandString.php
49

because you can't remotely execute without cmd.

I'm not sure if I understand what is behind this. Can you elaborate?

BYK edited edge metadata.Jun 23 2016, 11:19 AM
BYK updated this revision to Diff 38900.
  • Feedback
OCram added a subscriber: OCram.Jul 26 2016, 7:48 AM
BYK edited edge metadata.Oct 4 2016, 8:14 AM
BYK updated this revision to Diff 40106.

rebase

BYK added a comment.Oct 4 2016, 8:18 AM

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 updated this revision to Diff 40322.Oct 21 2016, 7:32 AM

rebase

BYK updated this revision to Diff 41284.Jan 9 2017, 9:39 PM

rebase

@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?

hach-que resigned from this revision.Apr 3 2017, 11:51 PM
This revision now requires changes to proceed.Apr 3 2017, 11:51 PM
BYK added a comment.Jan 8 2018, 2:48 PM

@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.

BYK abandoned this revision.Jan 8 2018, 3:02 PM

Alright! You know where to find me or this patch if interest arises :)

Thanks again for the earlier reviews and guidance.