diff --git a/cmd/state-exec/cmd.go b/cmd/state-exec/cmd.go index e7a40a8720..8a878c23da 100644 --- a/cmd/state-exec/cmd.go +++ b/cmd/state-exec/cmd.go @@ -3,12 +3,11 @@ package main import ( "fmt" "os" - "os/exec" ) func runCmd(meta *executorMeta) (int, error) { userArgs := os.Args[1:] - cmd := exec.Command(meta.MatchingBin, userArgs...) + cmd := Command(meta.MatchingBin, userArgs...) cmd.Stdin = os.Stdin cmd.Stdout = os.Stdout cmd.Stderr = os.Stderr diff --git a/cmd/state-exec/cmd_lin_mac.go b/cmd/state-exec/cmd_lin_mac.go new file mode 100644 index 0000000000..b78775a432 --- /dev/null +++ b/cmd/state-exec/cmd_lin_mac.go @@ -0,0 +1,10 @@ +//go:build !windows +// +build !windows + +package main + +import "os/exec" + +func Command(name string, arg ...string) *exec.Cmd { + return exec.Command(name, arg...) +} diff --git a/cmd/state-exec/cmd_windows.go b/cmd/state-exec/cmd_windows.go new file mode 100644 index 0000000000..28092e2ff1 --- /dev/null +++ b/cmd/state-exec/cmd_windows.go @@ -0,0 +1,99 @@ +package main + +import ( + "os/exec" + "path/filepath" + "strings" + "syscall" + + "github.com/ActiveState/cli/cmd/state-exec/internal/logr" +) + +func Command(name string, arg ...string) *exec.Cmd { + cmd := exec.Command(name, arg...) + + exeName := filepath.Base(strings.ToLower(name)) + if exeName == "cmd" || strings.HasSuffix(exeName, ".bat") || strings.HasSuffix(exeName, ".cmd") { + // Go currently does not escape arguments properly on Windows, it account for spaces and tab characters, but not + // other characters that need escaping such as `<` and `>`. + // This can be dropped once we update to a Go version that fixes this bug: https://github.com/golang/go/issues/68313 + cmd.SysProcAttr = &syscall.SysProcAttr{CmdLine: makeCmdLine(cmd.Args)} + logr.Debug("processed command line: %s", cmd.SysProcAttr.CmdLine) + } + + return cmd +} + +// makeCmdLine builds a command line out of args by escaping "special" +// characters and joining the arguments with spaces. +// Based on syscall\exec_windows.go +func makeCmdLine(args []string) string { + var b []byte + for _, v := range args { + if len(b) > 0 { + b = append(b, ' ') + } + b = appendEscapeArg(b, v) + } + return string(b) +} + +// appendEscapeArg escapes the string s, as per escapeArg, +// appends the result to b, and returns the updated slice. +// Based on syscall\exec_windows.go +func appendEscapeArg(b []byte, s string) []byte { + if len(s) == 0 { + return append(b, `""`...) + } + + needsBackslash := false + needsQuotes := false + for i := 0; i < len(s); i++ { + switch s[i] { + case '"', '\\': + needsBackslash = true + // Based on https://github.com/sebres/PoC/blob/master/SB-0D-001-win-exec/SOLUTION.md#definition + case ' ', '\t', '<', '>', '&', '|', '^', '!', '(', ')', '%': + needsQuotes = true + } + } + + if !needsBackslash && !needsQuotes { + // No special handling required; normal case. + return append(b, s...) + } + if !needsBackslash { + // hasSpace is true, so we need to quote the string. + b = append(b, '"') + b = append(b, s...) + return append(b, '"') + } + + if needsQuotes { + b = append(b, '"') + } + slashes := 0 + for i := 0; i < len(s); i++ { + c := s[i] + switch c { + default: + slashes = 0 + case '\\': + slashes++ + case '"': + for ; slashes > 0; slashes-- { + b = append(b, '\\') + } + b = append(b, '\\') + } + b = append(b, c) + } + if needsQuotes { + for ; slashes > 0; slashes-- { + b = append(b, '\\') + } + b = append(b, '"') + } + + return b +} diff --git a/internal/osutils/exeutils.go b/internal/osutils/exeutils.go index 6998f88ee6..a789ba209d 100644 --- a/internal/osutils/exeutils.go +++ b/internal/osutils/exeutils.go @@ -97,7 +97,7 @@ func ExecSimple(bin string, args []string, env []string) (string, string, error) func ExecSimpleFromDir(dir, bin string, args []string, env []string) (string, string, error) { logging.Debug("ExecSimpleFromDir: dir: %s, bin: %s, args: %#v, env: %#v", dir, bin, args, env) - c := exec.Command(bin, args...) + c := Command(bin, args...) if dir != "" { c.Dir = dir } @@ -117,7 +117,7 @@ func ExecSimpleFromDir(dir, bin string, args []string, env []string) (string, st // Execute will run the given command and with optional settings for the exec.Cmd struct func Execute(command string, arg []string, optSetter func(cmd *exec.Cmd) error) (int, *exec.Cmd, error) { - cmd := exec.Command(command, arg...) + cmd := Command(command, arg...) logging.Debug("Executing command: %s, with args: %s", cmd, arg) if optSetter != nil { if err := optSetter(cmd); err != nil { @@ -145,7 +145,7 @@ func ExecuteAndPipeStd(command string, arg []string, env []string) (int, *exec.C // ExecuteAndForget will run the given command in the background, returning immediately. func ExecuteAndForget(command string, args []string, opts ...func(cmd *exec.Cmd) error) (*os.Process, error) { logging.Debug("Executing: %s %v", command, args) - cmd := exec.Command(command, args...) + cmd := Command(command, args...) for _, optSetter := range opts { if err := optSetter(cmd); err != nil { @@ -172,7 +172,7 @@ func ExecuteAndForget(command string, args []string, opts ...func(cmd *exec.Cmd) func ExecuteInBackground(command string, args []string, opts ...func(cmd *exec.Cmd) error) (*exec.Cmd, *bytes.Buffer, *bytes.Buffer, error) { logging.Debug("Executing: %s %v", command, args) - cmd := exec.Command(command, args...) + cmd := Command(command, args...) var stdoutBuf, stderrBuf bytes.Buffer for _, optSetter := range opts { diff --git a/internal/osutils/exeutils_unix.go b/internal/osutils/exeutils_unix.go index e37bae40fe..e62196f76e 100644 --- a/internal/osutils/exeutils_unix.go +++ b/internal/osutils/exeutils_unix.go @@ -3,6 +3,12 @@ package osutils +import "os/exec" + const ExeExtension = "" var exts = []string{} + +func Command(name string, arg ...string) *exec.Cmd { + return exec.Command(name, arg...) +} diff --git a/internal/osutils/exeutils_windows.go b/internal/osutils/exeutils_windows.go index 9221eab9b2..885c07c412 100644 --- a/internal/osutils/exeutils_windows.go +++ b/internal/osutils/exeutils_windows.go @@ -2,7 +2,10 @@ package osutils import ( "os" + "os/exec" + "path/filepath" "strings" + "syscall" "github.com/thoas/go-funk" ) @@ -15,3 +18,91 @@ func init() { PATHEXT := os.Getenv("PATHEXT") exts = funk.Uniq(funk.Map(strings.Split(PATHEXT, string(os.PathListSeparator)), strings.ToLower).([]string)).([]string) } + +func Command(name string, arg ...string) *exec.Cmd { + cmd := exec.Command(name, arg...) + + exeName := filepath.Base(strings.ToLower(name)) + if exeName == "cmd" || strings.HasSuffix(exeName, ".bat") || strings.HasSuffix(exeName, ".cmd") { + // Go currently does not escape arguments properly on Windows, it account for spaces and tab characters, but not + // other characters that need escaping such as `<` and `>`. + // This can be dropped once we update to a Go version that fixes this bug: https://github.com/golang/go/issues/68313 + cmd.SysProcAttr = &syscall.SysProcAttr{CmdLine: makeCmdLine(cmd.Args)} + } + + return cmd +} + +// makeCmdLine builds a command line out of args by escaping "special" +// characters and joining the arguments with spaces. +// Based on syscall\exec_windows.go +func makeCmdLine(args []string) string { + var b []byte + for _, v := range args { + if len(b) > 0 { + b = append(b, ' ') + } + b = appendEscapeArg(b, v) + } + return string(b) +} + +// appendEscapeArg escapes the string s, as per escapeArg, +// appends the result to b, and returns the updated slice. +// Based on syscall\exec_windows.go +func appendEscapeArg(b []byte, s string) []byte { + if len(s) == 0 { + return append(b, `""`...) + } + + needsBackslash := false + needsQuotes := false + for i := 0; i < len(s); i++ { + switch s[i] { + case '"', '\\': + needsBackslash = true + // Based on https://github.com/sebres/PoC/blob/master/SB-0D-001-win-exec/SOLUTION.md#definition + case ' ', '\t', '<', '>', '&', '|', '^', '!', '(', ')', '%': + needsQuotes = true + } + } + + if !needsBackslash && !needsQuotes { + // No special handling required; normal case. + return append(b, s...) + } + if !needsBackslash { + // hasSpace is true, so we need to quote the string. + b = append(b, '"') + b = append(b, s...) + return append(b, '"') + } + + if needsQuotes { + b = append(b, '"') + } + slashes := 0 + for i := 0; i < len(s); i++ { + c := s[i] + switch c { + default: + slashes = 0 + case '\\': + slashes++ + case '"': + for ; slashes > 0; slashes-- { + b = append(b, '\\') + } + b = append(b, '\\') + } + b = append(b, c) + } + if needsQuotes { + for ; slashes > 0; slashes-- { + b = append(b, '\\') + } + b = append(b, '"') + } + + return b +} diff --git a/internal/testhelpers/e2e/session.go b/internal/testhelpers/e2e/session.go index 2dcac555e4..53eeccb889 100644 --- a/internal/testhelpers/e2e/session.go +++ b/internal/testhelpers/e2e/session.go @@ -4,7 +4,6 @@ import ( "fmt" "io/fs" "os" - "os/exec" "path/filepath" "regexp" "runtime" @@ -12,16 +11,17 @@ import ( "testing" "time" - "github.com/ActiveState/cli/internal/subshell" - "github.com/ActiveState/cli/pkg/platform/model" - "github.com/ActiveState/cli/pkg/platform/model/buildplanner" - "github.com/ActiveState/cli/pkg/projectfile" "github.com/ActiveState/termtest" "github.com/go-openapi/strfmt" "github.com/google/uuid" "github.com/phayes/permbits" "github.com/stretchr/testify/require" + "github.com/ActiveState/cli/internal/subshell" + "github.com/ActiveState/cli/pkg/platform/model" + "github.com/ActiveState/cli/pkg/platform/model/buildplanner" + "github.com/ActiveState/cli/pkg/projectfile" + "github.com/ActiveState/cli/internal/condition" "github.com/ActiveState/cli/internal/config" "github.com/ActiveState/cli/internal/constants" @@ -298,7 +298,7 @@ func (s *Session) SpawnCmdWithOpts(exe string, optSetters ...SpawnOptSetter) *Sp args = spawnOpts.Args } - cmd := exec.Command(shell, args...) + cmd := osutils.Command(shell, args...) cmd.Env = spawnOpts.Env if spawnOpts.Dir != "" { diff --git a/pkg/platform/runtime/executors/executors.go b/pkg/platform/runtime/executors/executors.go index eef56e3e40..052e082f6a 100644 --- a/pkg/platform/runtime/executors/executors.go +++ b/pkg/platform/runtime/executors/executors.go @@ -46,6 +46,10 @@ func (es *Executors) ExecutorSrc() (string, error) { return installation.ExecutorExec() } +func (es *Executors) SetExecutorSrc(path string) { + es.altExecSrcPath = path +} + func (es *Executors) Apply(sockPath string, targeter Targeter, env map[string]string, exes envdef.ExecutablePaths) error { logging.Debug("Creating executors at %s, exes: %v", es.executorPath, exes) diff --git a/test/integration/executor_int_test.go b/test/integration/executor_int_test.go index 63b83b8e9f..6c33c9943f 100644 --- a/test/integration/executor_int_test.go +++ b/test/integration/executor_int_test.go @@ -3,14 +3,23 @@ package integration import ( "os" "path/filepath" + "runtime" + "strings" "testing" + "time" - "github.com/ActiveState/cli/internal/testhelpers/suite" + "github.com/ActiveState/termtest" "github.com/ActiveState/cli/internal/constants" + "github.com/ActiveState/cli/internal/environment" + "github.com/ActiveState/cli/internal/fileutils" "github.com/ActiveState/cli/internal/osutils" + "github.com/ActiveState/cli/internal/svcctl" "github.com/ActiveState/cli/internal/testhelpers/e2e" + "github.com/ActiveState/cli/internal/testhelpers/suite" "github.com/ActiveState/cli/internal/testhelpers/tagsuite" + "github.com/ActiveState/cli/pkg/platform/runtime/executors" + "github.com/ActiveState/cli/pkg/platform/runtime/target" ) type ExecutorIntegrationTestSuite struct { @@ -90,3 +99,51 @@ func (suite *ExecutorIntegrationTestSuite) TestExecutorSizeOnDisk() { maxSize := sizeByMegs(4) suite.Require().LessOrEqual(fi.Size(), maxSize, "executor (%d bytes) should be less than or equal to %d bytes", fi.Size(), maxSize) } + +func (suite *ExecutorIntegrationTestSuite) TestExecutorBatArguments() { + suite.OnlyRunForTags(tagsuite.Executor) + + if runtime.GOOS != "windows" { + suite.T().Skip("This test is only for windows") + } + + ts := e2e.New(suite.T(), true) + defer ts.Close() + + root := environment.GetRootPathUnsafe() + executorsPath := filepath.Join(ts.Dirs.Work, "executors") + srcExes := fileutils.ListFilesUnsafe(filepath.Join(root, "test", "integration", "testdata", "batarguments")) + reportExe := filepath.Join(executorsPath, "report.exe") + + t := target.NewCustomTarget("ActiveState-CLI", "test", constants.ValidZeroUUID, "", target.TriggerExecutor) + executors := executors.New(executorsPath) + executors.SetExecutorSrc(ts.ExecutorExe) + err := executors.Apply( + svcctl.NewIPCSockPathFromGlobals().String(), + t, + osutils.EnvSliceToMap(ts.Env), + srcExes, + ) + suite.Require().NoError(err) + suite.Require().FileExists(reportExe) + + // Force override ACTIVESTATE_CI to false, because communicating with the svc will fail, and if this is true + // the executor will interrupt. + // For this test we don't care about the svc communication. + env := e2e.OptAppendEnv("ACTIVESTATE_CI=false") + + inputs := []string{"aa", "hello world", "&whoami", "imnot|apipe", "%NotAppData%", "^NotEscaped", "(NotAGroup)"} + outputs := `"` + strings.Join(inputs, `" "`) + `"` + cp := ts.SpawnCmdWithOpts(reportExe, e2e.OptArgs(inputs...), env) + cp.Expect(outputs, termtest.OptExpectTimeout(5*time.Second)) + cp.ExpectExitCode(0) + + cp = ts.SpawnCmdWithOpts(reportExe, e2e.OptArgs("&whoami"), env) + cp.Expect(`"&whoami"`, termtest.OptExpectTimeout(5*time.Second)) + cp.ExpectExitCode(0) + + // Ensure regular arguments aren't quoted + cp = ts.SpawnCmdWithOpts(reportExe, e2e.OptArgs("ImNormal", "I'm Special", "ImAlsoNormal"), env) + cp.Expect(`ImNormal "I'm Special" ImAlsoNormal`, termtest.OptExpectTimeout(5*time.Second)) + cp.ExpectExitCode(0) +} diff --git a/test/integration/testdata/batarguments/report.bat b/test/integration/testdata/batarguments/report.bat new file mode 100644 index 0000000000..75ca8e2eaf --- /dev/null +++ b/test/integration/testdata/batarguments/report.bat @@ -0,0 +1 @@ +@echo %*