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

syscall: exec_windows.go: arguments should not be escaped to work with msiexec #15566

Open
josephspurrier opened this Issue May 6, 2016 · 33 comments

Comments

Projects
None yet
@josephspurrier

josephspurrier commented May 6, 2016

Problem: When I try to run a command in Go on Windows using exec.Command() and then exec.Run(), the arguments are escaped using this logic: https://github.com/golang/go/blob/master/src/syscall/exec_windows.go#L26. That logic escapes the quotes around TARGETDIR="%v" which need to be there. I currently am assigning to Cmd.SysProcAttr manually to get around the escaping.

  1. What version of Go are you using (go version)?
    Go 1.6.2
  2. What operating system and processor architecture are you using (go env)?
    Windows 10 Pro x64
  3. What did you do?
  4. The code is available here, but needs to be run on Windows: https://play.golang.org/p/aU1PlbNTqM
  5. Download any MSI. I used https://fastdl.mongodb.org/win32/mongodb-win32-x86_64-2008plus-ssl-3.2.6-signed.msi and renamed it to package.msi.
  6. You can run Error() and NoError() to see the difference
  7. What did you expect to see?
    I expect to see the MSI say: Completed the ... Setup Wizard.
  8. What did you see instead?
    I see the Windows Installer help Windows and when I click it, I get: exit status 1639 which is:
    ERROR_INVALID_COMMAND_LINE 1639 Invalid command line argument. Consult the Windows Installer SDK for detailed command line help.

@josephspurrier josephspurrier changed the title from syscall/exec_windows.go: arguments should not be quoted to work with msiexec to syscall/exec_windows.go: arguments should not be escaped to work with msiexec May 6, 2016

@alexbrainman

This comment has been minimized.

Member

alexbrainman commented May 6, 2016

args := fmt.Sprintf(/a "%v" TARGETDIR="%v", msiFile, targetDir)
cmd := exec.Command("msiexec", args)

How many arguments do you want msiexec to get? Because you are passing one argument - args (args is a single string). If you want to pass many arguments, than you should pass them one by one:

cmd := exec.Command("msiexec", "arg1", "arg2", "arg3")

and so on. I think waht you want is:

cmd := exec.Command("msiexec", "/a", "package.msi", `TARGETDIR=C:\test folder`)

Not tested. I don't even know how msiexec works.

Alex

@josephspurrier

This comment has been minimized.

josephspurrier commented May 6, 2016

You are correct, I should have used that in the example. That is the way I originally tested and same error.

@josephspurrier

This comment has been minimized.

josephspurrier commented May 6, 2016

Actually, let me clarify because it is a different error, but same reasoning.

When I run:

cmd := exec.Command("msiexec", "/a", "package.msi", `TARGETDIR="C:\testfolder"`)

I get this error: "Could not access network location "C:\testfolder". And then: exit status 1603

I need quotes around the TARGETDIR value because there could be a space in the path.

This is the command that ultimately I'm trying to run:

msiexec /a "package.msi" TARGETDIR="C:\test folder"
@alexbrainman

This comment has been minimized.

Member

alexbrainman commented May 6, 2016

I get this error: "Could not access network location "C:\testfolder". And then: exit status 1603

msiexec is, probably, confused about having " character in the file name. You confused it.

I need quotes around the TARGETDIR value because there could be a space in the path.

The exec.Command passes arguments to the program as is. It will deal with spaces, quotes, double quotes and everything else. That is what syscall.EscapeArg does. You job is to provide all parameters exactly as your child process expects them. Leave all the quoting to do for syscall.EscapeArg.

This is the command that ultimately I'm trying to run:

msiexec /a "package.msi" TARGETDIR="C:\test folder"

Like I suggested above, you want:

cmd := exec.Command("msiexec", "/a", "package.msi", `TARGETDIR=C:\test folder`)

syscall.EscapeArg will insert whatever escape characters to make sure that space in TARGETDIR=C:\test folder does make it across to msexec.

Alex

@mattn

This comment has been minimized.

Member

mattn commented May 6, 2016

When run msiexec on cmd.exe, some of double-quotes will be removed.

dummy.c

#include <windows.h>
#include <stdio.h>

int
main(int argc, wchar_t* argv[]) {
  int i, narg = 0;
  LPWSTR* args = CommandLineToArgvW(GetCommandLineW(), &narg);
  for (i = 0; i < narg; i++) printf("%ws\n", args[i]);
  LocalFree(args);
  return 0;
}
C:\dev>dummy /a "package.msi" TARGETDIR="C:\testfolder"
dummy
/a
package.msi
TARGETDIR=C:\testfolder

But when run dummy.exe from go, those are not removed.

package main

import (
    "os"
    "os/exec"
)

func main() {
    cmd := exec.Command(`dummy`, "/a", "package.msi", `TARGETDIR="C:\testfolder"`)
    cmd.Stdout = os.Stdout
    cmd.Stderr = os.Stderr
    cmd.Run()
}
C:\dev>go run d.go
dummy
/a
package.msi
TARGETDIR="C:\testfolder"
@josephspurrier

This comment has been minimized.

josephspurrier commented May 6, 2016

When I run this command, it works properly:

cmd := exec.Command("msiexec", "/a", "package.msi", `TARGETDIR=C:\testfolder`)

When I run this command, I get the MSIEXEC WIndows Installer Help and then exit status 1639:

cmd := exec.Command("msiexec", "/a", "package.msi", `TARGETDIR=C:\test folder`)
@alexbrainman

This comment has been minimized.

Member

alexbrainman commented May 6, 2016

When I run this command, I get the MSIEXEC WIndows Installer Help and then exit status 1639:

I actually tried running your program, and I agree that exec.Command escaping rules don't work with msiexec. It is second program (that I know of) that behaves this way - first one is cmd.exe (https://blogs.msdn.microsoft.com/twistylittlepassagesallalike/2011/04/23/everyone-quotes-command-line-arguments-the-wrong-way/).

The Go exec.Command escaping will work if child program reads its arguments with CommandLineToArgv. And most programs doo. But cmd.exe and msiexec.exe do not. So you have to use Cmd.SysProcAttr.CmdLine to build command line by hand. And you do that already. I don't see what else you could do.

Alex

@alexbrainman

This comment has been minimized.

Member

alexbrainman commented May 6, 2016

But when run dummy.exe from go, those are not removed.

This is to be expected. Arguments are passed as is - quotes and all. If you don't want quotes, don't pass them inside parameters.

Alex

@mattn

This comment has been minimized.

Member

mattn commented May 6, 2016

@alexbrainman This is my expected. Just explaining go's behavior. :)

@josephspurrier

This comment has been minimized.

josephspurrier commented May 6, 2016

Ah very interesting! I didn't know cmd and msiexec behaved differently. It looks like other languages have this problem as well. Luckily, we can specify the arguments in a raw format directly using Cmd.SysProcAttr.CmdLine. Do you think a comment added to exec.Command would be sufficient enough?

Just for reference, it looks like arguments will be parsed differently depending on the compiler of the program you are passing to: http://daviddeley.com/autohotkey/parameters/parameters.htm#WIN.

@alexbrainman

This comment has been minimized.

Member

alexbrainman commented May 7, 2016

Do you think a comment added to exec.Command would be sufficient enough?

Do you propose to say that it is a mess? :-)

Also #15588 might be of interest to you.

Alex

@josephspurrier

This comment has been minimized.

josephspurrier commented May 8, 2016

Haha yeah, that should do it! Or something like: The Cmd's Args are escaped and may not work properly with some programs. You can skip escaping them by manually assigning the Args to Cmd.SysProcAttr.CmdLine after prefixing with a space.

And I checked out 15588 - CommandLineToArgv() looks like a good way to test commands to ensure they will be split correctly when read by a C/C++ programs - thanks.

And for more reference, I ran tests with different syntax in command prompt and the results are below. It's a shame you can't put quotes around the entire property like this because this wouldn't be an issue, but it's a msiexec problem, not a Go problem: "TARGETDIR=C:\testfolder"

# MSI Success
msiexec /a package.msi TARGETDIR=C:\testfolder

# MSI Success
msiexec /a package.msi TARGETDIR="C:\test folder"

# MSI Error
msiexec /a package.msi "TARGETDIR=C:\test folder"

# MSI Error
msiexec /a package.msi TARGETDIR=\"C:\test folder\"

# MSI Error
msiexec /a package.msi TARGETDIR=""C:\test folder""

# MSI Error
msiexec /a package.msi TARGETDIR=^\"C:\test folder^\"

# MSI Error
msiexec /a package.msi TARGETDIR='C:\test folder'
@alexbrainman

This comment has been minimized.

Member

alexbrainman commented May 8, 2016

The Cmd's Args are escaped and may not work properly with some programs. You can skip escaping them by manually assigning the Args to Cmd.SysProcAttr.CmdLine after prefixing with a space.

I am bit concerned about adding a lot of confusing mambo jambo because of two programs (cmd.exe and msiexec.exe). For most users we don't even need to say word "escape", and everything works perfectly.

Maybe we should specifically mention cmd.exe and msexec.exe as being different. And say that for some argument strings Cmd.Args will not work, and they should build command line by hand and pass it via Cmd.SysProcAttr.CmdLine. What do you think?

Another alternative would be to create new function similar to syscall.EscapeArg, but specifically for cmd.exe. And ignore msexec.exe altogether.

Alex

@josephspurrier

This comment has been minimized.

josephspurrier commented May 8, 2016

I agree on preventing it from becoming confusing. Can we do both options? Since manually setting Cmd.SysProcAttr.CmdLine is an option, adding a comment about it would be helpful.

And for a more official way, I like the idea of adding an alternative syscall.EscapeArg for cmd. We could then run (hopefully) msiexec and other applications by passing them to cmd like this:

cmd /k "msiexec /a C:\package.msi TARGETDIR=^"C:\test folder^""
@alexbrainman

This comment has been minimized.

Member

alexbrainman commented May 8, 2016

We could then run (hopefully) msiexec and other applications by passing them to cmd like this:

cmd /k "msiexec /a C:\package.msi TARGETDIR=^"C:\test folder^""

I don't see how having special function that handles cmd.exe arguments will help with msiexec.exe. The idea is that this function allows you not to worry about quotes and spaces and similar. If you are prepared to take care of quotes and space and others in your code, then Cmd.SysProcAttr.CmdLine is all you need.

@josephspurrier

This comment has been minimized.

josephspurrier commented May 8, 2016

I am going to take care of quotes and spaces in my code, I'm just trying to think about what makes the most sense for other users that will experience the same problem. A comment seems like it makes the most sense right now since I don't have any good use cases for a separate function for cmd.exe.

@alexbrainman

This comment has been minimized.

Member

alexbrainman commented May 8, 2016

A comment seems like it makes the most sense right now since I don't have any good use cases for a separate function for cmd.exe.

Sounds good to me. Feel free to send the change.

Alex

@bradfitz bradfitz changed the title from syscall/exec_windows.go: arguments should not be escaped to work with msiexec to syscall: exec_windows.go: arguments should not be escaped to work with msiexec May 9, 2016

@bradfitz bradfitz added the OS-Windows label May 9, 2016

@bradfitz bradfitz added this to the Go1.8Maybe milestone May 9, 2016

@gopherbot

This comment has been minimized.

gopherbot commented Jul 2, 2016

CL https://golang.org/cl/24730 mentions this issue.

@gopherbot

This comment has been minimized.

gopherbot commented Oct 13, 2016

CL https://golang.org/cl/30947 mentions this issue.

@rsc

This comment has been minimized.

Contributor

rsc commented Oct 13, 2016

I just sent CL 30947, which does the right quoting for cmd.exe at least. There is a test that now passes and did not before. The actual rules seem easy enough that we should provide them.

The hard question is how to know when to turn on the additional quoting. The CL looks for cmd or cmd.exe or %COMSPEC%, which is a bit of a hack. I would like to have something in the SysProcAttr that lets you opt in or out explicitly, but I would also like it if we could do the right thing by default in more cases than we do now. Is that possible?

Detecting explicit cmd is easy, if a bit hacky, but then in #17149 we have an ordinary batch file being executed and that has the same issue. We could also apply the rules for programs ending in ".bat". Is that enough? Do all batch files still end in ".bat"?

@josephspurrier

This comment has been minimized.

josephspurrier commented Oct 13, 2016

Nah, they also use .cmd.

@rsc

This comment has been minimized.

Contributor

rsc commented Oct 13, 2016

I guess the bigger question is: is there a reliable way to detect batch files? Must they end in ".bat" or ".cmd"? And if something does end in ".bat" or ".cmd" is it guaranteed to be a batch file, or at least to be reinvoked using cmd /c ?

@rasky

This comment has been minimized.

Member

rasky commented Oct 13, 2016

I'm not sure trying to detect this is the correct way. I suggest to add (somewhere) a CmdCommand() function that does the escaping, and then specify in the documentation that cmd requires a non-standard escaping and thus point users to CmdCommand()

@alexbrainman

This comment has been minimized.

Member

alexbrainman commented Feb 19, 2017

Maybe the same issue

Probably is the same issue. Perhaps %windir%\system32\inetsrv\appcmd uses different command line parameters escaping rules.

Alex

@bradfitz bradfitz modified the milestones: Go1.9Early, Go1.10Early May 3, 2017

@bradfitz bradfitz modified the milestones: Go1.10Early, Go1.10 Jun 14, 2017

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Sep 30, 2017

I think that in the ideal world we would quote arguments in the safest way by default, and would provide fields in syscall.SysProcAttr to request less safe quoting mechanisms. That would protect the naïve user and permit the expert to get what they need.

Is that possible without breaking backward compatibility?

@alexbrainman

This comment has been minimized.

Member

alexbrainman commented Sep 30, 2017

Is that possible without breaking backward compatibility?

We already doing that as far as I am concerned. Executing most programs work. Except for cmd.exe and msiexec.exe (and maybe few others that we do not know about). For exceptions users can use syscall.SysProcAttr.CmdLine to format command line as they wish.

It is tempting to add code similar to CL 30947 to handle cmd.exe, but then we should also handle batch files. For simple cmd.exe invocations (where no escaping required) exec.Command already works, so changing the syscall package code will not change that. But if exec.Command("cmd.exe", ...) needs escaping, then it would be broken now - so fixing this code seems like a win / win.

Alex

@bradfitz bradfitz added the help wanted label Dec 7, 2017

@bradfitz bradfitz modified the milestones: Go1.10, Go1.11 Dec 7, 2017

@clburlison clburlison referenced this issue Jan 23, 2018

Closed

Install msi #12

@bradfitz bradfitz modified the milestones: Go1.11, Go1.12 May 18, 2018

s-h-a-d-o-w added a commit to s-h-a-d-o-w/nvm-windows that referenced this issue May 21, 2018

Fixes coreybutler#41 - Issue with blank spaces in path
exec.Command() can't handle spaces, even when quoted.
Command has to be modified using SysProcAttr.
See also:
golang/go#15566

Plus: Minor refactoring - reordering of functions, more consistent capitalization, sorting of imports and whatnot.
@s-h-a-d-o-w

This comment has been minimized.

s-h-a-d-o-w commented May 21, 2018

For simple cmd.exe invocations (where no escaping required)

Considering that probably half of the billions of child processes spawned each day on Windows have something to do with cmd and spaces in paths are nothing unusual since at least the year 2000, I find this curiously optimistic.

And odd that the member CmdLine isn't even listed here: https://golang.org/pkg/syscall/#SysProcAttr
Also no example for how to use it to be found aside from e.g. here: https://medium.com/@felixge/killing-a-child-process-and-all-of-its-children-in-go-54079af94773

I probably won't use Go any more myself but still wanted to note that this is obviously a very common use case (simply check the stars on nvm-windows if you don't know what nvm is), so it would be for your own benefit to document this instead of dismiss it.
But that's just my two cents... Go has survived without this for 8 years already after all. :)

@ianlancetaylor ianlancetaylor added this to the Go1.12 milestone May 31, 2018

s-h-a-d-o-w added a commit to s-h-a-d-o-w/nvm-windows that referenced this issue Aug 3, 2018

Fixes #41 - Issue with blank spaces in path
exec.Command() can't handle spaces, even when quoted.
Command has to be modified using SysProcAttr.
See also:
golang/go#15566

Plus: Minor refactoring - reordering of functions, more consistent capitalization, sorting of imports and whatnot.
@magical

This comment has been minimized.

Contributor

magical commented Aug 31, 2018

In #27199, @sebres proposes what looks like a clever algorithm for quoting arguments in such a way that they can survive being passed through cmd.exe, while also working fine in the normal case where cmd.exe isn't involved. If it works, this would obviate the need try and sniff out .bat files.

@alexbrainman

This comment has been minimized.

Member

alexbrainman commented Sep 1, 2018

In #27199, @sebres proposes what looks like a clever algorithm for quoting arguments in such a way that they can survive being passed through cmd.exe, while also working fine in the normal case where cmd.exe isn't involved. If it works, this would obviate the need try and sniff out .bat files.

@magical someone still needs to write the code. I have very little free time now days. If you want to contribute fix to building command line that executes cmd.exe, you are welcome to - https://golang.org/doc/contribute.html I will be happy to help you in any way I can.

Alex

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment