Merge pull request #15766 from hqhq/hq_fix_device

Add mode check for device

daemon/volumes_unix.go

@@ -71,11 +71,10 @@ func parseBindMount(spec string, mountLabel string, config *runconfig.Config) (*
 	case 3:
 		bind.Destination = arr[1]
 		mode := arr[2]
-		isValid, isRw := volume.ValidateMountMode(mode)
+		if !volume.ValidMountMode(mode) {
-		if !isValid {
 			return nil, fmt.Errorf("invalid mode for volumes-from: %s", mode)
 		}
-		bind.RW = isRw
+		bind.RW = volume.ReadWrite(mode)
 		// Mode field is used by SELinux to decide whether to apply label
 		bind.Mode = mode
 	default:
@@ -268,7 +267,7 @@ func parseVolumesFrom(spec string) (string, string, error) {
 
 	if len(specParts) == 2 {
 		mode = specParts[1]
-		if isValid, _ := volume.ValidateMountMode(mode); !isValid {
+		if !volume.ValidMountMode(mode) {
 			return "", "", fmt.Errorf("invalid mode for volumes-from: %s", mode)
 		}
 	}

integration-cli/docker_cli_run_test.go

@@ -785,6 +785,20 @@ func (s *DockerSuite) TestRunAddingOptionalDevices(c *check.C) {
 	}
 }
 
+func (s *DockerSuite) TestRunAddingOptionalDevicesNoSrc(c *check.C) {
+	out, _ := dockerCmd(c, "run", "--device", "/dev/zero:rw", "busybox", "sh", "-c", "ls /dev/zero")
+	if actual := strings.Trim(out, "\r\n"); actual != "/dev/zero" {
+		c.Fatalf("expected output /dev/zero, received %s", actual)
+	}
+}
+
+func (s *DockerSuite) TestRunAddingOptionalDevicesInvalideMode(c *check.C) {
+	_, _, err := dockerCmdWithError("run", "--device", "/dev/zero:ro", "busybox", "sh", "-c", "ls /dev/zero")
+	if err == nil {
+		c.Fatalf("run container with device mode ro should fail")
+	}
+}
+
 func (s *DockerSuite) TestRunModeHostname(c *check.C) {
 	testRequires(c, SameHostDaemon)
 

opts/opts.go

@@ -170,11 +170,32 @@ func ValidateLink(val string) (string, error) {
 	return val, nil
 }
 
+// ValidDeviceMode checks if the mode for device is valid or not.
+// Valid mode is a composition of r (read), w (write), and m (mknod).
+func ValidDeviceMode(mode string) bool {
+	var legalDeviceMode = map[rune]bool{
+		'r': true,
+		'w': true,
+		'm': true,
+	}
+	if mode == "" {
+		return false
+	}
+	for _, c := range mode {
+		if !legalDeviceMode[c] {
+			return false
+		}
+		legalDeviceMode[c] = false
+	}
+	return true
+}
+
 // ValidateDevice Validate a path for devices
 // It will make sure 'val' is in the form:
 //    [host-dir:]container-path[:mode]
+// It will also validate the device mode.
 func ValidateDevice(val string) (string, error) {
-	return validatePath(val, false)
+	return validatePath(val, ValidDeviceMode)
 }
 
 // ValidatePath Validate a path for volumes
@@ -182,27 +203,27 @@ func ValidateDevice(val string) (string, error) {
 //    [host-dir:]container-path[:rw|ro]
 // It will also validate the mount mode.
 func ValidatePath(val string) (string, error) {
-	return validatePath(val, true)
+	return validatePath(val, volume.ValidMountMode)
 }
 
-func validatePath(val string, validateMountMode bool) (string, error) {
+func validatePath(val string, validator func(string) bool) (string, error) {
 	var containerPath string
 	var mode string
 
 	if strings.Count(val, ":") > 2 {
-		return val, fmt.Errorf("bad format for volumes: %s", val)
+		return val, fmt.Errorf("bad format for path: %s", val)
 	}
 
 	splited := strings.SplitN(val, ":", 3)
 	if splited[0] == "" {
-		return val, fmt.Errorf("bad format for volumes: %s", val)
+		return val, fmt.Errorf("bad format for path: %s", val)
 	}
 	switch len(splited) {
 	case 1:
 		containerPath = splited[0]
 		val = path.Clean(containerPath)
 	case 2:
-		if isValid, _ := volume.ValidateMountMode(splited[1]); validateMountMode && isValid {
+		if isValid := validator(splited[1]); isValid {
 			containerPath = splited[0]
 			mode = splited[1]
 			val = fmt.Sprintf("%s:%s", path.Clean(containerPath), mode)
@@ -213,8 +234,8 @@ func validatePath(val string, validateMountMode bool) (string, error) {
 	case 3:
 		containerPath = splited[1]
 		mode = splited[2]
-		if isValid, _ := volume.ValidateMountMode(splited[2]); validateMountMode && !isValid {
+		if isValid := validator(splited[2]); !isValid {
-			return val, fmt.Errorf("bad mount mode specified : %s", mode)
+			return val, fmt.Errorf("bad mode specified: %s", mode)
 		}
 		val = fmt.Sprintf("%s:%s:%s", splited[0], containerPath, mode)
 	}

opts/opts_test.go

@@ -289,24 +289,24 @@ func TestValidatePath(t *testing.T) {
 		"/rw:rw",
 	}
 	invalid := map[string]string{
-		"":                "bad format for volumes: ",
+		"":                "bad format for path: ",
 		"./":              "./ is not an absolute path",
 		"../":             "../ is not an absolute path",
 		"/:../":           "../ is not an absolute path",
 		"/:path":          "path is not an absolute path",
-		":":               "bad format for volumes: :",
+		":":               "bad format for path: :",
 		"/tmp:":           " is not an absolute path",
-		":test":           "bad format for volumes: :test",
+		":test":           "bad format for path: :test",
-		":/test":          "bad format for volumes: :/test",
+		":/test":          "bad format for path: :/test",
 		"tmp:":            " is not an absolute path",
-		":test:":          "bad format for volumes: :test:",
+		":test:":          "bad format for path: :test:",
-		"::":              "bad format for volumes: ::",
+		"::":              "bad format for path: ::",
-		":::":             "bad format for volumes: :::",
+		":::":             "bad format for path: :::",
-		"/tmp:::":         "bad format for volumes: /tmp:::",
+		"/tmp:::":         "bad format for path: /tmp:::",
-		":/tmp::":         "bad format for volumes: :/tmp::",
+		":/tmp::":         "bad format for path: :/tmp::",
 		"path:ro":         "path is not an absolute path",
-		"/path:/path:sw":  "bad mount mode specified : sw",
+		"/path:/path:sw":  "bad mode specified: sw",
-		"/path:/path:rwz": "bad mount mode specified : rwz",
+		"/path:/path:rwz": "bad mode specified: rwz",
 	}
 
 	for _, path := range valid {
@@ -333,27 +333,30 @@ func TestValidateDevice(t *testing.T) {
 		"/with space",
 		"/home:/with space",
 		"relative:/absolute-path",
-		"hostPath:/containerPath:ro",
+		"hostPath:/containerPath:r",
 		"/hostPath:/containerPath:rw",
 		"/hostPath:/containerPath:mrw",
 	}
 	invalid := map[string]string{
-		"":        "bad format for volumes: ",
+		"":        "bad format for path: ",
 		"./":      "./ is not an absolute path",
 		"../":     "../ is not an absolute path",
 		"/:../":   "../ is not an absolute path",
 		"/:path":  "path is not an absolute path",
-		":":       "bad format for volumes: :",
+		":":       "bad format for path: :",
 		"/tmp:":   " is not an absolute path",
-		":test":   "bad format for volumes: :test",
+		":test":   "bad format for path: :test",
-		":/test":  "bad format for volumes: :/test",
+		":/test":  "bad format for path: :/test",
 		"tmp:":    " is not an absolute path",
-		":test:":  "bad format for volumes: :test:",
+		":test:":  "bad format for path: :test:",
-		"::":      "bad format for volumes: ::",
+		"::":      "bad format for path: ::",
-		":::":     "bad format for volumes: :::",
+		":::":     "bad format for path: :::",
-		"/tmp:::": "bad format for volumes: /tmp:::",
+		"/tmp:::": "bad format for path: /tmp:::",
-		":/tmp::": "bad format for volumes: :/tmp::",
+		":/tmp::": "bad format for path: :/tmp::",
 		"path:ro": "ro is not an absolute path",
+		"path:rr": "rr is not an absolute path",
+		"a:/b:ro": "bad mode specified: ro",
+		"a:/b:rr": "bad mode specified: rr",
 	}
 
 	for _, path := range valid {

runconfig/parse.go

@@ -474,7 +474,11 @@ func ParseDevice(device string) (DeviceMapping, error) {
 		permissions = arr[2]
 		fallthrough
 	case 2:
+		if opts.ValidDeviceMode(arr[1]) {
+			permissions = arr[1]
+		} else {
 			dst = arr[1]
+		}
 		fallthrough
 	case 1:
 		src = arr[0]

runconfig/parse_test.go

@@ -317,15 +317,20 @@ func TestParseDevice(t *testing.T) {
 			PathInContainer:   "/dev/snd",
 			CgroupPermissions: "rwm",
 		},
+		"/dev/snd:rw": {
+			PathOnHost:        "/dev/snd",
+			PathInContainer:   "/dev/snd",
+			CgroupPermissions: "rw",
+		},
 		"/dev/snd:/something": {
 			PathOnHost:        "/dev/snd",
 			PathInContainer:   "/something",
 			CgroupPermissions: "rwm",
 		},
-		"/dev/snd:/something:ro": {
+		"/dev/snd:/something:rw": {
 			PathOnHost:        "/dev/snd",
 			PathInContainer:   "/something",
-			CgroupPermissions: "ro",
+			CgroupPermissions: "rw",
 		},
 	}
 	for device, deviceMapping := range valids {

volume/volume.go

@@ -49,13 +49,13 @@ var roModes = map[string]bool{
 	"Z,ro": true,
 }
 
-// ValidateMountMode will make sure the mount mode is valid.
+// ValidMountMode will make sure the mount mode is valid.
-// returns if it's a valid mount mode and if it's read-write or not.
+// returns if it's a valid mount mode or not.
-func ValidateMountMode(mode string) (bool, bool) {
+func ValidMountMode(mode string) bool {
-	return roModes[mode] || rwModes[mode], rwModes[mode]
+	return roModes[mode] || rwModes[mode]
 }
 
-// ReadWrite tells you if a mode string is a valid read-only mode or not.
+// ReadWrite tells you if a mode string is a valid read-write mode or not.
 func ReadWrite(mode string) bool {
 	return rwModes[mode]
 }