replace shell parsing w/ direct shell call

Instead of trying to parse the shell command line into arguments for input to exec.Command this replaces that all with passing it to an 'sh -c' call. This will "fix" the wack-o-mole game of spot fixing issues that keep coming up with the shell parsing package. Windows is handled separately (no 'sh') and is setup to only accept/run a single command.
hashicorp · Jul 30, 2021 · 1b5a0a8 · 1b5a0a8
1 parent e4a17d2
commit 1b5a0a8
Show file tree

Hide file tree

Showing 4 changed files with 154 additions and 31 deletions.
diff --git a/manager/command-prep.go b/manager/command-prep.go
@@ -0,0 +1,12 @@
+package manager
+
+const whitespace = " \t\n\v\f\r"
+
+func prepCommand(command string) ([]string, error) {
+	if len(command) == 0 {
+		return []string{}, nil
+	}
+
+	cmd := []string{"sh", "-c", command}
+	return cmd, nil
+}
diff --git a/manager/command-prep_windows.go b/manager/command-prep_windows.go
@@ -0,0 +1,13 @@
+// +build windows
+
+package manager
+
+func prepCommand(command string) ([]string, error) {
+	switch {
+	case len(command) == 0:
+		return []string{}, nil
+	case len(strings.Fields(command)) > 1:
+		return cmd, fmt.Errorf("only single commands supported on windows")
+	}
+	return []string{command}, nil
+}
diff --git a/manager/runner.go b/manager/runner.go
@@ -10,15 +10,16 @@ import (
 	"sync"
 	"time"
 
+	"github.com/pkg/errors"
+
 	"github.com/hashicorp/consul-template/child"
 	"github.com/hashicorp/consul-template/config"
 	dep "github.com/hashicorp/consul-template/dependency"
 	"github.com/hashicorp/consul-template/renderer"
 	"github.com/hashicorp/consul-template/template"
 	"github.com/hashicorp/consul-template/watch"
+
 	multierror "github.com/hashicorp/go-multierror"
-	shellwords "github.com/mattn/go-shellwords"
-	"github.com/pkg/errors"
 )
 
 const (
@@ -1151,7 +1152,7 @@ type spawnChildInput struct {
 // spawnChild spawns a child process with the given inputs and returns the
 // resulting child.
 func spawnChild(i *spawnChildInput) (*child.Child, error) {
-	args, err := parseCommand(i.Command)
+	args, err := prepCommand(i.Command)
 	if err != nil {
 		return nil, errors.Wrap(err, "failed parsing command")
 	}
@@ -1178,14 +1179,6 @@ func spawnChild(i *spawnChildInput) (*child.Child, error) {
 	return child, nil
 }
 
-// parseCommand parses the shell command line into usable format
-func parseCommand(command string) ([]string, error) {
-	p := shellwords.NewParser()
-	p.ParseEnv = true
-	p.ParseBacktick = true
-	return p.Parse(command)
-}
-
 // quiescence is an internal representation of a single template's quiescence
 // state.
 type quiescence struct {

diff --git a/manager/runner_test.go b/manager/runner_test.go
@@ -10,6 +10,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/hashicorp/consul-template/child"
 	"github.com/hashicorp/consul-template/config"
 	dep "github.com/hashicorp/consul-template/dependency"
 	"github.com/hashicorp/consul-template/template"
@@ -669,7 +670,8 @@ func TestRunner_Start(t *testing.T) {
 
 		c := config.DefaultConfig().Merge(&config.Config{
 			Exec: &config.ExecConfig{
-				Command: config.String(`sleep 30`),
+				Command:     config.String(`sleep 30`),
+				KillTimeout: config.TimeDuration(time.Duration(10 * time.Second)),
 			},
 			Templates: &config.TemplateConfigs{
 				&config.TemplateConfig{
@@ -1059,48 +1061,151 @@ func TestRunner_quiescence(t *testing.T) {
 	})
 }
 
-func TestRunner_parseCommand(t *testing.T) {
+func TestRunner_command(t *testing.T) {
 	type testCase struct {
-		name, input string
-		expect      []string
+		name, input, out string
+		parsed           []string
 	}
-	runTest := func(tc testCase) {
-		out, err := parseCommand(tc.input)
-		mismatchErr := "bad command parse\ngot: '%#v'\nwanted: '%#v'"
+	os.Setenv("FOO", "bar")
+
+	parseTest := func(tc testCase) {
+		out, err := prepCommand(tc.input)
+		mismatchErr := "bad command parse\n   got: '%#v'\nwanted: '%#v'"
 		switch {
 		case err != nil:
-			t.Fatal("unexpected error:", err)
-		case len(out) != len(tc.expect):
-			t.Fatalf(mismatchErr, out, tc.expect)
-		case !reflect.DeepEqual(out, tc.expect):
-			t.Fatalf(mismatchErr, out, tc.expect)
+			t.Error("unexpected error:", err)
+		case len(out) != len(tc.parsed):
+			t.Errorf(mismatchErr, out, tc.parsed)
+		case !reflect.DeepEqual(out, tc.parsed):
+			t.Errorf(mismatchErr, out, tc.parsed)
+		}
+	}
+	runTest := func(tc testCase) {
+		stdout := new(bytes.Buffer)
+		stderr := new(bytes.Buffer)
+		args, _ := prepCommand(tc.input)
+		child, err := child.New(&child.NewInput{
+			Stdin:   os.Stdin,
+			Stdout:  stdout,
+			Stderr:  stderr,
+			Command: args[0],
+			Args:    args[1:],
+		})
+		if err != nil {
+			t.Fatal("error creating child process:", err)
+		}
+		child.Start()
+		defer child.Stop()
+		if err != nil {
+			t.Fatal("error starting child process:", err)
+		}
+		<-child.ExitCh()
+		switch {
+		case stderr.Len() > 0:
+			t.Errorf("unexpected error output: %s", stderr.String())
+		case tc.out != stdout.String():
+			t.Errorf("wrong command output\n   got: '%#v'\nwanted: '%#v'",
+				stdout.String(), tc.out)
 		}
 	}
 	for i, tc := range []testCase{
 		{
 			name:   "null",
 			input:  "",
-			expect: []string{},
+			parsed: []string{},
+			out:    "",
+		},
+		{
+			name:   "single",
+			input:  "echo",
+			parsed: []string{"sh", "-c", "echo"},
+			out:    "\n",
 		},
 		{
 			name:   "simple",
-			input:  "echo hi",
-			expect: []string{"echo", "hi"},
+			input:  "printf hi",
+			parsed: []string{"sh", "-c", "printf hi"},
+			out:    "hi",
+		},
+		{
+			name:   "subshell-bash", // GH-1456 & GH-1463
+			input:  "bash -c 'printf hi'",
+			parsed: []string{"sh", "-c", "bash -c 'printf hi'"},
+			out:    "hi",
 		},
 		{
 			name:   "subshell-single-quoting", // GH-1456 & GH-1463
-			input:  "sh -c 'echo hi'",
-			expect: []string{"sh", "-c", "echo hi"},
+			input:  "sh -c 'printf hi'",
+			parsed: []string{"sh", "-c", "sh -c 'printf hi'"},
+			out:    "hi",
 		},
 		{
 			name:   "subshell-double-quoting",
-			input:  `sh -c "echo hi"`,
-			expect: []string{"sh", "-c", "echo hi"},
+			input:  `sh -c "echo -n hi"`,
+			parsed: []string{"sh", "-c", `sh -c "echo -n hi"`},
+			out:    "hi",
+		},
+		{
+			name:   "subshell-call",
+			input:  `echo -n $(echo -n foo)`,
+			parsed: []string{"sh", "-c", "echo -n $(echo -n foo)"},
+			out:    "foo",
+		},
+		{
+			name:   "pipe",
+			input:  `seq 1 5 | grep 3`,
+			parsed: []string{"sh", "-c", "seq 1 5 | grep 3"},
+			out:    "3\n",
+		},
+		{
+			name:   "conditional",
+			input:  `sh -c 'if true; then printf foo; fi'`,
+			parsed: []string{"sh", "-c", "sh -c 'if true; then printf foo; fi'"},
+			out:    "foo",
+		},
+		{
+			name:   "command-substition",
+			input:  `sh -c 'echo -n $(which /bin/sh)'`,
+			parsed: []string{"sh", "-c", "sh -c 'echo -n $(which /bin/sh)'"},
+			out:    "/bin/sh",
+		},
+		{
+			name:   "curly-brackets",
+			input:  "sh -c '{ if [ -f /bin/sh ]; then echo -n foo; else echo -n bar; fi }'",
+			parsed: []string{"sh", "-c", "sh -c '{ if [ -f /bin/sh ]; then echo -n foo; else echo -n bar; fi }'"},
+			out:    "foo",
+		},
+		{
+			name:   "and",
+			input:  "true && true && echo -n and",
+			parsed: []string{"sh", "-c", "true && true && echo -n and"},
+			out:    "and",
+		},
+		{
+			name:   "or",
+			input:  "false || false || echo -n or",
+			parsed: []string{"sh", "-c", "false || false || echo -n or"},
+			out:    "or",
+		},
+		{
+			name:   "backgrounded",
+			input:  "(sleep .1; echo -n hi) &",
+			parsed: []string{"sh", "-c", "(sleep .1; echo -n hi) &"},
+			out:    "hi",
+		},
+		{
+			name:   "env",
+			input:  "echo -n ${FOO}",
+			parsed: []string{"sh", "-c", "echo -n ${FOO}"},
+			out:    "bar",
 		},
 	} {
 		t.Run(fmt.Sprintf("%d_%s", i, tc.name),
 			func(t *testing.T) {
-				runTest(tc)
+				parseTest(tc)
+				if len(tc.input) > 0 {
+					runTest(tc)
+				}
 			})
 	}
 }