From fba99bf8dd98f25a058d63345cefa4975e5edd85 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Clerget?= Date: Wed, 2 Jun 2021 16:05:29 +0200 Subject: [PATCH] Allow escaping commas and colons in source bind path with the following syntax: - `singularity shell -B /tmp/comma\\,separated_dir:/data image` - `singularity shell -B "/tmp/comma\,separated_dir":/data image` - `singularity shell -B /tmp/colon\\:sep_dir:/data image` - `singularity shell -B /tmp/"colon\:sep_dir":/data image` - Fixes #5923 - Fixes #5959 --- cmd/internal/cli/action_flags.go | 2 +- cmd/internal/cli/actions_linux.go | 2 +- e2e/actions/actions.go | 45 +++++++ pkg/cmdline/flag.go | 16 +++ .../engine/singularity/config/config.go | 119 +++++++++++++----- 5 files changed, 153 insertions(+), 31 deletions(-) diff --git a/cmd/internal/cli/action_flags.go b/cmd/internal/cli/action_flags.go index 425d6eaae4..baaafb7dcf 100644 --- a/cmd/internal/cli/action_flags.go +++ b/cmd/internal/cli/action_flags.go @@ -84,7 +84,7 @@ var actionAppFlag = cmdline.Flag{ var actionBindFlag = cmdline.Flag{ ID: "actionBindFlag", Value: &BindPaths, - DefaultValue: []string{}, + DefaultValue: cmdline.StringArray{}, // to allow commas in bind path Name: "bind", ShortHand: "B", Usage: "a user-bind path specification. spec has the format src[:dest[:opts]], where src and dest are outside and inside paths. If dest is not given, it is set equal to src. Mount options ('opts') may be specified as 'ro' (read-only) or 'rw' (read/write, which is the default). Multiple bind paths can be given by a comma separated list.", diff --git a/cmd/internal/cli/actions_linux.go b/cmd/internal/cli/actions_linux.go index 26ebd98db4..85f1902e3c 100644 --- a/cmd/internal/cli/actions_linux.go +++ b/cmd/internal/cli/actions_linux.go @@ -408,7 +408,7 @@ func execStarter(cobraCmd *cobra.Command, image string, args []string, name stri img.File.Close() } - binds, err := singularityConfig.ParseBindPath(strings.Join(BindPaths, ",")) + binds, err := singularityConfig.ParseBindPath(BindPaths) if err != nil { sylog.Fatalf("while parsing bind path: %s", err) } diff --git a/e2e/actions/actions.go b/e2e/actions/actions.go index f2e9dcb60b..75986fe5ef 100644 --- a/e2e/actions/actions.go +++ b/e2e/actions/actions.go @@ -14,6 +14,7 @@ import ( "os" "path/filepath" "strconv" + "strings" "syscall" "testing" "time" @@ -1065,6 +1066,8 @@ func (c actionTests) actionBinds(t *testing.T) { contCanaryFile := "/canary/file" hostCanaryFile := filepath.Join(hostCanaryDir, "file") + hostCanaryFileWithComma := filepath.Join(hostCanaryDir, "file,comma") + hostCanaryFileWithColon := filepath.Join(hostCanaryDir, "file:colon") canaryFileBind := hostCanaryFile + ":" + contCanaryFile canaryDirBind := hostCanaryDir + ":" + contCanaryDir @@ -1091,6 +1094,12 @@ func (c actionTests) actionBinds(t *testing.T) { if err := fs.Touch(hostCanaryFile); err != nil { t.Fatalf("failed to create canary_file: %s", err) } + if err := fs.Touch(hostCanaryFileWithComma); err != nil { + t.Fatalf("failed to create canary_file_comma: %s", err) + } + if err := fs.Touch(hostCanaryFileWithColon); err != nil { + t.Fatalf("failed to create canary_file_colon: %s", err) + } if err := os.Chmod(hostCanaryFile, 0777); err != nil { t.Fatalf("failed to apply permissions on canary_file: %s", err) } @@ -1467,6 +1476,42 @@ func (c actionTests) actionBinds(t *testing.T) { postRun: checkHostDir(filepath.Join(hostWorkDir, "scratch/scratch", "dir")), exit: 0, }, + { + name: "BindFileWithCommaOK", + args: []string{ + "--bind", strings.ReplaceAll(hostCanaryFileWithComma, ",", "\\,") + ":" + contCanaryFile, + sandbox, + "test", "-f", contCanaryFile, + }, + exit: 0, + }, + { + name: "BindFileWithCommaKO", + args: []string{ + "--bind", hostCanaryFileWithComma + ":" + contCanaryFile, + sandbox, + "test", "-f", contCanaryFile, + }, + exit: 255, + }, + { + name: "BindFileWithColonOK", + args: []string{ + "--bind", strings.ReplaceAll(hostCanaryFileWithColon, ":", "\\:") + ":" + contCanaryFile, + sandbox, + "test", "-f", contCanaryFile, + }, + exit: 0, + }, + { + name: "BindFileWithColonKO", + args: []string{ + "--bind", hostCanaryFileWithColon + ":" + contCanaryFile, + sandbox, + "test", "-f", contCanaryFile, + }, + exit: 255, + }, } for _, profile := range e2e.Profiles { diff --git a/pkg/cmdline/flag.go b/pkg/cmdline/flag.go index df7b3eefca..5ebd12b6e1 100644 --- a/pkg/cmdline/flag.go +++ b/pkg/cmdline/flag.go @@ -22,6 +22,8 @@ const ( Linux = "linux" ) +type StringArray []string + // Flag holds information about a command flag type Flag struct { ID string @@ -92,6 +94,8 @@ func (m *flagManager) registerFlagForCmd(flag *Flag, cmds ...*cobra.Command) err m.registerStringVar(flag, cmds) case []string: m.registerStringSliceVar(flag, cmds) + case StringArray: + m.registerStringArrayVar(flag, cmds) case bool: m.registerBoolVar(flag, cmds) case int: @@ -129,6 +133,18 @@ func (m *flagManager) registerStringSliceVar(flag *Flag, cmds []*cobra.Command) return nil } +func (m *flagManager) registerStringArrayVar(flag *Flag, cmds []*cobra.Command) error { + for _, c := range cmds { + if flag.ShortHand != "" { + c.Flags().StringArrayVarP(flag.Value.(*[]string), flag.Name, flag.ShortHand, ([]string)(flag.DefaultValue.(StringArray)), flag.Usage) + } else { + c.Flags().StringArrayVar(flag.Value.(*[]string), flag.Name, ([]string)(flag.DefaultValue.(StringArray)), flag.Usage) + } + m.setFlagOptions(flag, c) + } + return nil +} + func (m *flagManager) registerBoolVar(flag *Flag, cmds []*cobra.Command) error { for _, c := range cmds { if flag.ShortHand != "" { diff --git a/pkg/runtime/engine/singularity/config/config.go b/pkg/runtime/engine/singularity/config/config.go index 2bc86ec3e4..cbfe8caffb 100644 --- a/pkg/runtime/engine/singularity/config/config.go +++ b/pkg/runtime/engine/singularity/config/config.go @@ -284,10 +284,8 @@ func (e *EngineConfig) GetCustomHome() bool { // ParseBindPath parses a string and returns all encountered // bind paths as array. -func ParseBindPath(bindpaths string) ([]BindPath, error) { - var bind string +func ParseBindPath(paths []string) ([]BindPath, error) { var binds []BindPath - var elem int var validOptions = map[string]bool{ "ro": true, @@ -312,13 +310,16 @@ func ParseBindPath(bindpaths string) ([]BindPath, error) { // - source1:destination1 -> [source1:, destination1] // - source1:destination1:option1 -> [source1:, destination1:, option1] // - source1:destination1:option1,option2 -> [source1:, destination1:, option1, option2] - for _, m := range re.FindAllString(bindpaths, -1) { - s := strings.TrimSpace(m) - isColon := bind != "" && bind[len(bind)-1] == ':' - // options are taken only if the bind has a source - // and a destination - if elem == 2 { + for _, path := range paths { + concatComma := false + concatColon := false + bind := "" + elem := 0 + + for _, m := range re.FindAllString(path, -1) { + s := strings.TrimSpace(m) + isOption := false for option, flag := range validOptions { @@ -334,44 +335,104 @@ func ParseBindPath(bindpaths string) ([]BindPath, error) { } } } - if isOption { + + if elem == 2 && !isOption { + bp, err := newBindPath(bind, validOptions) + if err != nil { + return nil, fmt.Errorf("while getting bind path: %s", err) + } + binds = append(binds, bp) + elem = 0 + bind = "" + } + + if elem == 0 { + // escaped commas and colons + if (len(s) > 0 && s[len(s)-1] == '\\') || concatComma { + if !concatComma { + bind += s[:len(s)-1] + "," + } else { + bind += s + elem++ + } + concatComma = !concatComma + continue + } else if (len(s) >= 2 && s[len(s)-2] == '\\' && s[len(s)-1] == ':') || concatColon { + bind += s + if concatColon { + elem++ + } + concatColon = !concatColon + continue + } else if bind == "" { + bind = s + } + } + + isColon := bind != "" && bind[len(bind)-1] == ':' + + // options are taken only if the bind has a source + // and a destination + if elem == 2 && isOption { if !isColon { bind += "," } bind += s continue + } else if elem > 2 { + return nil, fmt.Errorf("wrong bind syntax: %s", bind) } - } else if elem > 2 { - return nil, fmt.Errorf("wrong bind syntax: %s", bind) - } - elem++ + if bind != "" { + if isColon { + if elem > 0 { + bind += s + } + fmt.Println("append", bind, s) + //bind += s + elem++ + continue + } + bp, err := newBindPath(bind, validOptions) + if err != nil { + return nil, fmt.Errorf("while getting bind path: %s", err) + } + binds = append(binds, bp) + elem = 0 + } + // new bind path + bind = s + elem++ + } if bind != "" { - if isColon { - bind += s - continue - } bp, err := newBindPath(bind, validOptions) if err != nil { return nil, fmt.Errorf("while getting bind path: %s", err) } binds = append(binds, bp) - elem = 1 } - // new bind path - bind = s } - if bind != "" { - bp, err := newBindPath(bind, validOptions) - if err != nil { - return nil, fmt.Errorf("while getting bind path: %s", err) + return binds, nil +} + +func splitBy(str string, sep byte) []string { + var list []string + + re := regexp.MustCompile(fmt.Sprintf(`(?m)([^\\]%c)`, sep)) + cursor := 0 + + indexes := re.FindAllStringIndex(str, -1) + for i, index := range indexes { + list = append(list, str[cursor:index[1]-1]) + cursor = index[1] + if len(indexes)-1 == i { + return append(list, str[cursor:]) } - binds = append(binds, bp) } - return binds, nil + return append(list, str) } // newBindPath returns BindPath record based on the provided bind @@ -379,9 +440,9 @@ func ParseBindPath(bindpaths string) ([]BindPath, error) { func newBindPath(bind string, validOptions map[string]bool) (BindPath, error) { var bp BindPath - splitted := strings.SplitN(bind, ":", 3) + splitted := splitBy(bind, ':') - bp.Source = splitted[0] + bp.Source = strings.ReplaceAll(splitted[0], "\\:", ":") if bp.Source == "" { return bp, fmt.Errorf("empty bind source for bind path %q", bind) }