From 899168eb50b8491ee978b2337525d4e4ad3f3fe4 Mon Sep 17 00:00:00 2001 From: "brian m. carlson" Date: Fri, 14 Sep 2018 21:50:09 +0000 Subject: [PATCH 1/5] tools: move ShellQuote to subprocess In a future commit, we'll be adding some additional shell-related handling which logically belongs to the subprocess package. To avoid an import loop, move the ShellQuote function and its required variable from the tools package to the subprocess package. Update the only caller of this function. This commit contains no functional change. --- lfshttp/ssh.go | 3 ++- subprocess/subprocess.go | 19 +++++++++++++++++++ subprocess/subprocess_test.go | 35 +++++++++++++++++++++++++++++++++++ tools/str_tools.go | 19 +------------------ tools/str_tools_test.go | 28 ---------------------------- 5 files changed, 57 insertions(+), 47 deletions(-) create mode 100644 subprocess/subprocess_test.go diff --git a/lfshttp/ssh.go b/lfshttp/ssh.go index 8fe8f3df..805e5ccc 100644 --- a/lfshttp/ssh.go +++ b/lfshttp/ssh.go @@ -11,6 +11,7 @@ import ( "time" "github.com/git-lfs/git-lfs/config" + "github.com/git-lfs/git-lfs/subprocess" "github.com/git-lfs/git-lfs/tools" "github.com/rubyist/tracerx" ) @@ -120,7 +121,7 @@ func sshFormatArgs(cmd string, args []string, needShell bool) (string, []string) return cmd, args } - joined := cmd + " " + strings.Join(tools.ShellQuote(args), " ") + joined := cmd + " " + strings.Join(subprocess.ShellQuote(args), " ") return "sh", []string{"-c", joined} } diff --git a/subprocess/subprocess.go b/subprocess/subprocess.go index 53e0b8e7..17272119 100644 --- a/subprocess/subprocess.go +++ b/subprocess/subprocess.go @@ -8,6 +8,7 @@ import ( "fmt" "os" "os/exec" + "regexp" "strconv" "strings" @@ -83,6 +84,24 @@ func Output(cmd *Cmd) (string, error) { return strings.Trim(string(out), " \n"), err } +var shellWordRe = regexp.MustCompile(`\A[A-Za-z0-9_@.-]+\z`) + +// ShellQuote returns a copied string slice where each element is quoted +// suitably for sh. +func ShellQuote(strs []string) []string { + dup := make([]string, 0, len(strs)) + + for _, str := range strs { + // Quote anything that looks slightly complicated. + if shellWordRe.FindStringIndex(str) == nil { + dup = append(dup, "'"+strings.Replace(str, "'", "'\\''", -1)+"'") + } else { + dup = append(dup, str) + } + } + return dup +} + func Trace(name string, args ...string) { tracerx.Printf("exec: %s %s", name, quotedArgs(args)) } diff --git a/subprocess/subprocess_test.go b/subprocess/subprocess_test.go new file mode 100644 index 00000000..a2999a4b --- /dev/null +++ b/subprocess/subprocess_test.go @@ -0,0 +1,35 @@ +package subprocess + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +type ShellQuoteTestCase struct { + Given []string + Expected []string +} + +func (c *ShellQuoteTestCase) Assert(t *testing.T) { + actual := ShellQuote(c.Given) + + assert.Equal(t, c.Expected, actual, + "tools: expected ShellQuote(%q) to equal %#v (was %#v)", + c.Given, c.Expected, actual, + ) +} + +func TestShellQuote(t *testing.T) { + for desc, c := range map[string]ShellQuoteTestCase{ + "simple": {[]string{"foo", "bar", "an_id"}, []string{"foo", "bar", "an_id"}}, + "leading space": {[]string{" foo", "bar"}, []string{"' foo'", "bar"}}, + "trailing space": {[]string{"foo", "bar "}, []string{"foo", "'bar '"}}, + "internal space": {[]string{"foo bar", "baz quux"}, []string{"'foo bar'", "'baz quux'"}}, + "backslash": {[]string{`foo\bar`, `b\az`}, []string{`'foo\bar'`, `'b\az'`}}, + "quotes": {[]string{`foo"bar`, "b'az"}, []string{`'foo"bar'`, "'b'\\''az'"}}, + "mixed quotes": {[]string{`"foo'ba\"r\"'"`}, []string{`'"foo'\''ba\"r\"'\''"'`}}, + } { + t.Run(desc, c.Assert) + } +} diff --git a/tools/str_tools.go b/tools/str_tools.go index 14f6debe..5c459e4d 100644 --- a/tools/str_tools.go +++ b/tools/str_tools.go @@ -46,22 +46,6 @@ func QuotedFields(s string) []string { return out } -// ShellQuote returns a copied string slice where each element is quoted -// suitably for sh. -func ShellQuote(strs []string) []string { - dup := make([]string, 0, len(strs)) - - for _, str := range strs { - // Quote anything that looks slightly complicated. - if shellWordRe.FindStringIndex(str) == nil { - dup = append(dup, "'"+strings.Replace(str, "'", "'\\''", -1)+"'") - } else { - dup = append(dup, str) - } - } - return dup -} - // Ljust returns a copied string slice where each element is left justified to // match the width of the longest element in the set. func Ljust(strs []string) []string { @@ -129,8 +113,7 @@ func Indent(str string) string { } var ( - tabRe = regexp.MustCompile(`(?m)^[ \t]+`) - shellWordRe = regexp.MustCompile(`\A[A-Za-z0-9_@.-]+\z`) + tabRe = regexp.MustCompile(`(?m)^[ \t]+`) ) // Undent removes all leading tabs in the given string "str", line-wise. diff --git a/tools/str_tools_test.go b/tools/str_tools_test.go index 7a693b9b..eb878bd0 100644 --- a/tools/str_tools_test.go +++ b/tools/str_tools_test.go @@ -71,34 +71,6 @@ func TestQuotedFields(t *testing.T) { } } -type ShellQuoteTestCase struct { - Given []string - Expected []string -} - -func (c *ShellQuoteTestCase) Assert(t *testing.T) { - actual := ShellQuote(c.Given) - - assert.Equal(t, c.Expected, actual, - "tools: expected ShellQuote(%q) to equal %#v (was %#v)", - c.Given, c.Expected, actual, - ) -} - -func TestShellQuote(t *testing.T) { - for desc, c := range map[string]ShellQuoteTestCase{ - "simple": {[]string{"foo", "bar", "an_id"}, []string{"foo", "bar", "an_id"}}, - "leading space": {[]string{" foo", "bar"}, []string{"' foo'", "bar"}}, - "trailing space": {[]string{"foo", "bar "}, []string{"foo", "'bar '"}}, - "internal space": {[]string{"foo bar", "baz quux"}, []string{"'foo bar'", "'baz quux'"}}, - "backslash": {[]string{`foo\bar`, `b\az`}, []string{`'foo\bar'`, `'b\az'`}}, - "quotes": {[]string{`foo"bar`, "b'az"}, []string{`'foo"bar'`, "'b'\\''az'"}}, - "mixed quotes": {[]string{`"foo'ba\"r\"'"`}, []string{`'"foo'\''ba\"r\"'\''"'`}}, - } { - t.Run(desc, c.Assert) - } -} - func TestLongestReturnsEmptyStringGivenEmptySet(t *testing.T) { assert.Equal(t, "", Longest(nil)) } From 54fc03da4382baf0a4cbb1d280a31b4e4e6c5e67 Mon Sep 17 00:00:00 2001 From: "brian m. carlson" Date: Fri, 14 Sep 2018 22:28:28 +0000 Subject: [PATCH 2/5] subprocess: don't escape shell command due to slash Add the slash to the list of characters which don't require quoting for the shell. No POSIX-compatible shell requires escaping the slash, and ignoring it for the purposes of escaping lets us write cleaner, nicer-to-read trace output. Don't do the same with the backslash, which while being the equivalent on Windows, is handled specially by POSIX shells. --- subprocess/subprocess.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/subprocess/subprocess.go b/subprocess/subprocess.go index 17272119..acb04bae 100644 --- a/subprocess/subprocess.go +++ b/subprocess/subprocess.go @@ -84,7 +84,7 @@ func Output(cmd *Cmd) (string, error) { return strings.Trim(string(out), " \n"), err } -var shellWordRe = regexp.MustCompile(`\A[A-Za-z0-9_@.-]+\z`) +var shellWordRe = regexp.MustCompile(`\A[A-Za-z0-9_@/.-]+\z`) // ShellQuote returns a copied string slice where each element is quoted // suitably for sh. From 912891d48f6ff1f3f8819bace1211fe5df740547 Mon Sep 17 00:00:00 2001 From: "brian m. carlson" Date: Fri, 14 Sep 2018 22:31:39 +0000 Subject: [PATCH 3/5] subprocess: add functions to format shell commands There are a small number of places where we'll want to pass data to the shell. Add a function which formats a command name and arguments into a command that invokes "sh -c" with the appropriate args. Additionally add a form that quotes its arguments, and use this in the SSH code, which wants to pass data to the shell. --- lfshttp/ssh.go | 3 +- subprocess/subprocess.go | 15 ++++++++++ subprocess/subprocess_test.go | 56 +++++++++++++++++++++++++++++++++++ 3 files changed, 72 insertions(+), 2 deletions(-) diff --git a/lfshttp/ssh.go b/lfshttp/ssh.go index 805e5ccc..fc8bf77c 100644 --- a/lfshttp/ssh.go +++ b/lfshttp/ssh.go @@ -121,8 +121,7 @@ func sshFormatArgs(cmd string, args []string, needShell bool) (string, []string) return cmd, args } - joined := cmd + " " + strings.Join(subprocess.ShellQuote(args), " ") - return "sh", []string{"-c", joined} + return subprocess.FormatForShellQuotedArgs(cmd, args) } func sshGetLFSExeAndArgs(osEnv config.Environment, gitEnv config.Environment, e Endpoint, method string) (string, []string) { diff --git a/subprocess/subprocess.go b/subprocess/subprocess.go index acb04bae..fe1bfce2 100644 --- a/subprocess/subprocess.go +++ b/subprocess/subprocess.go @@ -102,6 +102,21 @@ func ShellQuote(strs []string) []string { return dup } +// FormatForShell takes a command name and an argument string and returns a +// command and arguments that pass this command to the shell. Note that neither +// the command nor the arguments are quoted. Consider FormatForShellQuoted +// instead. +func FormatForShell(name string, args string) (string, []string) { + return "sh", []string{"-c", name + " " + args} +} + +// FormatForShellQuotedArgs takes a command name and an argument string and +// returns a command and arguments that pass this command to the shell. The +// arguments are escaped, but the name of the command is not. +func FormatForShellQuotedArgs(name string, args []string) (string, []string) { + return FormatForShell(name, strings.Join(ShellQuote(args), " ")) +} + func Trace(name string, args ...string) { tracerx.Printf("exec: %s %s", name, quotedArgs(args)) } diff --git a/subprocess/subprocess_test.go b/subprocess/subprocess_test.go index a2999a4b..20d158a7 100644 --- a/subprocess/subprocess_test.go +++ b/subprocess/subprocess_test.go @@ -33,3 +33,59 @@ func TestShellQuote(t *testing.T) { t.Run(desc, c.Assert) } } + +type FormatForShellQuotedArgsTestCase struct { + GivenCmd string + GivenArgs []string + ExpectedArgs []string +} + +func (c *FormatForShellQuotedArgsTestCase) Assert(t *testing.T) { + actualCmd, actualArgs := FormatForShellQuotedArgs(c.GivenCmd, c.GivenArgs) + + assert.Equal(t, "sh", actualCmd, + "tools: expected FormatForShell command to equal 'sh' (was #%v)", + actualCmd) + assert.Equal(t, c.ExpectedArgs, actualArgs, + "tools: expected FormatForShell(%q, %v) to equal %#v (was %#v)", + c.GivenCmd, c.GivenArgs, c.ExpectedArgs, actualArgs, + ) +} + +func TestFormatForShellQuotedArgs(t *testing.T) { + for desc, c := range map[string]FormatForShellQuotedArgsTestCase{ + "simple": {"foo", []string{"bar", "baz"}, []string{"-c", "foo bar baz"}}, + "spaces": {"foo quux", []string{" bar", "baz "}, []string{"-c", "foo quux ' bar' 'baz '"}}, + "backslashes": {"bin/foo", []string{"\\bar", "b\\az"}, []string{"-c", "bin/foo '\\bar' 'b\\az'"}}, + } { + t.Run(desc, c.Assert) + } +} + +type FormatForShellTestCase struct { + GivenCmd string + GivenArgs string + ExpectedArgs []string +} + +func (c *FormatForShellTestCase) Assert(t *testing.T) { + actualCmd, actualArgs := FormatForShell(c.GivenCmd, c.GivenArgs) + + assert.Equal(t, "sh", actualCmd, + "tools: expected FormatForShell command to equal 'sh' (was #%v)", + actualCmd) + assert.Equal(t, c.ExpectedArgs, actualArgs, + "tools: expected FormatForShell(%q, %v) to equal %#v (was %#v)", + c.GivenCmd, c.GivenArgs, c.ExpectedArgs, actualArgs, + ) +} + +func TestFormatForShell(t *testing.T) { + for desc, c := range map[string]FormatForShellTestCase{ + "simple": {"foo", "bar", []string{"-c", "foo bar"}}, + "spaces": {"foo quux", "bar baz", []string{"-c", "foo quux bar baz"}}, + "quotes": {"bin/foo", "bar \"baz quux\" 'fred wilma'", []string{"-c", "bin/foo bar \"baz quux\" 'fred wilma'"}}, + } { + t.Run(desc, c.Assert) + } +} From 9580353dca4d4a46dff902318a6322e1fb3564c2 Mon Sep 17 00:00:00 2001 From: "brian m. carlson" Date: Fri, 14 Sep 2018 22:46:17 +0000 Subject: [PATCH 4/5] subprocess: add a function to shell quote a single string We currently have a function to shell quote a list of strings, but in some cases we may want to just quote a single string. To avoid the complexity of having to create a bunch of temporaries and then index out the result, create a function that shell quotes just a single string. --- subprocess/subprocess.go | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/subprocess/subprocess.go b/subprocess/subprocess.go index fe1bfce2..621fed7e 100644 --- a/subprocess/subprocess.go +++ b/subprocess/subprocess.go @@ -86,18 +86,22 @@ func Output(cmd *Cmd) (string, error) { var shellWordRe = regexp.MustCompile(`\A[A-Za-z0-9_@/.-]+\z`) +// ShellQuoteSingle returns a string which is quoted suitably for sh. +func ShellQuoteSingle(str string) string { + // Quote anything that looks slightly complicated. + if shellWordRe.FindStringIndex(str) == nil { + return "'" + strings.Replace(str, "'", "'\\''", -1) + "'" + } + return str +} + // ShellQuote returns a copied string slice where each element is quoted // suitably for sh. func ShellQuote(strs []string) []string { dup := make([]string, 0, len(strs)) for _, str := range strs { - // Quote anything that looks slightly complicated. - if shellWordRe.FindStringIndex(str) == nil { - dup = append(dup, "'"+strings.Replace(str, "'", "'\\''", -1)+"'") - } else { - dup = append(dup, str) - } + dup = append(dup, ShellQuoteSingle(str)) } return dup } From 764607543192a8c458969372efbf9c2dde5ca96b Mon Sep 17 00:00:00 2001 From: "brian m. carlson" Date: Fri, 14 Sep 2018 23:52:38 +0000 Subject: [PATCH 5/5] tq: pass custom transfer adapter args to the shell When a custom transfer adapter is specified in the configuration, it is not possible to specify multiple arguments to the process, because only the last value of the lfs.customtransfer.*.args option is read and the value is not split. To make things easier and more flexible, concatenate the path (after quoting) and the arguments and pass them to the shell. Update the documentation to reflect this change. Update the test custom transfer adapter to parse its arguments (which are otherwise ignored) and mention them to standard error. Make the test check that the arguments are parsed as the shell would expect them to be. --- docs/custom-transfers.md | 3 ++- docs/man/git-lfs-config.5.ronn | 2 +- t/cmd/lfstest-standalonecustomadapter.go | 4 ++++ t/t-custom-transfers.sh | 8 +++++++- tq/custom.go | 3 ++- 5 files changed, 16 insertions(+), 4 deletions(-) diff --git a/docs/custom-transfers.md b/docs/custom-transfers.md index 4984747d..6c27bd0e 100644 --- a/docs/custom-transfers.md +++ b/docs/custom-transfers.md @@ -56,7 +56,8 @@ A custom transfer process is defined under a settings group called If the custom transfer process requires any arguments, these can be provided here. Typically you would only need this if your process was multi-purpose or - particularly flexible, most of the time you won't need it. + particularly flexible, most of the time you won't need it. Note that this + string will be expanded by the shell. * `lfs.customtransfer..concurrent` diff --git a/docs/man/git-lfs-config.5.ronn b/docs/man/git-lfs-config.5.ronn index 5ad63751..8b71b224 100644 --- a/docs/man/git-lfs-config.5.ronn +++ b/docs/man/git-lfs-config.5.ronn @@ -128,7 +128,7 @@ be scoped inside the configuration for a remote. * `lfs.customtransfer..args` If the custom transfer process requires any arguments, these can be provided - here. + here. This string will be expanded by the shell. * `lfs.customtransfer..concurrent` diff --git a/t/cmd/lfstest-standalonecustomadapter.go b/t/cmd/lfstest-standalonecustomadapter.go index 148c1788..94cb8940 100644 --- a/t/cmd/lfstest-standalonecustomadapter.go +++ b/t/cmd/lfstest-standalonecustomadapter.go @@ -28,6 +28,10 @@ func main() { os.Exit(1) } + for _, arg := range os.Args { + writeToStderr(fmt.Sprintf("Saw argument %q\n", arg), errWriter) + } + for scanner.Scan() { line := scanner.Text() var req request diff --git a/t/t-custom-transfers.sh b/t/t-custom-transfers.sh index cc1e026a..94b37e22 100755 --- a/t/t-custom-transfers.sh +++ b/t/t-custom-transfers.sh @@ -29,7 +29,7 @@ begin_test "custom-transfer-wrong-path" # use PIPESTATUS otherwise we get exit code from tee res=${PIPESTATUS[0]} grep "xfer: adapter \"testcustom\" Begin()" pushcustom.log - grep "Failed to start custom transfer command" pushcustom.log + grep "xfer: Aborting worker process" pushcustom.log if [ "$res" = "0" ]; then echo "Push should have failed because of an incorrect custom transfer path." exit 1 @@ -123,6 +123,7 @@ begin_test "custom-transfer-standalone" # set up custom transfer adapter to use a specific transfer agent git config lfs.customtransfer.testcustom.path lfstest-standalonecustomadapter + git config lfs.customtransfer.testcustom.args "--arg1 '--arg2 --arg3' --arg4" git config lfs.customtransfer.testcustom.concurrent false git config lfs.standalonetransferagent testcustom export TEST_STANDALONE_BACKUP_PATH="$(pwd)/test-custom-transfer-standalone-backup" @@ -187,6 +188,11 @@ begin_test "custom-transfer-standalone" grep "Terminating test custom adapter gracefully" fetchcustom.log + # Test argument parsing. + grep 'Saw argument "--arg1"' fetchcustom.log + grep 'Saw argument "--arg2 --arg3"' fetchcustom.log + grep 'Saw argument "--arg4"' fetchcustom.log + objectlist=`find .git/lfs/objects -type f` [ "$(echo "$objectlist" | wc -l)" -eq 12 ] ) diff --git a/tq/custom.go b/tq/custom.go index a98c4505..ed75982b 100644 --- a/tq/custom.go +++ b/tq/custom.go @@ -128,7 +128,8 @@ func (a *customAdapter) WorkerStarting(workerNum int) (interface{}, error) { // Start a process per worker // If concurrent = false we have already dialled back workers to 1 a.Trace("xfer: starting up custom transfer process %q for worker %d", a.name, workerNum) - cmd := subprocess.ExecCommand(a.path, a.args) + cmdName, cmdArgs := subprocess.FormatForShell(subprocess.ShellQuoteSingle(a.path), a.args) + cmd := subprocess.ExecCommand(cmdName, cmdArgs...) outp, err := cmd.StdoutPipe() if err != nil { return nil, fmt.Errorf("Failed to get stdout for custom transfer command %q remote: %v", a.path, err)