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

Escaping quotes - portable way #15812

Closed
krwq opened this issue Nov 30, 2015 · 22 comments
Closed

Escaping quotes - portable way #15812

krwq opened this issue Nov 30, 2015 · 22 comments
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Diagnostics.Process
Milestone

Comments

@krwq
Copy link
Member

krwq commented Nov 30, 2015

How do you escape quotes the portable way when using Process with UseShellExecute and Arguments?

EDIT:
UseShellExecute:

System.PlatformNotSupportedException:  UseShellExecute must always be set to false.
   at System.Diagnostics.ProcessStartInfo.set_UseShellExecute(Boolean value)

Process could perhaps have an option of taking string[] to match Main and be able roundtrip

@stephentoub
Copy link
Member

Why would anyone want to start an app as shell does it, huh?

I'm going to ignore your sarcasm.

What exactly do you expect to happen on Linux and OS X?

@pallavit
Copy link
Contributor

Why would anyone want to start an app as shell does it, huh?

The reason Process does not support UseShellExecute is because there is no good way to support it in OneCore. The underlying windows APIs are not available. Also, as Stephen mentioned what do you expect the behavior to be cross-plat?

@pallavit
Copy link
Contributor

pallavit commented Dec 1, 2015

@krwq Let me know your thoughts on this? Since we are looking at the surface area gap, I would like to make sure, I am accounting for any issues raised.

@ghost
Copy link

ghost commented Dec 1, 2015

Perhaps some related information can be found here: http://developer.xamarin.com/api/property/System.Diagnostics.ProcessStartInfo.UseShellExecute/.

Probably can be accomplished by storing list of known terminals var KnownShells = {"/usr/bin/terminal", "/usr/bin/gnome-terminal", "/usr/bin/knosole"} and then create a virtual tty listener for communication between .NET process and terminal. The execution would be spawning a child process with available terminal and arguments. This can also be restricted to desktop context in Unix as in Windows. Would be nice to also let user override the terminal choice using an environment variable say CORECLR_TERMINAL=/usr/bin/gnome-terminal in case system has multiple choices and the selection based on KnownShells list precedence is not what user prefers.

@pallavit
Copy link
Contributor

pallavit commented Dec 2, 2015

Would be nice to also let user override the terminal choice using an environment variable say CORECLR_TERMINAL=/usr/bin/gnome-terminal

Thanks for the suggestion. However what you are asking for seems like a new functionality rather than a behavior for UseShellExecute.

@krwq Unfortunately, I don't think we can bring forward UseShellExecute due to platform limitations. However, if you would like Process to support a scenario, please propose a new API and I will be happy to take a look at it.

@krwq
Copy link
Member Author

krwq commented Dec 2, 2015

For the ShellExecute from the name means execute as default shell does it on a given OS so what @jasonwilliams200OK is exactly what I was expecting would happen. Something similar to what would happen in c++ when I called system("command args")

For the quotes - each shell has some mechanism of escaping quotes. I'm expecting either string[] overload so that the problem dissappears or something for escaping quotes - if we know how to split the string we should know how to join it

@mellinoe
Copy link
Contributor

mellinoe commented Dec 2, 2015

if we know how to split the string we should know how to join it

We don't really know how to do that; the command line arguments are already parsed when they gets to us (in the form of an array). Is that what you meant? I recall that we decided against against exposing Environment.CommandLine because it wasn't possible to reconstruct the "original arguments" used to create the process. I think you're asking for something similar to that, like a method that would take an array of strings and launch a process with those arguments, meaning we'd have to reconstruct a single (properly-escaped) string out of that array to start the process with.

@krwq
Copy link
Member Author

krwq commented Dec 2, 2015

We can use:
http://linux.die.net/man/3/execvp
for shell execute version and all problems disappear

@pallavit
Copy link
Contributor

pallavit commented Dec 2, 2015

@krwq How would we implement this on windows? The OS does not support the necessary functionality on OneCore.

@krwq
Copy link
Member Author

krwq commented Dec 3, 2015

We have Windows source code and we can check what they are doing in CommandLineToArgvW and reverse the process

We actually have this: https://msdn.microsoft.com/en-us/library/xwy0k9bb.aspx
and either check the source code or use that directly

@krwq
Copy link
Member Author

krwq commented Dec 3, 2015

Also all the rules are here:
http://www.robvanderwoude.com/escapechars.php

@pallavit
Copy link
Contributor

pallavit commented Dec 3, 2015

Sorry now I am a little confused now. Are you asking for UseShellExecute or do you simply want a way to pass arguments to the Process? The issue you created is a little wage.

@krwq
Copy link
Member Author

krwq commented Dec 3, 2015

I want to be able to use argv[] and be able to pass that or part of it to ShellExecute (or some platform independent way of escaping characters so i can do it myself)

@pallavit
Copy link
Contributor

pallavit commented Dec 3, 2015

Ahh thanks I understand the scenario a little bit more. Environment.GetCommandLineArgs() does provide you a way to get argv[] although its implementation is host specific i.e. it is dependent on how host is configured and what it passes to the runtime as process arguments. That should do most of what you are wanting to do.

Using CommandLineToArgvW in a hosted app is risky. It simply will give you the complete commandLine that was passed to the windows. It has no knowledge of what and how host consumes it. And in a ton of scenarios like in an f# script or PS script, the developer also does not know what the host does, because all that happens in the background and is hidden from him.

The part where you want to pass these arguments to ShellExecute is something that can't be supported. However, we can support passing these arguments to CreateProcess, which I believe will also do the trick for you. Is that right?

@krwq
Copy link
Member Author

krwq commented Dec 3, 2015

Thanks, passing argv to CreateProcess sounds good to me and I think that should unblock most of the common scenarios. 😃 I was thinking of ShellExecute as something similar to running "cmd /c commandline" on Windows without the host scenarios in my head.

@joshfree joshfree assigned Priya91 and unassigned pallavit Apr 26, 2016
@Priya91 Priya91 removed their assignment Dec 7, 2016
@Priya91
Copy link
Contributor

Priya91 commented Dec 7, 2016

We need an api proposal for this, marking it as up for grabs..

@krwq
Copy link
Member Author

krwq commented Dec 8, 2016

Here is example missing scenario for further reference:

  • get all args in Main, parse first and remove it from the list
  • execute another app based on those args.
    Examples:
dotnet run <args> - we parse "run" but want to pass on `<args>`
git add <args>

It is not simply string.Join as some of the args require escaping, quotes etc and the syntax might be different depending on OS. The worst possible test case scenario is probably passing regex with spaces as it can contain any different kind of characters like ^ which is used as escape character on windows

@krwq
Copy link
Member Author

krwq commented Dec 13, 2016

@Priya91 - I actually think that this is more broken than I thought:
http://stackoverflow.com/questions/5510343/escape-command-line-arguments-in-c-sharp

I hit this today - basically when you try to pass the path wrapped with quotes and ending with \ it is not working as expected

krwq referenced this issue in dotnet/cli Dec 13, 2016
@Priya91
Copy link
Contributor

Priya91 commented Jun 21, 2017

Here's a usecase for the scenario: dotnet/corefx#21196 (comment)

Also sample parsing code: https://github.com/AtsushiKan/test/blob/master/Paste.cs

@TSlivede
Copy link

TSlivede commented Aug 29, 2017

I beleave https://github.com/dotnet/corefx/issues/23592 is closely related. It's not about UseShellExecute but about avoiding to quote arguments and instead passing a list of arguments directly.

Regarding UseShellExecute:
I think, use shell execute should not call anything in the terminal - that's not what "shell" means - most desktop computers currently have a GUI shell. I'm aware, that on linux "shell" most often means "command line shell" but UseShellExecute was never about that. Instead UseShellExecute allows the shell (on Windows: shell32.dll not cmd.exe) to do some intermediate stuff, like interpreting the ProcessStartInfo.Verb property and deciding whether the given path actually belongs to an application or if it's just the path to a file which should be opened by another application. Test app:

using System;
using System.Diagnostics;

public class MainClass{
	public static int Main(string[] args){
		ProcessStartInfo startInfo = new ProcessStartInfo(args[0]);
		startInfo.UseShellExecute=true;
		Process.Start(startInfo);
		startInfo.UseShellExecute=false;
		Process.Start(startInfo);
		return 0;
	}
}

Passing notepad.exe as argument opens notepad twice, while passing path\to\file.txt opens notepad once and geves an error if UseShellExecute is false. Notice: In neither case any console window is opened and passing a cmd command like dir does not succeed.

EDIT:
I just saw https://github.com/dotnet/corefx/issues/19956 which suggests the right thing:

This tracks improving Unix implementation to copy Mono instead of just launching with sh.

@TSlivede
Copy link

Maybe this should be closed as duplicate of https://github.com/dotnet/corefx/issues/19956 or https://github.com/dotnet/corefx/issues/23592

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 5.0 milestone Jan 31, 2020
@stephentoub
Copy link
Member

Closing as dup.

@ghost ghost locked as resolved and limited conversation to collaborators Jan 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Diagnostics.Process
Projects
None yet
Development

No branches or pull requests

7 participants