From bcb89fc8b2e7cd3bfe1b5017c0e9246608a399f5 Mon Sep 17 00:00:00 2001
From: Daniel J Walsh <dwalsh@redhat.com>
Date: Tue, 27 Jun 2023 08:40:10 -0400
Subject: [PATCH] Fix readonly=false failure

There was a huge cut and paste of mount options which were not constent
in parsing tmpfs, bind and volume mounts.  Consolidated into a single
function to guarantee all parse the same.

Fixes: https://github.com/containers/podman/issues/18995

Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
---
 pkg/specgenutil/volumes.go | 365 +++++++++++++++----------------------
 test/system/060-mount.bats |  16 ++
 2 files changed, 158 insertions(+), 223 deletions(-)

diff --git a/pkg/specgenutil/volumes.go b/pkg/specgenutil/volumes.go
index c70206addf..c77710ffea 100644
--- a/pkg/specgenutil/volumes.go
+++ b/pkg/specgenutil/volumes.go
@@ -236,22 +236,39 @@ func Mounts(mountFlag []string) (map[string]spec.Mount, map[string]*specgen.Name
 	return finalMounts, finalNamedVolumes, finalImageVolumes, nil
 }
 
-// Parse a single bind mount entry from the --mount flag.
-func getBindMount(args []string) (spec.Mount, error) {
-	newMount := spec.Mount{
-		Type: define.TypeBind,
-	}
-
-	var setSource, setDest, setRORW, setSuid, setDev, setExec, setRelabel, setOwnership bool
+func parseMountOptions(mountType string, args []string) (*spec.Mount, error) {
+	var setTmpcopyup, setRORW, setSuid, setDev, setExec, setRelabel, setOwnership bool
 
+	mnt := spec.Mount{}
 	for _, val := range args {
 		kv := strings.SplitN(val, "=", 2)
 		switch kv[0] {
 		case "bind-nonrecursive":
-			newMount.Options = append(newMount.Options, "bind")
+			if mountType != define.TypeBind {
+				return nil, fmt.Errorf("%q option not supported for %q mount types", kv[0], mountType)
+			}
+			mnt.Options = append(mnt.Options, "bind")
+		case "bind-propagation":
+			if mountType != define.TypeBind {
+				return nil, fmt.Errorf("%q option not supported for %q mount types", kv[0], mountType)
+			}
+			if len(kv) == 1 {
+				return nil, fmt.Errorf("%v: %w", kv[0], errOptionArg)
+			}
+			mnt.Options = append(mnt.Options, kv[1])
+		case "consistency":
+			// Often used on MACs and mistakenly on Linux platforms.
+			// Since Docker ignores this option so shall we.
+			continue
+		case "idmap":
+			if len(kv) > 1 {
+				mnt.Options = append(mnt.Options, fmt.Sprintf("idmap=%s", kv[1]))
+			} else {
+				mnt.Options = append(mnt.Options, "idmap")
+			}
 		case "readonly", "ro", "rw":
 			if setRORW {
-				return newMount, fmt.Errorf("cannot pass 'readonly', 'ro', or 'rw' options more than once: %w", errOptionArg)
+				return nil, fmt.Errorf("cannot pass 'readonly', 'ro', or 'rw' mnt.Options more than once: %w", errOptionArg)
 			}
 			setRORW = true
 			// Can be formatted as one of:
@@ -266,121 +283,162 @@ func getBindMount(args []string) (spec.Mount, error) {
 			}
 			switch len(kv) {
 			case 1:
-				newMount.Options = append(newMount.Options, kv[0])
+				mnt.Options = append(mnt.Options, kv[0])
 			case 2:
 				switch strings.ToLower(kv[1]) {
 				case "true":
-					newMount.Options = append(newMount.Options, kv[0])
+					mnt.Options = append(mnt.Options, kv[0])
 				case "false":
 					// Set the opposite only for rw
 					// ro's opposite is the default
 					if kv[0] == "rw" {
-						newMount.Options = append(newMount.Options, "ro")
+						mnt.Options = append(mnt.Options, "ro")
 					}
-				default:
-					return newMount, fmt.Errorf("'readonly', 'ro', or 'rw' must be set to true or false, instead received %q: %w", kv[1], errOptionArg)
 				}
-			default:
-				return newMount, fmt.Errorf("badly formatted option %q: %w", val, errOptionArg)
 			}
-		case "nosuid", "suid":
-			if setSuid {
-				return newMount, fmt.Errorf("cannot pass 'nosuid' and 'suid' options more than once: %w", errOptionArg)
-			}
-			setSuid = true
-			newMount.Options = append(newMount.Options, kv[0])
 		case "nodev", "dev":
 			if setDev {
-				return newMount, fmt.Errorf("cannot pass 'nodev' and 'dev' options more than once: %w", errOptionArg)
+				return nil, fmt.Errorf("cannot pass 'nodev' and 'dev' mnt.Options more than once: %w", errOptionArg)
 			}
 			setDev = true
-			newMount.Options = append(newMount.Options, kv[0])
+			mnt.Options = append(mnt.Options, kv[0])
 		case "noexec", "exec":
 			if setExec {
-				return newMount, fmt.Errorf("cannot pass 'noexec' and 'exec' options more than once: %w", errOptionArg)
+				return nil, fmt.Errorf("cannot pass 'noexec' and 'exec' mnt.Options more than once: %w", errOptionArg)
 			}
 			setExec = true
-			newMount.Options = append(newMount.Options, kv[0])
-		case "shared", "rshared", "private", "rprivate", "slave", "rslave", "unbindable", "runbindable", "Z", "z":
-			newMount.Options = append(newMount.Options, kv[0])
-		case "bind-propagation":
-			if len(kv) == 1 {
-				return newMount, fmt.Errorf("%v: %w", kv[0], errOptionArg)
+			mnt.Options = append(mnt.Options, kv[0])
+		case "nosuid", "suid":
+			if setSuid {
+				return nil, fmt.Errorf("cannot pass 'nosuid' and 'suid' mnt.Options more than once: %w", errOptionArg)
+			}
+			setSuid = true
+			mnt.Options = append(mnt.Options, kv[0])
+		case "relabel":
+			if setRelabel {
+				return nil, fmt.Errorf("cannot pass 'relabel' option more than once: %w", errOptionArg)
+			}
+			setRelabel = true
+			if len(kv) != 2 {
+				return nil, fmt.Errorf("%s mount option must be 'private' or 'shared': %w", kv[0], util.ErrBadMntOption)
+			}
+			switch kv[1] {
+			case "private":
+				mnt.Options = append(mnt.Options, "Z")
+			case "shared":
+				mnt.Options = append(mnt.Options, "z")
+			default:
+				return nil, fmt.Errorf("%s mount option must be 'private' or 'shared': %w", kv[0], util.ErrBadMntOption)
 			}
-			newMount.Options = append(newMount.Options, kv[1])
+		case "shared", "rshared", "private", "rprivate", "slave", "rslave", "unbindable", "runbindable", "Z", "z":
+			mnt.Options = append(mnt.Options, kv[0])
 		case "src", "source":
+			if mountType == define.TypeTmpfs {
+				return nil, fmt.Errorf("%q option not supported for %q mount types", kv[0], mountType)
+			}
+			if mnt.Source != "" {
+				return nil, fmt.Errorf("cannot pass %q option more than once: %w", kv[0], errOptionArg)
+			}
 			if len(kv) == 1 {
-				return newMount, fmt.Errorf("%v: %w", kv[0], errOptionArg)
+				return nil, fmt.Errorf("%v: %w", kv[0], errOptionArg)
 			}
 			if len(kv[1]) == 0 {
-				return newMount, fmt.Errorf("host directory cannot be empty: %w", errOptionArg)
+				return nil, fmt.Errorf("host directory cannot be empty: %w", errOptionArg)
 			}
-			newMount.Source = kv[1]
-			setSource = true
+			mnt.Source = kv[1]
 		case "target", "dst", "destination":
+			if mnt.Destination != "" {
+				return nil, fmt.Errorf("cannot pass %q option more than once: %w", kv[0], errOptionArg)
+			}
 			if len(kv) == 1 {
-				return newMount, fmt.Errorf("%v: %w", kv[0], errOptionArg)
+				return nil, fmt.Errorf("%v: %w", kv[0], errOptionArg)
 			}
 			if err := parse.ValidateVolumeCtrDir(kv[1]); err != nil {
-				return newMount, err
+				return nil, err
 			}
-			newMount.Destination = unixPathClean(kv[1])
-			setDest = true
-		case "relabel":
-			if setRelabel {
-				return newMount, fmt.Errorf("cannot pass 'relabel' option more than once: %w", errOptionArg)
+			mnt.Destination = unixPathClean(kv[1])
+		case "tmpcopyup", "notmpcopyup":
+			if mountType != define.TypeTmpfs {
+				return nil, fmt.Errorf("%q option not supported for %q mount types", kv[0], mountType)
 			}
-			setRelabel = true
-			if len(kv) != 2 {
-				return newMount, fmt.Errorf("%s mount option must be 'private' or 'shared': %w", kv[0], util.ErrBadMntOption)
+			if setTmpcopyup {
+				return nil, fmt.Errorf("cannot pass 'tmpcopyup' and 'notmpcopyup' mnt.Options more than once: %w", errOptionArg)
 			}
-			switch kv[1] {
-			case "private":
-				newMount.Options = append(newMount.Options, "Z")
-			case "shared":
-				newMount.Options = append(newMount.Options, "z")
-			default:
-				return newMount, fmt.Errorf("%s mount option must be 'private' or 'shared': %w", kv[0], util.ErrBadMntOption)
+			setTmpcopyup = true
+			mnt.Options = append(mnt.Options, kv[0])
+		case "tmpfs-mode":
+			if mountType != define.TypeTmpfs {
+				return nil, fmt.Errorf("%q option not supported for %q mount types", kv[0], mountType)
+			}
+			if len(kv) == 1 {
+				return nil, fmt.Errorf("%v: %w", kv[0], errOptionArg)
 			}
+			mnt.Options = append(mnt.Options, fmt.Sprintf("mode=%s", kv[1]))
+		case "tmpfs-size":
+			if mountType != define.TypeTmpfs {
+				return nil, fmt.Errorf("%q option not supported for %q mount types", kv[0], mountType)
+			}
+			if len(kv) == 1 {
+				return nil, fmt.Errorf("%v: %w", kv[0], errOptionArg)
+			}
+			mnt.Options = append(mnt.Options, fmt.Sprintf("size=%s", kv[1]))
 		case "U", "chown":
 			if setOwnership {
-				return newMount, fmt.Errorf("cannot pass 'U' or 'chown' option more than once: %w", errOptionArg)
+				return nil, fmt.Errorf("cannot pass 'U' or 'chown' option more than once: %w", errOptionArg)
 			}
 			ok, err := validChownFlag(val)
 			if err != nil {
-				return newMount, err
+				return nil, err
 			}
 			if ok {
-				newMount.Options = append(newMount.Options, "U")
+				mnt.Options = append(mnt.Options, "U")
 			}
 			setOwnership = true
-		case "idmap":
-			if len(kv) > 1 {
-				newMount.Options = append(newMount.Options, fmt.Sprintf("idmap=%s", kv[1]))
-			} else {
-				newMount.Options = append(newMount.Options, "idmap")
+		case "volume-label":
+			if mountType != define.TypeVolume {
+				return nil, fmt.Errorf("%q option not supported for %q mount types", kv[0], mountType)
 			}
-		case "consistency":
-			// Often used on MACs and mistakenly on Linux platforms.
-			// Since Docker ignores this option so shall we.
-			continue
+			return nil, fmt.Errorf("the --volume-label option is not presently implemented")
+		case "volume-opt":
+			if mountType != define.TypeVolume {
+				return nil, fmt.Errorf("%q option not supported for %q mount types", kv[0], mountType)
+			}
+			mnt.Options = append(mnt.Options, val)
 		default:
-			return newMount, fmt.Errorf("%s: %w", kv[0], util.ErrBadMntOption)
+			return nil, fmt.Errorf("%s: %w", kv[0], util.ErrBadMntOption)
 		}
 	}
+	if len(mnt.Destination) == 0 {
+		return nil, errNoDest
+	}
+	return &mnt, nil
+}
 
-	if !setDest {
+// Parse a single bind mount entry from the --mount flag.
+func getBindMount(args []string) (spec.Mount, error) {
+	newMount := spec.Mount{
+		Type: define.TypeBind,
+	}
+	var err error
+	mnt, err := parseMountOptions(newMount.Type, args)
+	if err != nil {
+		return newMount, err
+	}
+
+	if len(mnt.Destination) == 0 {
 		return newMount, errNoDest
 	}
 
-	if !setSource {
-		newMount.Source = newMount.Destination
+	if len(mnt.Source) == 0 {
+		mnt.Source = mnt.Destination
 	}
 
-	options, err := parse.ValidateVolumeOpts(newMount.Options)
+	options, err := parse.ValidateVolumeOpts(mnt.Options)
 	if err != nil {
 		return newMount, err
 	}
+	newMount.Source = mnt.Source
+	newMount.Destination = mnt.Destination
 	newMount.Options = options
 	return newMount, nil
 }
@@ -392,87 +450,16 @@ func getTmpfsMount(args []string) (spec.Mount, error) {
 		Source: define.TypeTmpfs,
 	}
 
-	var setDest, setRORW, setSuid, setDev, setExec, setTmpcopyup, setOwnership bool
-
-	for _, val := range args {
-		kv := strings.SplitN(val, "=", 2)
-		switch kv[0] {
-		case "tmpcopyup", "notmpcopyup":
-			if setTmpcopyup {
-				return newMount, fmt.Errorf("cannot pass 'tmpcopyup' and 'notmpcopyup' options more than once: %w", errOptionArg)
-			}
-			setTmpcopyup = true
-			newMount.Options = append(newMount.Options, kv[0])
-		case "ro", "rw":
-			if setRORW {
-				return newMount, fmt.Errorf("cannot pass 'ro' and 'rw' options more than once: %w", errOptionArg)
-			}
-			setRORW = true
-			newMount.Options = append(newMount.Options, kv[0])
-		case "nosuid", "suid":
-			if setSuid {
-				return newMount, fmt.Errorf("cannot pass 'nosuid' and 'suid' options more than once: %w", errOptionArg)
-			}
-			setSuid = true
-			newMount.Options = append(newMount.Options, kv[0])
-		case "nodev", "dev":
-			if setDev {
-				return newMount, fmt.Errorf("cannot pass 'nodev' and 'dev' options more than once: %w", errOptionArg)
-			}
-			setDev = true
-			newMount.Options = append(newMount.Options, kv[0])
-		case "noexec", "exec":
-			if setExec {
-				return newMount, fmt.Errorf("cannot pass 'noexec' and 'exec' options more than once: %w", errOptionArg)
-			}
-			setExec = true
-			newMount.Options = append(newMount.Options, kv[0])
-		case "tmpfs-mode":
-			if len(kv) == 1 {
-				return newMount, fmt.Errorf("%v: %w", kv[0], errOptionArg)
-			}
-			newMount.Options = append(newMount.Options, fmt.Sprintf("mode=%s", kv[1]))
-		case "tmpfs-size":
-			if len(kv) == 1 {
-				return newMount, fmt.Errorf("%v: %w", kv[0], errOptionArg)
-			}
-			newMount.Options = append(newMount.Options, fmt.Sprintf("size=%s", kv[1]))
-		case "src", "source":
-			return newMount, fmt.Errorf("source is not supported with tmpfs mounts")
-		case "target", "dst", "destination":
-			if len(kv) == 1 {
-				return newMount, fmt.Errorf("%v: %w", kv[0], errOptionArg)
-			}
-			if err := parse.ValidateVolumeCtrDir(kv[1]); err != nil {
-				return newMount, err
-			}
-			newMount.Destination = unixPathClean(kv[1])
-			setDest = true
-		case "U", "chown":
-			if setOwnership {
-				return newMount, fmt.Errorf("cannot pass 'U' or 'chown' option more than once: %w", errOptionArg)
-			}
-			ok, err := validChownFlag(val)
-			if err != nil {
-				return newMount, err
-			}
-			if ok {
-				newMount.Options = append(newMount.Options, "U")
-			}
-			setOwnership = true
-		case "consistency":
-			// Often used on MACs and mistakenly on Linux platforms.
-			// Since Docker ignores this option so shall we.
-			continue
-		default:
-			return newMount, fmt.Errorf("%s: %w", kv[0], util.ErrBadMntOption)
-		}
+	var err error
+	mnt, err := parseMountOptions(newMount.Type, args)
+	if err != nil {
+		return newMount, err
 	}
-
-	if !setDest {
+	if len(mnt.Destination) == 0 {
 		return newMount, errNoDest
 	}
-
+	newMount.Destination = mnt.Destination
+	newMount.Options = mnt.Options
 	return newMount, nil
 }
 
@@ -517,84 +504,16 @@ func getDevptsMount(args []string) (spec.Mount, error) {
 func getNamedVolume(args []string) (*specgen.NamedVolume, error) {
 	newVolume := new(specgen.NamedVolume)
 
-	var setDest, setRORW, setSuid, setDev, setExec, setOwnership bool
-
-	for _, val := range args {
-		kv := strings.SplitN(val, "=", 2)
-		switch kv[0] {
-		case "volume-opt":
-			newVolume.Options = append(newVolume.Options, val)
-		case "ro", "rw":
-			if setRORW {
-				return nil, fmt.Errorf("cannot pass 'ro' and 'rw' options more than once: %w", errOptionArg)
-			}
-			setRORW = true
-			newVolume.Options = append(newVolume.Options, kv[0])
-		case "nosuid", "suid":
-			if setSuid {
-				return nil, fmt.Errorf("cannot pass 'nosuid' and 'suid' options more than once: %w", errOptionArg)
-			}
-			setSuid = true
-			newVolume.Options = append(newVolume.Options, kv[0])
-		case "nodev", "dev":
-			if setDev {
-				return nil, fmt.Errorf("cannot pass 'nodev' and 'dev' options more than once: %w", errOptionArg)
-			}
-			setDev = true
-			newVolume.Options = append(newVolume.Options, kv[0])
-		case "noexec", "exec":
-			if setExec {
-				return nil, fmt.Errorf("cannot pass 'noexec' and 'exec' options more than once: %w", errOptionArg)
-			}
-			setExec = true
-			newVolume.Options = append(newVolume.Options, kv[0])
-		case "volume-label":
-			return nil, fmt.Errorf("the --volume-label option is not presently implemented")
-		case "src", "source":
-			if len(kv) == 1 {
-				return nil, fmt.Errorf("%v: %w", kv[0], errOptionArg)
-			}
-			newVolume.Name = kv[1]
-		case "target", "dst", "destination":
-			if len(kv) == 1 {
-				return nil, fmt.Errorf("%v: %w", kv[0], errOptionArg)
-			}
-			if err := parse.ValidateVolumeCtrDir(kv[1]); err != nil {
-				return nil, err
-			}
-			newVolume.Dest = unixPathClean(kv[1])
-			setDest = true
-		case "idmap":
-			if len(kv) > 1 {
-				newVolume.Options = append(newVolume.Options, fmt.Sprintf("idmap=%s", kv[1]))
-			} else {
-				newVolume.Options = append(newVolume.Options, "idmap")
-			}
-		case "U", "chown":
-			if setOwnership {
-				return newVolume, fmt.Errorf("cannot pass 'U' or 'chown' option more than once: %w", errOptionArg)
-			}
-			ok, err := validChownFlag(val)
-			if err != nil {
-				return newVolume, err
-			}
-			if ok {
-				newVolume.Options = append(newVolume.Options, "U")
-			}
-			setOwnership = true
-		case "consistency":
-			// Often used on MACs and mistakenly on Linux platforms.
-			// Since Docker ignores this option so shall we.
-			continue
-		default:
-			return nil, fmt.Errorf("%s: %w", kv[0], util.ErrBadMntOption)
-		}
+	mnt, err := parseMountOptions(define.TypeVolume, args)
+	if err != nil {
+		return nil, err
 	}
-
-	if !setDest {
+	if len(mnt.Destination) == 0 {
 		return nil, errNoDest
 	}
-
+	newVolume.Options = mnt.Options
+	newVolume.Name = mnt.Source
+	newVolume.Dest = mnt.Destination
 	return newVolume, nil
 }
 
diff --git a/test/system/060-mount.bats b/test/system/060-mount.bats
index dd8aa3e97b..30fe10e410 100644
--- a/test/system/060-mount.bats
+++ b/test/system/060-mount.bats
@@ -84,6 +84,22 @@ load helpers
     is "$output" "" "podman image mount, no args, after umount"
 }
 
+@test "podman run --mount ro=false " {
+    local volpath=/path/in/container
+    local stdopts="type=volume,destination=$volpath"
+
+    # Variations on a theme (not by Paganini). All of these should fail.
+    for varopt in readonly readonly=true ro=true ro rw=false;do
+        run_podman 1 run --rm -q --mount $stdopts,$varopt $IMAGE touch $volpath/a
+        is "$output" "touch: $volpath/a: Read-only file system" "with $varopt"
+    done
+
+    # All of these should pass
+    for varopt in rw rw=true ro=false readonly=false;do
+        run_podman run --rm -q --mount $stdopts,$varopt $IMAGE touch $volpath/a
+    done
+}
+
 @test "podman run --mount image" {
     skip_if_rootless "too hard to test rootless"