Inconsistent --tmpfs behavior

This fix tries to address the issue raised in #22420. When
`--tmpfs` is specified with `/tmp`, the default value is
`rw,nosuid,nodev,noexec,relatime,size=65536k`. When `--tmpfs`
is specified with `/tmp:rw`, then the value changed to
`rw,nosuid,nodev,noexec,relatime`.

The reason for such an inconsistency is because docker tries
to add `size=65536k` option only when user provides no option.

This fix tries to address this issue by always pre-progating
`size=65536k` along with `rw,nosuid,nodev,noexec,relatime`.
If user provides a different value (e.g., `size=8192k`), it
will override the `size=65536k` anyway since the combined
options will be parsed and merged to remove any duplicates.

Additional test cases have been added to cover the changes
in this fix.

This fix fixes #22420.

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

mount/flags.go

@@ -5,15 +5,7 @@ import (
 	"strings"
 )
 
-// Parse fstab type mount options into mount() flags
+var flags = map[string]struct {
-// and device specific data
-func parseOptions(options string) (int, string) {
-	var (
-		flag int
-		data []string
-	)
-
-	flags := map[string]struct {
 	clear bool
 	flag  int
 }{
@@ -52,6 +44,81 @@ func parseOptions(options string) (int, string) {
 	"nostrictatime": {true, STRICTATIME},
 }
 
+var validFlags = map[string]bool{
+	"":          true,
+	"size":      true,
+	"mode":      true,
+	"uid":       true,
+	"gid":       true,
+	"nr_inodes": true,
+	"nr_blocks": true,
+	"mpol":      true,
+}
+
+var propagationFlags = map[string]bool{
+	"bind":        true,
+	"rbind":       true,
+	"unbindable":  true,
+	"runbindable": true,
+	"private":     true,
+	"rprivate":    true,
+	"shared":      true,
+	"rshared":     true,
+	"slave":       true,
+	"rslave":      true,
+}
+
+// MergeTmpfsOptions merge mount options to make sure there is no duplicate.
+func MergeTmpfsOptions(options []string) ([]string, error) {
+	// We use collisions maps to remove duplicates.
+	// For flag, the key is the flag value (the key for propagation flag is -1)
+	// For data=value, the key is the data
+	flagCollisions := map[int]bool{}
+	dataCollisions := map[string]bool{}
+
+	var newOptions []string
+	// We process in reverse order
+	for i := len(options) - 1; i >= 0; i-- {
+		option := options[i]
+		if option == "defaults" {
+			continue
+		}
+		if f, ok := flags[option]; ok && f.flag != 0 {
+			// There is only one propagation mode
+			key := f.flag
+			if propagationFlags[option] {
+				key = -1
+			}
+			// Check to see if there is collision for flag
+			if !flagCollisions[key] {
+				// We prepend the option and add to collision map
+				newOptions = append([]string{option}, newOptions...)
+				flagCollisions[key] = true
+			}
+			continue
+		}
+		opt := strings.SplitN(option, "=", 2)
+		if len(opt) != 2 || !validFlags[opt[0]] {
+			return nil, fmt.Errorf("Invalid tmpfs option %q", opt)
+		}
+		if !dataCollisions[opt[0]] {
+			// We prepend the option and add to collision map
+			newOptions = append([]string{option}, newOptions...)
+			dataCollisions[opt[0]] = true
+		}
+	}
+
+	return newOptions, nil
+}
+
+// Parse fstab type mount options into mount() flags
+// and device specific data
+func parseOptions(options string) (int, string) {
+	var (
+		flag int
+		data []string
+	)
+
 	for _, o := range strings.Split(options, ",") {
 		// If the option does not exist in the flags table or the flag
 		// is not supported on the platform,
@@ -72,16 +139,6 @@ func parseOptions(options string) (int, string) {
 // ParseTmpfsOptions parse fstab type mount options into flags and data
 func ParseTmpfsOptions(options string) (int, string, error) {
 	flags, data := parseOptions(options)
-	validFlags := map[string]bool{
-		"":          true,
-		"size":      true,
-		"mode":      true,
-		"uid":       true,
-		"gid":       true,
-		"nr_inodes": true,
-		"nr_blocks": true,
-		"mpol":      true,
-	}
 	for _, o := range strings.Split(data, ",") {
 		opt := strings.SplitN(o, "=", 2)
 		if !validFlags[opt[0]] {

mount/mount_unix_test.go

@@ -137,3 +137,26 @@ func TestGetMounts(t *testing.T) {
 		t.Fatal("/ should be mounted at least")
 	}
 }
+
+func TestMergeTmpfsOptions(t *testing.T) {
+	options := []string{"noatime", "ro", "size=10k", "defaults", "atime", "defaults", "rw", "rprivate", "size=1024k", "slave"}
+	expected := []string{"atime", "rw", "size=1024k", "slave"}
+	merged, err := MergeTmpfsOptions(options)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(expected) != len(merged) {
+		t.Fatalf("Expected %s got %s", expected, merged)
+	}
+	for index := range merged {
+		if merged[index] != expected[index] {
+			t.Fatalf("Expected %s for the %dth option, got %s", expected, index, merged)
+		}
+	}
+
+	options = []string{"noatime", "ro", "size=10k", "atime", "rw", "rprivate", "size=1024k", "slave", "size"}
+	_, err = MergeTmpfsOptions(options)
+	if err == nil {
+		t.Fatal("Expected error got nil")
+	}
+}