-
Notifications
You must be signed in to change notification settings - Fork 17.6k
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
os/exec: Cannot execute command with space in the name on Windows, when there are parameters #17149
Comments
I started following this down through |
Go encodes child process parameters in a way that is understood by most programs. Go uses rules similar to what CommandLineToArgvW implements. Unfortunately, your child process is cmd.exe (cmd.exe is called to execute the batch file you've requested). And cmd.exe parses its input parameters differently. You can read this very long post http://daviddeley.com/autohotkey/parameters/parameters.htm#WIN about it all.
You should stop using exec.Command to build your child process command line, and build it yourself (as per rules described in the doco I mentioned). Then you can pass raw command line to your child process by setting exec.Cmd.SysProcAttr to syscall.SysProcAttr with CmdLine set appropriately. Maybe we could fix this problem by changing Go encoding algorithm to use cmd.exe rules every time we execute batch file. There is more of the same discussion on issue #15566. Alex |
I guess this should be fixed by small change. diff --git a/src/syscall/exec_windows.go b/src/syscall/exec_windows.go
index cafce1e..a0e1f56 100644
--- a/src/syscall/exec_windows.go
+++ b/src/syscall/exec_windows.go
@@ -91,6 +91,9 @@ func makeCmdLine(args []string) string {
}
s += EscapeArg(v)
}
+ if s != "" && s[0] == '"' && len(args) > 1 {
+ s = `"` + s + `"`
+ }
return s
}
If the first argument contain spaces, and it have arguments (no matter if the argument not contains spaces), whole of string should be quoted.
|
CL https://golang.org/cl/32490 mentions this issue. |
Postponing decisions about this to Go 1.9. |
+1 This is still an issue here, also see: docker/machine#3152 |
@cannect docker/machine#3152 title mentions "powershell", but this issue is about running batch files. How do you know that if we make running batch file work, that it will fix docker/machine#3152 ? Alex |
As you can see here: docker/machine#3152 (comment) there is some confidence we think it is related with this issue. |
@cannect do you know of a simple way to reproduce docker/machine#3152 ? Alex |
Hi @alexbrainman , I had exact the same issues as described in the first post of docker/machine#3152.
Unfortunately I have no experience whatsoever with Go. |
@cannect thank you very much for these steps. But I have never used Docker for Windows. How do I install it and use it?
How do I install it? What are the steps?
What are the commands I should run to reproduce this? What did you do? What did you expect to see? What did you see instead? Thank you Alex |
I am not working on this issue, if that is what you are asking. Alex |
For anyone needing a more concrete example of how to do the manual path construction, it works something like this:
Note that you're on the hook to have everything on the CmdLine escaped/quoted/etc properly, which can be a real chore. |
Here's an updated version of @ItsMattL's workaround: // command to execute, may contain quotes, backslashes and spaces
var commandLine = `"C:\Program Files\echo.bat" "hello friend"`
var comSpec = os.Getenv("COMSPEC")
if comSpec == "" {
comSpec = os.Getenv("SystemRoot") + "\\System32\\cmd.exe"
}
childProcess = exec.Command(comSpec)
childProcess.SysProcAttr = &syscall.SysProcAttr{CmdLine: "/C \"" + commandLine + "\""}
// Then execute and read the output
out, _ := childProcess.CombinedOutput()
fmt.Printf("Output: %s", out) |
For Linux system: |
At this risk of sounding pedantic 😅, should the executable path be included in the
I'm also wondering if the arguments should be escaped when we create the cmdline ourselves as a more general solution. We might get close by using |
@rafd123 I think you are right! So, the proper code becomes: // command to execute, may contain quotes, backslashes and spaces
var commandLine = `"C:\Program Files\echo.bat" "hello friend"`
var comSpec = os.Getenv("COMSPEC")
if comSpec == "" {
comSpec = os.Getenv("SystemRoot") + "\\System32\\cmd.exe"
}
childProcess = exec.Command(comSpec)
childProcess.SysProcAttr = &syscall.SysProcAttr{CmdLine: comSpec + " /C \"" + commandLine + "\""}
// Then execute and read the output
out, _ := childProcess.CombinedOutput()
fmt.Printf("Output: %s", out) |
What do you think about something like this as a general solution? Caveats:
|
@rafd123 Looks like a good improvement. However I don't see why we would need to handle batch files differently. It's important to pass them through And yeah, you would need to follow the excellent article that you linked to, and escape meta characters with We need a Windows-specific escaping mechanism. |
Digging into this a bit further, I noticed that the dotnet core equivalent of Specifically, this works fine in dotnet: using System.Diagnostics;
Process.Start(@"C:\Program Files\echo.bat", new[] {"hello world"}); Debugging how both golang and dotnet ultimately call Windows' This is how dotnet calls This is how golang calls Empirically, I can also confirm that if I force golang to pass All that said:
|
@rafd123 Following what is done in .NET Core, and adapting it to the expected behavior in Go is probably the best we can do. Thank you for the excellent research! In your suggested |
(CC @golang/windows) |
It still exists in golang 1.20, e.g: |
This addresses golang#17149 by calling CreateProcess() in the same way that that dotnet does for its equivalent syscall abstraction since dotnet doesn't suffer from the same problem due to this difference. This change assumes that argv0 always represents the name/path of the executable and is represented as the argv[0] element per: - How exec.Command() builds exec.Cmd.Args - The following syscall/exec_unix code path: https://github.com/golang/go/blob/e8c8b79f000515e086012df632f01fc0ec21076b/src/syscall/exec_unix.go#L169-L171
This addresses golang#17149 by calling CreateProcess() and CreateProcessAsUser()in the same way that that dotnet does for its equivalent syscall abstraction since dotnet doesn't suffer from the same problem due to this difference. See the dotnet equivalent here: https://github.com/dotnet/runtime/blob/2d411c4dfc1d71b2387ac64089014ec811ad7af0/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Windows.cs#L578 ...and here: https://github.com/dotnet/runtime/blob/2d411c4dfc1d71b2387ac64089014ec811ad7af0/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Windows.cs#L554 This change assumes that argv0 always represents the name/path of the executable and is represented as the argv[0] element per: - How exec.Command() builds exec.Cmd.Args - The following syscall/exec_unix code path: https://github.com/golang/go/blob/e8c8b79f000515e086012df632f01fc0ec21076b/src/syscall/exec_unix.go#L169-L171
@bcmills @golang/windows I'd like to submit a PR that modifies TL:DR modify Concretely, this is the code change I'd like to propose: rafd123@d24b468#diff-ec673c10a75fe2d2faa9c788e03823294b263c68cc3de50f4a0865a53e28f3a3 My biggest concern with this change is that it assumes that That said, there seems to be a precendent for this assumption in Lines 169 to 171 in e8c8b79
What do you think? Is this a reasonable approach? If so, I'll submit a PR. |
This addresses golang#17149 by calling CreateProcess() and CreateProcessAsUser()in the same way that that dotnet does for its equivalent syscall abstraction since dotnet doesn't suffer from the same problem due to this difference. See the dotnet equivalent here: https://github.com/dotnet/runtime/blob/2d411c4dfc1d71b2387ac64089014ec811ad7af0/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Windows.cs#L578 ...and here: https://github.com/dotnet/runtime/blob/2d411c4dfc1d71b2387ac64089014ec811ad7af0/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Windows.cs#L554 This change assumes that argv0 always represents the name/path of the executable and is represented as the argv[0] element per: - How exec.Command() builds exec.Cmd.Args - The following syscall/exec_unix code path: https://github.com/golang/go/blob/e8c8b79f000515e086012df632f01fc0ec21076b/src/syscall/exec_unix.go#L169-L171
@rafd123, on Unix it is possible for an But maybe we could leave Or, since this quoting seems to be mostly specific to |
Change https://go.dev/cl/530275 mentions this issue: |
…discrepancies Notably, this fixes the escaping of the first argument when it contains quoted spaces, and fixes a panic in DecomposeCommandLine when it contains more than 8192 arguments. Fixes golang/go#58817. For golang/go#17149. For golang/go#63236. Change-Id: Ib72913b8182998adc1420d73ee0f9dc017dfbf32 Reviewed-on: https://go-review.googlesource.com/c/sys/+/530275 LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Quim Muntal <quimmuntal@gmail.com> Reviewed-by: Than McIntosh <thanm@google.com> Auto-Submit: Bryan Mills <bcmills@google.com>
I've been struggling with this bug and have recreated the proposed fixes in this conversation in my app. The latest proposal works well for the use case in the OP, but I think it should be noted that when using |
Based on the fuzz test added in https://go.dev/cl/530275, I believe that one part of the fix for this will be to change Another part of the fix for this issue will probably be to add a fuzz test to Finally, we should either change |
Go 1.7.1 on windows-amd64, Windows 10 latest.
Consider a test project:
main.go
contents:folder name/test.bat
contents:Expected output is two runs with "Success" in them.
Actual:
It appears that having params on a command, where the command contains a space, breaks the parsing of it. I haven't been able to work around this by experimenting with various ways of quoting the command, using backslashes or slashes, etc.
The text was updated successfully, but these errors were encountered: