Page MenuHomePhabricator

Fix Windows escaping
AbandonedPublic

Authored by BYK on Apr 10 2016, 3:05 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Mar 24, 9:30 PM
Unknown Object (File)
Mon, Mar 18, 7:02 PM
Unknown Object (File)
Mon, Mar 18, 4:36 PM
Unknown Object (File)
Fri, Mar 15, 3:00 PM
Unknown Object (File)
Fri, Mar 15, 2:12 PM
Unknown Object (File)
Fri, Mar 8, 12:41 PM
Unknown Object (File)
Fri, Mar 8, 10:46 AM
Unknown Object (File)
Feb 18 2024, 3:56 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 Passed
Unit
Tests Passed
Build Status
Buildable 12780
Build 16268: 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.
src/future/exec/ExecFuture.php
337–338

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

428–436

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

765–768

closeProcess already closes all pipes so this is redundant.

src/future/exec/PhutilExecPassthru.php
85

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
46

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

src/future/exec/ExecFuture.php
428–436

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

src/xsprintf/__tests__/PhutilCsprintfTestCase.php
123

Should probably do

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

instead.

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.

  • Rebase
  • Add comment about taskkill change
  • Slightly better php invocations in tests
  • Enable one more test for Windows

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

  • 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
338

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.

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
338

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.

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.

hach-que removed a reviewer: hach-que.

@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.
epriestley added inline comments.
src/xsprintf/__tests__/PhutilCsprintfTestCase.php
98

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
src/xsprintf/__tests__/PhutilCsprintfTestCase.php
98

Ah, okay I see. Will fix.

src/xsprintf/__tests__/PhutilCsprintfTestCase.php
98

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.

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.

src/xsprintf/PhutilCommandString.php
52

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

src/xsprintf/PhutilCommandString.php
52

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

src/xsprintf/PhutilCommandString.php
52

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

BYK edited edge metadata.
  • Remove CMD.exe support and fix edge cases
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
643

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
52

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 added a reviewer: hach-que.

(requesting changes because the proc_open is clearly wrong)

This revision now requires changes to proceed.Jun 22 2016, 10:42 PM
src/future/exec/ExecFuture.php
643

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
52

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.
  • Feedback
BYK edited edge metadata.

rebase

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?

This revision now requires changes to proceed.Apr 3 2017, 11:51 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.

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

Thanks again for the earlier reviews and guidance.