go: os/exec: Cannot execute command with space in the name on Windows, when there are parameters

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 lpApplicationName for CreateProcess is cited as a security best-practice as a fail-safe should the caller fail to add quotes around the path of the executable in lpCommandLine. FWIW, dotnet ensures that the executable is quoted for this very reason, and it turns out golang does, too, via makeCmdLine’s use of appendEscapeArg.
• I can’t find any official documentation around this subtle behavior difference with CreateProcess as to why it works when NOT specifying lpApplicationName, 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. 😅

Why not fixed?

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

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.

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>

@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: https://github.com/golang/go/issues/17149#issuecomment-1426878988

Concretely, this is the code change I’d like to propose: https://github.com/rafd123/go/commit/d24b468bb4318ef6085569d7a2e9e3fd8f93a1b3#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: https://github.com/rafd123/go/commit/d24b468bb4318ef6085569d7a2e9e3fd8f93a1b3#diff-f0ee8d33c527f62f2e48195ea748d2d7295c26910144012ee6e108fbf08f21a6

That said, there seems to be a precendent for this assumption in syscall/exec_unix.go here: https://github.com/golang/go/blob/e8c8b79f000515e086012df632f01fc0ec21076b/src/syscall/exec_unix.go#L169-L171

What do you think? Is this a reasonable approach? If so, I’ll submit a PR.

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

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.Command without the SysProcAttr escape 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.

Wow. What a bug. XD

I agree with https://github.com/golang/go/issues/17149#issuecomment-257518448 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.