os/exec: Cannot execute command with space in the name on Windows, when there are parameters
calmh opened this issue · 43 comments
Go 1.7.1 on windows-amd64, Windows 10 latest.
Consider a test project:
src/github.com/calmh/wincmdtest/
main.go
folder name/
test.bat
main.go contents:
package main
import (
"fmt"
"os/exec"
"strings"
)
func main() {
execCmd("./folder name/test.bat")
execCmd("./folder name/test.bat", "one param")
}
func execCmd(path string, args ...string) {
fmt.Printf("Running: %q %q\n", path, strings.Join(args, " "))
cmd := exec.Command(path, args...)
bs, err := cmd.CombinedOutput()
fmt.Printf("Output: %s", bs)
fmt.Printf("Error: %v\n\n", err)
}folder name/test.bat contents:
@echo off
echo SuccessExpected output is two runs with "Success" in them.
Actual:
C:\Users\jb\Go\src\github.com\calmh\wincmdtest>go run main.go
Running: "./folder name/test.bat" ""
Output: Success
Error: <nil>
Running: "./folder name/test.bat" "one param"
Output: '.' is not recognized as an internal or external command,
operable program or batch file.
Error: exit status 1
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.
I started following this down through syscall.StartProcess, but not really being a Windows programmer I ran into unacceptable levels of nope almost immediately so probably won't try to fix this myself, sorry. :(
It appears that having params on a command, where the command contains a space, breaks the parsing of it.
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.
I haven't been able to work around this by experimenting with various ways of quoting the command, using backslashes or slashes, etc.
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.
c:\dev\go-sandbox\space>go run main.go
Running: ".\\folder name\\test.bat" ""
Output: Success
Error: <nil>
Running: ".\\folder name\\test.bat" "one param"
Output: Success
Error: <nil>
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 ?
Is there a simple way to reproduce 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 ?
How did you personally discovered docker/machine#3152 ? Did you encountered that problem yourself?
Alex
Hi @alexbrainman ,
I had exact the same issues as described in the first post of docker/machine#3152.
I do not have the knowledge to reproduce that issue in Go. But I can tell you my steps:
- WIndows 10 Pro
- Docker for Windows
- Started two docker machines in Windows 10 bash, listed as active in
docker-machine ls - Looked those docker machines up with:
docker-machine lsin Powershell, machines not active. (but they were active of course but not listed as such in Powershell)
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?
Docker for Windows
How do I install it? What are the steps?
Looked those docker machines up with: docker-machine ls in Powershell, machines not active. (but they were active of course but not listed as such in Powershell)
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 agree with #17149 (comment) that the answer here should be to rewrite the path for argv[0] from using slash to backslash. We should try to fix this early in the Go 1.12 cycle.
Any progress on this issue?
I'd like to add that it only seems to be a problem when both the command and the parameter contain spaces. If only one or the other has a space then everything works. I've expanded on @calmh's example to demonstrate:
main.go:
package main
import (
"fmt"
"os/exec"
"strings"
)
func main() {
execCmd("C:/Users/blye/Go/src/github.com/benlye/cmdtest/folder name/test.bat")
execCmd("C:/Users/blye/Go/src/github.com/benlye/cmdtest/folder name/test.bat", "oneparam")
execCmd("C:/Users/blye/Go/src/github.com/benlye/cmdtest/folder name/test.bat", "one param")
execCmd("C:/Users/blye/Go/src/github.com/benlye/cmdtest/foldername/test.bat")
execCmd("C:/Users/blye/Go/src/github.com/benlye/cmdtest/foldername/test.bat", "oneparam")
execCmd("C:/Users/blye/Go/src/github.com/benlye/cmdtest/foldername/test.bat", "one param")
}
func execCmd(path string, args ...string) {
fmt.Printf("Running: %q %q\n", path, strings.Join(args, " "))
cmd := exec.Command(path, args...)
bs, err := cmd.CombinedOutput()
fmt.Printf("Output: %s", bs)
fmt.Printf("Error: %v\n\n", err)
}Produces only one failure (the 3rd test):
C:\Users\blye\Go\src\github.com\benlye\cmdtest>go run main.go
Running: "C:/Users/blye/Go/src/github.com/benlye/cmdtest/folder name/test.bat" ""
Output: Success
Error: <nil>
Running: "C:/Users/blye/Go/src/github.com/benlye/cmdtest/folder name/test.bat" "oneparam"
Output: Success
Error: <nil>
Running: "C:/Users/blye/Go/src/github.com/benlye/cmdtest/folder name/test.bat" "one param"
Output: 'C:/Users/blye/Go/src/github.com/benlye/cmdtest/folder' is not recognized as an internal or external command,
operable program or batch file.
Error: exit status 1
Running: "C:/Users/blye/Go/src/github.com/benlye/cmdtest/foldername/test.bat" ""
Output: Success
Error: <nil>
Running: "C:/Users/blye/Go/src/github.com/benlye/cmdtest/foldername/test.bat" "oneparam"
Output: Success
Error: <nil>
Running: "C:/Users/blye/Go/src/github.com/benlye/cmdtest/foldername/test.bat" "one param"
Output: Success
Error: <nil>
Any progress on this issue?
I don't believe anyone working on this.
I'd like to add that it only seems to be a problem when both the command and the parameter contain spaces. ...
I am pretty sure we know what the problem is (see #17149 (comment) for details). We just have not decided how to fix it.
Alex
For those stuck by this bug:
As #17149 (comment) mentioned, this problem happened only when both binary path and arguments have space. So you can write a .bat file wrapping all arguments, and then run this .bat directly.
Why not fixed?
Wow. What a bug. XD
It took me a while to figure out what was wrong, I had to use Process Monitor in order to see what Go was passing as arguments to the process, any news on a fix?
any news on a fix?
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:
cmd := exec.Command(os.Getenv("SystemRoot")+`\System32\cmd.exe`)
args := []string{`/c`, `""C:\Program Files\echo.bat"`, `"hello friend""`}
cmd.SysProcAttr = &syscall.SysProcAttr{CmdLine: " " + strings.Join(args, " ")}
out, _ := cmd.CombinedOutput()
fmt.Printf("Output: %s", out)
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:
input = strings.ReplaceAll(input, " ", "\ ")
args := []string{
"ffmpeg",
fmt.Sprintf("-i %s", input),
fmt.Sprintf("-vf %s", "scale=-1:360"),
fmt.Sprintf("-c:v %s", "libx264"),
fmt.Sprintf("-crf %s", "18"),
fmt.Sprintf("-c:a %s", "copy"),
"b.mp4",
"-y",
}
myCmd := strings.Join(args, " ")
cmd := exec.Command("bash","-c", myCmd)
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)
At this risk of sounding pedantic 😅, should the executable path be included in the CmdLine, too? Not sure if it makes a difference in practice, but reasons I ask:
- The docs for the underlying Windows API mentions it.
- If you use
exec.Commandwithout theSysProcAttrescape hatch, it automatically prepends the executable to the args (see https://github.com/golang/go/blob/master/src/os/exec/exec.go#L379). Those args ultimate are used to generate the cmdLine (see https://github.com/golang/go/blob/master/src/syscall/exec_windows.go#L302) - If you run the batch file from Windows Run and watch the procmon output, it includes the exe in the command line.
- If you run the example code and watch the procmon output, it doesn't include the exe in the command line.
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 syscall.EscapeArg on each argument, but I think cmd.exe has its own set of crazy escaping rules.
@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:
syscall.EscapeArgprobably needs to be replaced with something that is aware ofcmd.exe's escaping rules in order to be airtight; see this.- I can imagine adding handling for the problems inherent with running
msiexec, too.
exec.go
package exec
import (
"os/exec"
)
// MakeCmd returns the Cmd struct to execute the named program with
// the given arguments.
//
// It handles the case where the program is a Windows batch file
// and the implication it has on argument quoting.
func MakeCmd(name string, args ...string) (*exec.Cmd, error) {
return makeCmd(name, args...)
}exec_windows.go
package exec
import (
"fmt"
"os"
"os/exec"
"path/filepath"
"strings"
"syscall"
)
func makeCmd(name string, args ...string) (*exec.Cmd, error) {
if len(args) == 0 {
return exec.Command(name), nil
}
name, err := exec.LookPath(name)
if err != nil {
return nil, err
}
if !isBatchFile(name) {
return exec.Command(name, args...), nil
}
argsEscaped := make([]string, len(args)+1)
argsEscaped[0] = syscall.EscapeArg(name)
for i, a := range args {
argsEscaped[i+1] = syscall.EscapeArg(a)
}
shell := os.Getenv("COMSPEC")
if shell == "" {
shell = "cmd.exe" // Will be expanded by exec.LookPath in exec.Command
}
cmd := exec.Command(shell)
cmd.Args = nil
cmd.SysProcAttr = &syscall.SysProcAttr{
CmdLine: fmt.Sprintf(`%s /c "%s"`, syscall.EscapeArg(cmd.Path), strings.Join(argsEscaped, " ")),
}
return cmd, nil
}
func isBatchFile(path string) bool {
ext := filepath.Ext(path)
return strings.EqualFold(ext, ".bat") || strings.EqualFold(ext, ".cmd")
}exec_other.go
//go:build !windows
package exec
func makeCmd(name string, args ...string) (*exec.Cmd, error) {
return exec.Command(name, args...), nil
}@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 CMD.EXE too, so they need the same escaping.
And yeah, you would need to follow the excellent article that you linked to, and escape meta characters with ^. Otherwise, there is a serious security issue where the one could inject commands by using "&malicious.exe in any of the arguments.
We need a Windows-specific escaping mechanism.
Digging into this a bit further, I noticed that the dotnet core equivalent of exec.Command does not suffer from this problem. Also note that dotnet is cross-platform these days and has a similar abstraction over OS-specific APIs for creating processes.
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' CreateProcess API, I noticed one meaningful difference: dotnet passes null for the optional lpApplicationName parameter of CreateProcess while golang does not.
This is how dotnet calls CreateProcess (note the null for lpApplicationName argument).
This is how golang calls CreateProcess (note the use of argv0p...which in practice maps to the name argument of exec.Command...also note that in practice argvp will also contain the name argument of exec.Command).
Empirically, I can also confirm that if I force golang to pass nil for lpApplicationName here instead of argv0p, executing exec.Command(`C:\Program Files\echo.bat`, "hello world") works without resorting to using the SysProcAttr escape hatch.
All that said:
- Specifying
lpApplicationNameforCreateProcessis cited as a security best-practice as a fail-safe should the caller fail to add quotes around the path of the executable inlpCommandLine. FWIW, dotnet ensures that the executable is quoted for this very reason, and it turns out golang does, too, viamakeCmdLine's use ofappendEscapeArg. - I can't find any official documentation around this subtle behavior difference with
CreateProcessas to why it works when NOT specifyinglpApplicationName, so at this point it's all circumstantial. All I was able to find is someone else pointing out this suble behavioral difference way back in 2001. - It's entirely possible I'm glossing over some other important detail. I'm stabbing in the dark at this point. 😅
@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 makeCmd(), you would just need to add proper escaping for CMD.EXE. We probably need to use ^ instead of " to surround the arguments. WDYT?
(CC @golang/windows)
It still exists in golang 1.20, e.g:
cmd.exe /c copy/b "input 1.ts"+"input 2.ts" ouput.ts
I guess that golang can automatically add double quotes to paths that contain Spaces, but many commands have their own coding rules, such as copy/b. The "+" in copy/b means concatenating the input files, but golang cannot parse it and cannot add double quotes to paths of input files that contain Spaces correctly.
@bcmills @golang/windows I'd like to submit a PR that modifies syscall/exec to address this issue, and I'd like to get buyoff on the approach before doing so.
TL:DR modify syscall/exec on Windows to pass nil for the lpApplicationName arg to CreateProcess() and CreateProcessAsUser() to mimic what dotnet core does since dotnet doesn't exhibit this issue due to this difference. For full background, see this comment: #17149 (comment)
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 syscall/exec.StartProcess's argv0 is represented within argv[0]. This assumption could be violated if, for example, folks don't use os/exec.Command and instead manually construct os/exec.Cmd without including os/exec.Cmd.Path in os/exec.Cmd.Args. I already ran into a violation of this assumption in one of the tests here: rafd123@d24b468#diff-f0ee8d33c527f62f2e48195ea748d2d7295c26910144012ee6e108fbf08f21a6
That said, there seems to be a precendent for this assumption in syscall/exec_unix.go here:
Lines 169 to 171 in e8c8b79
What do you think? Is this a reasonable approach? If so, I'll submit a PR.
@rafd123, on Unix it is possible for an exec.Cmd to have an Args[0] different from its Path. I would expect the same to be possible in general on Windows too, which would seem to imply calling CreateProcessW with an ApplicationName that matches Path and a CommandLine that matches Args.
But maybe we could leave ApplicationName nil when Path is already equal to Args[0], as would be the case if the user already called exec.LookPath to resolve the path passed to exec.Command?
Or, since this quoting seems to be mostly specific to cmd.exe and commands that implicitly reinvoke %COMSPEC% (specifically, commands with the extensions .bat and .cmd), maybe we should change the quoting algorithm to match that used by cmd.exe when the resolved Path has a .bat or .cmd extension? (Are there cases where that would regress existing programs, or do we expect that those are basically all using the SysProcAttr.CmdLine workaround already?)
Change https://go.dev/cl/530275 mentions this issue: windows: convert TestCommandLineRecomposition to a fuzz test and fix discrepancies
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 cmd.exe /c cmd metachars also need to be handled (not just spaces and quotes). These need to be further escaped with ^, though for reasons I can't yet explain that needs to itself be escaped as ^^^ in the final CmdLine string used in SysProcAttrs.
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 syscall.makeCmdLine to use the same approach as was taken for windows.ComposeCommandLine in that change.
Another part of the fix for this issue will probably be to add a fuzz test to os/exec, similar to windows.FuzzComposeCommandLine but using os/exec to start a Go program that prints its arguments (compare cmd/go/internal/work.FuzzSplitPkgConfigOutput). That will ensure that os.commandLineToArgv remains consistent with syscall.makeCmdLine.
Finally, we should either change os.commandLineToArgv to use CommandLineToArgvW directly, or add a fuzz test to ensure that its implementation is consistent with some other Windows function or program (perhaps CommandLineToArgvW, cmd.exe, or some other well-defined reference program).
I'm coming here directly from arduino IDE discussion. Yes, that IDE still doesn't work for people that have 2-part username with space in Windows, as path contain a space and we get an error while copying.
I find it hard to believe that a bug of this size persist for 8 years. I previously lost a couple of hours on debugging and fixing scripts, as it was still faster than recreating the whole environment elsewhere. Do you think it will ever be fixed?
@qmuntal Want to take a look at this? Thanks.
I see two alternative solutions here:
-
Leave
lpApplicationNamenil whenPathis already equal toArgs[0]. Not that we can't unconditionally setlpApplicationNameand prependPathtoArgsbecause that will break applications that don't expect the application name to be inArgs[0]. This fix works for the common case of initializing the command usingexec.Command, as it always prepend thePathtoArgs. On the other hand, it won't work ifos.ProcAttr.Diris set andPathis relative, as in this we would have to modifyArgs[0]to make it absolute, which will break applications expectingArgs[0]to be the application base name. I've prototyped this solution in CL 620915. -
Detect if
Pathpoints to a.bator.cmdfile, and if so, adapt the argument quoting and escaping rules. This has the benefit of potentially fixing other issues, for example #15566, but doing it right is more complex than the previous solution. Also, the @golang/security has mentioned in the past that they would rather not open this can of worms.
I personally prefer the second option, as it is more complete, but I'm also good with the first one.
Change https://go.dev/cl/620915 mentions this issue: syscall: conditionally pass the application name to CreateProcess on Windows
- Detect if
Pathpoints to a.bator.cmdfile, and if so, adapt the argument quoting and escaping rules. This has the benefit of potentially fixing other issues, for example syscall: exec_windows.go: arguments should not be escaped to work with msiexec #15566,
I also prefer that option.
But what are the .bat or .cmd files argument quoting and escaping rules? Are they documented somewhere?
I have a feeling that msiexec argument quoting and escaping rules are different again. Are these documented somewhere?
Alex
From the security perspective, the main pain point here is that the rules are not immediately obvious, so implementing multiple escaping schemes seems likely to result in hard to parse semantics, both for us and for the user (although, arguably, this is already true for the latter), which tends to be a recipe for security issues down the road.
I don't think we would have a problem with the second solution given two things:
- Detection of bat/cmd is reliable. Does Windows enforce the property that a filename ending in .bat/.cmd means its a batch file? Is there anything else we'd need to do to make sure we aren't using the wrong escaping scheme?
- The escaping rules we implement are documented (at least... somewhat) by Microsoft. We currently implement a vague approximation of how CommandLineToArgvW works, but if I'm remembering correctly some of that is based on guesswork. Ideally we would be able to say for executables we using X escaping algorithm, and for batch files we use Y algorithm (and, possibly, for misexec we use Z algorithm).
The bat/cmd quoting rules are well defined in here: https://learn.microsoft.com/en-us/windows-server/administration/windows-commands/cmd.
I've been investigating this issue a little bit more and I no longer think that we should directly jump to supporting bat/cmd scripts. I've submitted #69939 with a middle-wround alternative.
edited: Note that #69939 doesn't cover misexec. I'm inclined to say that misexec is already well served by syscall.SysProcAttr.CmdLine. The Go standard library can't support every niche application not following the standard rules.
Change https://go.dev/cl/621795 mentions this issue: syscall: quote "cmd.exe /c" command on Windows