Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use %COMSPEC% as the shell in Process.new(shell: true) #13567

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

HertzDevil
Copy link
Contributor

@HertzDevil HertzDevil commented Jun 15, 2023

Resolves #9030. Also effectively confirms in the specs that passing args with shell: true is not allowed.

Also effectively closes #12873, as Process.run("filedoesnotexist", shell: true) will now return Process::Status[1] instead of raising. (The exit codes are still different across platforms but I doubt anything further could be done about that.)

Unsetting %COMSPEC% is now a hard error, although under normal circumstances nobody should attempt to do that on Windows. It is needed because we do not look up cmd.exe in %PATH%, but the system drive's letter name is not fixed, i.e. we cannot simply assume C:\...\cmd.exe is correct.

@HertzDevil HertzDevil added kind:feature platform:windows Windows support based on the MSVC toolchain topic:stdlib:system labels Jun 15, 2023
@HertzDevil
Copy link
Contributor Author

Right now the command is not escaped at all. It seems this would suffice except for newlines? Should we do it?

Copy link
Member

@oprypin oprypin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change gets a veto from me.
I have expressed on the issue that this is not the way to go. Let me elaborate further.

Upsides of this change:

  1. You can write echo %cd% instead of cmd /c echo %cd%

Downsides of this change:

  1. There is no longer any way to pass a native unprocessed arg string to a Windows process, it has to be one produced from a list of strings or from cmd.exe. This makes some valid exotic inputs non-representable.
  2. There is no longer any part of standard library that makes sense to use with the output of Process.quote_windows. It may as well be deprecated. Any usages of Process.quote_windows (or any .quote on Windows) that exist in Crystal's own codebase (except this single one in src/crystal/system/win32/process.cr) are now wrong and dangerous.
  3. Process.quote used to be always the right thing to put into system( ) / ` `, now it is dangerous on Windows. There is no possible replacement for it with this change. There is no way to safely quote anything.
  4. The compiler immediately receives security holes. For example:

@oprypin
Copy link
Member

oprypin commented Jun 17, 2023

@HertzDevil

Right now the command is not escaped at all. It seems this would suffice except for newlines? Should we do it?

Actually the article seems to suggest that in cmd shell, the ^ can escape anything, including newlines.

If so, this part of my message is wrong:

There is no possible replacement for it with this change. There is no way to safely quote anything.

Then just updating the quote_windows implementation would resolve most of my concerns.

@straight-shoota
Copy link
Member

So I suppose to move this further, we should adapt Process.quote_windows to use ^ as escape character?

Then we can merge this and hopefully escaping should work correctly? We'll probably need some specs for that.

@HertzDevil
Copy link
Contributor Author

If I understand correctly, you want `command #{Process.quote("args")}` to behave identically to Process.run("command", ["args"]) even when args contains any shell metacharacters. Surely applying the ^ escape sequence inside .spawn would work, but then it makes things like `echo %CD%` unusable, as the % cannot be passed to cmd.exe unescaped. (Process.quote_windows doesn't make sense here as echo doesn't use CommandLineToArgvW.)

Applying ^ to .quote_windows universally would break use cases where the arguments aren't eventually passed to .spawn as a single concatenated String. I am not 100% sure those use cases don't exist.

IMHO if security is a concern, one simply should not use the backtick in the first place. It has never occurred to me that the standard library should guarantee `command #{Process.quote("args")}` works; just because it is doable in a POSIX shell doesn't mean we could say the same for CMD or any other shell we wish to support. There is not even a guarantee that this call can be equivalently represented by a single Process.run. Moving away from the backtick would necessitate a safe replacement in the macro language (#11456).

@straight-shoota
Copy link
Member

I'm not quite sure I can grasp the dilemma here.

It has never occurred to me that the standard library should guarantee `command #{Process.quote("args")}` works; just because it is doable in a POSIX shell doesn't mean we could say the same for CMD or any other shell we wish to support.

It would seem that Process.quote makes exactly that promise though:

This is then safe to pass as part of the command when using shell: true or system().

Whether that promise was correct in the first place might be a different question but I suppose we should try to follow it.

Applying ^ to .quote_windows universally would break use cases where the arguments aren't eventually passed to .spawn as a single concatenated String. I am not 100% sure those use cases don't exist.

Could you elaborate on why such use cases would break?

@HertzDevil
Copy link
Contributor Author

There is indeed one breakage. When the compiler prepares the linker arguments for MSVC, if they are too long they are written to "#{output_dir}/linker_args.txt". Since it is MSVC that reads them, not cmd.exe, the ^ escape sequences must not be written to this temporary file. However, the existing .quote_windows must still be applied for proper tokenization.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:feature platform:windows Windows support based on the MSVC toolchain topic:stdlib:system
Projects
Status: Review
3 participants