Merge pull request #9054 from vrothberg/fix-9040

make sure the workdir exists on container mount

libpod/container_api.go

@@ -776,3 +776,32 @@ func (c *Container) ShouldRestart(ctx context.Context) bool {
 	}
 	return c.shouldRestart()
 }
+
+// ResolvePath resolves the specified path on the root for the container.  The
+// root must either be the mounted image of the container or the already
+// mounted container storage.
+//
+// It returns the resolved root and the resolved path.  Note that the path may
+// resolve to the container's mount point or to a volume or bind mount.
+func (c *Container) ResolvePath(ctx context.Context, root string, path string) (string, string, error) {
+	logrus.Debugf("Resolving path %q (root %q) on container %s", path, root, c.ID())
+
+	// Minimal sanity checks.
+	if len(root)*len(path) == 0 {
+		return "", "", errors.Wrapf(define.ErrInternal, "ResolvePath: root (%q) and path (%q) must be non empty", root, path)
+	}
+	if _, err := os.Stat(root); err != nil {
+		return "", "", errors.Wrapf(err, "cannot locate root to resolve path on container %s", c.ID())
+	}
+
+	if !c.batched {
+		c.lock.Lock()
+		defer c.lock.Unlock()
+
+		if err := c.syncContainer(); err != nil {
+			return "", "", err
+		}
+	}
+
+	return c.resolvePath(root, path)
+}

libpod/container_internal_linux.go

@@ -174,25 +174,60 @@ func (c *Container) prepare() error {
 		return err
 	}
 
-	// Ensure container entrypoint is created (if required)
+	// Make sure the workdir exists
-	if c.config.CreateWorkingDir {
+	if err := c.resolveWorkDir(); err != nil {
-		workdir, err := securejoin.SecureJoin(c.state.Mountpoint, c.WorkingDir())
+		return err
-		if err != nil {
+	}
-			return errors.Wrapf(err, "error creating path to container %s working dir", c.ID())
-		}
-		rootUID := c.RootUID()
-		rootGID := c.RootGID()
 
-		if err := os.MkdirAll(workdir, 0755); err != nil {
+	return nil
-			if os.IsExist(err) {
+}
-				return nil
+
+// resolveWorkDir resolves the container's workdir and, depending on the
+// configuration, will create it, or error out if it does not exist.
+// Note that the container must be mounted before.
+func (c *Container) resolveWorkDir() error {
+	workdir := c.WorkingDir()
+
+	// If the specified workdir is a subdir of a volume or mount,
+	// we don't need to do anything.  The runtime is taking care of
+	// that.
+	if isPathOnVolume(c, workdir) || isPathOnBindMount(c, workdir) {
+		logrus.Debugf("Workdir %q resolved to a volume or mount", workdir)
+		return nil
+	}
+
+	_, resolvedWorkdir, err := c.resolvePath(c.state.Mountpoint, workdir)
+	if err != nil {
+		return err
+	}
+	logrus.Debugf("Workdir %q resolved to host path %q", workdir, resolvedWorkdir)
+
+	// No need to create it (e.g., `--workdir=/foo`), so let's make sure
+	// the path exists on the container.
+	if !c.config.CreateWorkingDir {
+		if _, err := os.Stat(resolvedWorkdir); err != nil {
+			if os.IsNotExist(err) {
+				return errors.Errorf("workdir %q does not exist on container %s", workdir, c.ID())
 			}
-			return errors.Wrapf(err, "error creating container %s working dir", c.ID())
+			// This might be a serious error (e.g., permission), so
+			// we need to return the full error.
+			return errors.Wrapf(err, "error detecting workdir %q on container %s", workdir, c.ID())
 		}
+	}
 
-		if err := os.Chown(workdir, rootUID, rootGID); err != nil {
+	// Ensure container entrypoint is created (if required).
-			return errors.Wrapf(err, "error chowning container %s working directory to container root", c.ID())
+	rootUID := c.RootUID()
+	rootGID := c.RootGID()
+
+	if err := os.MkdirAll(resolvedWorkdir, 0755); err != nil {
+		if os.IsExist(err) {
+			return nil
 		}
+		return errors.Wrapf(err, "error creating container %s workdir", c.ID())
+	}
+
+	if err := os.Chown(resolvedWorkdir, rootUID, rootGID); err != nil {
+		return errors.Wrapf(err, "error chowning container %s workdir to container root", c.ID())
 	}
 
 	return nil

libpod/container_path_resolution.go

@@ -0,0 +1,166 @@
+package libpod
+
+import (
+	"path/filepath"
+	"strings"
+
+	securejoin "github.com/cyphar/filepath-securejoin"
+	"github.com/opencontainers/runtime-spec/specs-go"
+	"github.com/pkg/errors"
+	"github.com/sirupsen/logrus"
+)
+
+// resolveContainerPaths resolves the container's mount point and the container
+// path as specified by the user.  Both may resolve to paths outside of the
+// container's mount point when the container path hits a volume or bind mount.
+//
+// It returns a bool, indicating whether containerPath resolves outside of
+// mountPoint (e.g., via a mount or volume), the resolved root (e.g., container
+// mount, bind mount or volume) and the resolved path on the root (absolute to
+// the host).
+func (container *Container) resolvePath(mountPoint string, containerPath string) (string, string, error) {
+	// Let's first make sure we have a path relative to the mount point.
+	pathRelativeToContainerMountPoint := containerPath
+	if !filepath.IsAbs(containerPath) {
+		// If the containerPath is not absolute, it's relative to the
+		// container's working dir.  To be extra careful, let's first
+		// join the working dir with "/", and the add the containerPath
+		// to it.
+		pathRelativeToContainerMountPoint = filepath.Join(filepath.Join("/", container.WorkingDir()), containerPath)
+	}
+	resolvedPathOnTheContainerMountPoint := filepath.Join(mountPoint, pathRelativeToContainerMountPoint)
+	pathRelativeToContainerMountPoint = strings.TrimPrefix(pathRelativeToContainerMountPoint, mountPoint)
+	pathRelativeToContainerMountPoint = filepath.Join("/", pathRelativeToContainerMountPoint)
+
+	// Now we have an "absolute container Path" but not yet resolved on the
+	// host (e.g., "/foo/bar/file.txt").  As mentioned above, we need to
+	// check if "/foo/bar/file.txt" is on a volume or bind mount.  To do
+	// that, we need to walk *down* the paths to the root.  Assuming
+	// volume-1 is mounted to "/foo" and volume-2 is mounted to "/foo/bar",
+	// we must select "/foo/bar".  Once selected, we need to rebase the
+	// remainder (i.e, "/file.txt") on the volume's mount point on the
+	// host.  Same applies to bind mounts.
+
+	searchPath := pathRelativeToContainerMountPoint
+	for {
+		volume, err := findVolume(container, searchPath)
+		if err != nil {
+			return "", "", err
+		}
+		if volume != nil {
+			logrus.Debugf("Container path %q resolved to volume %q on path %q", containerPath, volume.Name(), searchPath)
+
+			// TODO: We really need to force the volume to mount
+			// before doing this, but that API is not exposed
+			// externally right now and doing so is beyond the scope
+			// of this commit.
+			mountPoint, err := volume.MountPoint()
+			if err != nil {
+				return "", "", err
+			}
+			if mountPoint == "" {
+				return "", "", errors.Errorf("volume %s is not mounted, cannot copy into it", volume.Name())
+			}
+
+			// We found a matching volume for searchPath.  We now
+			// need to first find the relative path of our input
+			// path to the searchPath, and then join it with the
+			// volume's mount point.
+			pathRelativeToVolume := strings.TrimPrefix(pathRelativeToContainerMountPoint, searchPath)
+			absolutePathOnTheVolumeMount, err := securejoin.SecureJoin(mountPoint, pathRelativeToVolume)
+			if err != nil {
+				return "", "", err
+			}
+			return mountPoint, absolutePathOnTheVolumeMount, nil
+		}
+
+		if mount := findBindMount(container, searchPath); mount != nil {
+			logrus.Debugf("Container path %q resolved to bind mount %q:%q on path %q", containerPath, mount.Source, mount.Destination, searchPath)
+			// We found a matching bind mount for searchPath.  We
+			// now need to first find the relative path of our
+			// input path to the searchPath, and then join it with
+			// the source of the bind mount.
+			pathRelativeToBindMount := strings.TrimPrefix(pathRelativeToContainerMountPoint, searchPath)
+			absolutePathOnTheBindMount, err := securejoin.SecureJoin(mount.Source, pathRelativeToBindMount)
+			if err != nil {
+				return "", "", err
+			}
+			return mount.Source, absolutePathOnTheBindMount, nil
+
+		}
+
+		if searchPath == "/" {
+			// Cannot go beyond "/", so we're done.
+			break
+		}
+
+		// Walk *down* the path (e.g., "/foo/bar/x" -> "/foo/bar").
+		searchPath = filepath.Dir(searchPath)
+	}
+
+	// No volume, no bind mount but just a normal path on the container.
+	return mountPoint, resolvedPathOnTheContainerMountPoint, nil
+}
+
+// findVolume checks if the specified containerPath matches the destination
+// path of a Volume.  Returns a matching Volume or nil.
+func findVolume(c *Container, containerPath string) (*Volume, error) {
+	runtime := c.Runtime()
+	cleanedContainerPath := filepath.Clean(containerPath)
+	for _, vol := range c.Config().NamedVolumes {
+		if cleanedContainerPath == filepath.Clean(vol.Dest) {
+			return runtime.GetVolume(vol.Name)
+		}
+	}
+	return nil, nil
+}
+
+// isPathOnVolume returns true if the specified containerPath is a subdir of any
+// Volume's destination.
+func isPathOnVolume(c *Container, containerPath string) bool {
+	cleanedContainerPath := filepath.Clean(containerPath)
+	for _, vol := range c.Config().NamedVolumes {
+		if cleanedContainerPath == filepath.Clean(vol.Dest) {
+			return true
+		}
+		for dest := vol.Dest; dest != "/"; dest = filepath.Dir(dest) {
+			if cleanedContainerPath == dest {
+				return true
+			}
+		}
+	}
+	return false
+}
+
+// findBindMounts checks if the specified containerPath matches the destination
+// path of a Mount.  Returns a matching Mount or nil.
+func findBindMount(c *Container, containerPath string) *specs.Mount {
+	cleanedPath := filepath.Clean(containerPath)
+	for _, m := range c.Config().Spec.Mounts {
+		if m.Type != "bind" {
+			continue
+		}
+		if cleanedPath == filepath.Clean(m.Destination) {
+			mount := m
+			return &mount
+		}
+	}
+	return nil
+}
+
+/// isPathOnBindMount returns true if the specified containerPath is a subdir of any
+// Mount's destination.
+func isPathOnBindMount(c *Container, containerPath string) bool {
+	cleanedContainerPath := filepath.Clean(containerPath)
+	for _, m := range c.Config().Spec.Mounts {
+		if cleanedContainerPath == filepath.Clean(m.Destination) {
+			return true
+		}
+		for dest := m.Destination; dest != "/"; dest = filepath.Dir(dest) {
+			if cleanedContainerPath == dest {
+				return true
+			}
+		}
+	}
+	return false
+}

pkg/domain/infra/abi/archive.go

@@ -26,13 +26,18 @@ func (ic *ContainerEngine) ContainerCopyFromArchive(ctx context.Context, nameOrI
 		return nil, err
 	}
 
+	containerMountPoint, err := container.Mount()
+	if err != nil {
+		return nil, err
+	}
+
 	unmount := func() {
 		if err := container.Unmount(false); err != nil {
 			logrus.Errorf("Error unmounting container: %v", err)
 		}
 	}
 
-	_, resolvedRoot, resolvedContainerPath, err := ic.containerStat(container, containerPath)
+	_, resolvedRoot, resolvedContainerPath, err := ic.containerStat(container, containerMountPoint, containerPath)
 	if err != nil {
 		unmount()
 		return nil, err
@@ -71,6 +76,11 @@ func (ic *ContainerEngine) ContainerCopyToArchive(ctx context.Context, nameOrID
 		return nil, err
 	}
 
+	containerMountPoint, err := container.Mount()
+	if err != nil {
+		return nil, err
+	}
+
 	unmount := func() {
 		if err := container.Unmount(false); err != nil {
 			logrus.Errorf("Error unmounting container: %v", err)
@@ -83,7 +93,7 @@ func (ic *ContainerEngine) ContainerCopyToArchive(ctx context.Context, nameOrID
 		containerPath = "/."
 	}
 
-	_, resolvedRoot, resolvedContainerPath, err := ic.containerStat(container, containerPath)
+	_, resolvedRoot, resolvedContainerPath, err := ic.containerStat(container, containerMountPoint, containerPath)
 	if err != nil {
 		unmount()
 		return nil, err

pkg/domain/infra/abi/containers_stat.go

@@ -10,18 +10,11 @@ import (
 	"github.com/containers/podman/v2/libpod"
 	"github.com/containers/podman/v2/pkg/copy"
 	"github.com/containers/podman/v2/pkg/domain/entities"
-	securejoin "github.com/cyphar/filepath-securejoin"
-	"github.com/opencontainers/runtime-spec/specs-go"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
 )
 
-func (ic *ContainerEngine) containerStat(container *libpod.Container, containerPath string) (*entities.ContainerStatReport, string, string, error) {
+func (ic *ContainerEngine) containerStat(container *libpod.Container, containerMountPoint string, containerPath string) (*entities.ContainerStatReport, string, string, error) {
-	containerMountPoint, err := container.Mount()
-	if err != nil {
-		return nil, "", "", err
-	}
-
 	// Make sure that "/" copies the *contents* of the mount point and not
 	// the directory.
 	if containerPath == "/" {
@@ -30,7 +23,7 @@ func (ic *ContainerEngine) containerStat(container *libpod.Container, containerP
 
 	// Now resolve the container's path.  It may hit a volume, it may hit a
 	// bind mount, it may be relative.
-	resolvedRoot, resolvedContainerPath, err := resolveContainerPaths(container, containerMountPoint, containerPath)
+	resolvedRoot, resolvedContainerPath, err := container.ResolvePath(context.Background(), containerMountPoint, containerPath)
 	if err != nil {
 		return nil, "", "", err
 	}
@@ -94,138 +87,21 @@ func (ic *ContainerEngine) ContainerStat(ctx context.Context, nameOrID string, c
 		return nil, err
 	}
 
+	containerMountPoint, err := container.Mount()
+	if err != nil {
+		return nil, err
+	}
+
 	defer func() {
 		if err := container.Unmount(false); err != nil {
 			logrus.Errorf("Error unmounting container: %v", err)
 		}
 	}()
 
-	statReport, _, _, err := ic.containerStat(container, containerPath)
+	statReport, _, _, err := ic.containerStat(container, containerMountPoint, containerPath)
 	return statReport, err
 }
 
-// resolveContainerPaths resolves the container's mount point and the container
-// path as specified by the user.  Both may resolve to paths outside of the
-// container's mount point when the container path hits a volume or bind mount.
-//
-// NOTE: We must take volumes and bind mounts into account as, regrettably, we
-// can copy to/from stopped containers.  In that case, the volumes and bind
-// mounts are not present.  For running containers, the runtime (e.g., runc or
-// crun) takes care of these mounts.  For stopped ones, we need to do quite
-// some dance, as done below.
-func resolveContainerPaths(container *libpod.Container, mountPoint string, containerPath string) (string, string, error) {
-	// Let's first make sure we have a path relative to the mount point.
-	pathRelativeToContainerMountPoint := containerPath
-	if !filepath.IsAbs(containerPath) {
-		// If the containerPath is not absolute, it's relative to the
-		// container's working dir.  To be extra careful, let's first
-		// join the working dir with "/", and the add the containerPath
-		// to it.
-		pathRelativeToContainerMountPoint = filepath.Join(filepath.Join("/", container.WorkingDir()), containerPath)
-	}
-	resolvedPathOnTheContainerMountPoint := filepath.Join(mountPoint, pathRelativeToContainerMountPoint)
-	pathRelativeToContainerMountPoint = strings.TrimPrefix(pathRelativeToContainerMountPoint, mountPoint)
-	pathRelativeToContainerMountPoint = filepath.Join("/", pathRelativeToContainerMountPoint)
-
-	// Now we have an "absolute container Path" but not yet resolved on the
-	// host (e.g., "/foo/bar/file.txt").  As mentioned above, we need to
-	// check if "/foo/bar/file.txt" is on a volume or bind mount.  To do
-	// that, we need to walk *down* the paths to the root.  Assuming
-	// volume-1 is mounted to "/foo" and volume-2 is mounted to "/foo/bar",
-	// we must select "/foo/bar".  Once selected, we need to rebase the
-	// remainder (i.e, "/file.txt") on the volume's mount point on the
-	// host.  Same applies to bind mounts.
-
-	searchPath := pathRelativeToContainerMountPoint
-	for {
-		volume, err := findVolume(container, searchPath)
-		if err != nil {
-			return "", "", err
-		}
-		if volume != nil {
-			logrus.Debugf("Container path %q resolved to volume %q on path %q", containerPath, volume.Name(), searchPath)
-
-			// TODO: We really need to force the volume to mount
-			// before doing this, but that API is not exposed
-			// externally right now and doing so is beyond the scope
-			// of this commit.
-			mountPoint, err := volume.MountPoint()
-			if err != nil {
-				return "", "", err
-			}
-			if mountPoint == "" {
-				return "", "", errors.Errorf("volume %s is not mounted, cannot copy into it", volume.Name())
-			}
-
-			// We found a matching volume for searchPath.  We now
-			// need to first find the relative path of our input
-			// path to the searchPath, and then join it with the
-			// volume's mount point.
-			pathRelativeToVolume := strings.TrimPrefix(pathRelativeToContainerMountPoint, searchPath)
-			absolutePathOnTheVolumeMount, err := securejoin.SecureJoin(mountPoint, pathRelativeToVolume)
-			if err != nil {
-				return "", "", err
-			}
-			return mountPoint, absolutePathOnTheVolumeMount, nil
-		}
-
-		if mount := findBindMount(container, searchPath); mount != nil {
-			logrus.Debugf("Container path %q resolved to bind mount %q:%q on path %q", containerPath, mount.Source, mount.Destination, searchPath)
-			// We found a matching bind mount for searchPath.  We
-			// now need to first find the relative path of our
-			// input path to the searchPath, and then join it with
-			// the source of the bind mount.
-			pathRelativeToBindMount := strings.TrimPrefix(pathRelativeToContainerMountPoint, searchPath)
-			absolutePathOnTheBindMount, err := securejoin.SecureJoin(mount.Source, pathRelativeToBindMount)
-			if err != nil {
-				return "", "", err
-			}
-			return mount.Source, absolutePathOnTheBindMount, nil
-
-		}
-
-		if searchPath == "/" {
-			// Cannot go beyond "/", so we're done.
-			break
-		}
-
-		// Walk *down* the path (e.g., "/foo/bar/x" -> "/foo/bar").
-		searchPath = filepath.Dir(searchPath)
-	}
-
-	// No volume, no bind mount but just a normal path on the container.
-	return mountPoint, resolvedPathOnTheContainerMountPoint, nil
-}
-
-// findVolume checks if the specified container path matches a volume inside
-// the container.  It returns a matching volume or nil.
-func findVolume(c *libpod.Container, containerPath string) (*libpod.Volume, error) {
-	runtime := c.Runtime()
-	cleanedContainerPath := filepath.Clean(containerPath)
-	for _, vol := range c.Config().NamedVolumes {
-		if cleanedContainerPath == filepath.Clean(vol.Dest) {
-			return runtime.GetVolume(vol.Name)
-		}
-	}
-	return nil, nil
-}
-
-// findBindMount checks if the specified container path matches a bind mount
-// inside the container.  It returns a matching mount or nil.
-func findBindMount(c *libpod.Container, containerPath string) *specs.Mount {
-	cleanedPath := filepath.Clean(containerPath)
-	for _, m := range c.Config().Spec.Mounts {
-		if m.Type != "bind" {
-			continue
-		}
-		if cleanedPath == filepath.Clean(m.Destination) {
-			mount := m
-			return &mount
-		}
-	}
-	return nil
-}
-
 // secureStat extracts file info for path in a chroot'ed environment in root.
 func secureStat(root string, path string) (*buildahCopiah.StatForItem, error) {
 	var glob string

pkg/specgen/generate/container.go

@@ -203,20 +203,6 @@ func CompleteSpec(ctx context.Context, r *libpod.Runtime, s *specgen.SpecGenerat
 	}
 	s.Annotations = annotations
 
-	// workdir
-	if s.WorkDir == "" {
-		if newImage != nil {
-			workingDir, err := newImage.WorkingDir(ctx)
-			if err != nil {
-				return nil, err
-			}
-			s.WorkDir = workingDir
-		}
-	}
-	if s.WorkDir == "" {
-		s.WorkDir = "/"
-	}
-
 	if len(s.SeccompProfilePath) < 1 {
 		p, err := libpod.DefaultSeccompPath()
 		if err != nil {

pkg/specgen/generate/container_create.go

@@ -272,10 +272,18 @@ func createContainerOptions(ctx context.Context, rt *libpod.Runtime, s *specgen.
 	if s.Entrypoint != nil {
 		options = append(options, libpod.WithEntrypoint(s.Entrypoint))
 	}
-	// If the user did not set an workdir but the image did, ensure it is
+	// If the user did not specify a workdir on the CLI, let's extract it
-	// created.
+	// from the image.
 	if s.WorkDir == "" && img != nil {
 		options = append(options, libpod.WithCreateWorkingDir())
+		wd, err := img.WorkingDir(ctx)
+		if err != nil {
+			return nil, err
+		}
+		s.WorkDir = wd
+	}
+	if s.WorkDir == "" {
+		s.WorkDir = "/"
 	}
 	if s.StopSignal != nil {
 		options = append(options, libpod.WithStopSignal(*s.StopSignal))

test/e2e/run_working_dir_test.go

@@ -2,7 +2,6 @@ package integration
 
 import (
 	"os"
-	"strings"
 
 	. "github.com/containers/podman/v2/test/utils"
 	. "github.com/onsi/ginkgo"
@@ -41,12 +40,9 @@ var _ = Describe("Podman run", func() {
 	})
 
 	It("podman run a container using non existing --workdir", func() {
-		if !strings.Contains(podmanTest.OCIRuntime, "crun") {
-			Skip("Test only works on crun")
-		}
 		session := podmanTest.Podman([]string{"run", "--workdir", "/home/foobar", ALPINE, "pwd"})
 		session.WaitWithDefaultTimeout()
-		Expect(session.ExitCode()).To(Equal(127))
+		Expect(session.ExitCode()).To(Equal(126))
 	})
 
 	It("podman run a container on an image with a workdir", func() {

test/system/030-run.bats

@@ -589,4 +589,25 @@ json-file | f
     is "${lines[1]}" "$rand" "Container runs successfully despite warning"
 }
 
+@test "podman run - check workdir" {
+    # Workdirs specified via the CLI are not created on the root FS.
+    run_podman 126 run --rm --workdir /i/do/not/exist $IMAGE pwd
+    # Note: remote error prepends an attach error.
+    is "$output" "Error: .*workdir \"/i/do/not/exist\" does not exist on container.*"
+
+    testdir=$PODMAN_TMPDIR/volume
+    mkdir -p $testdir
+    randomcontent=$(random_string 10)
+    echo "$randomcontent" > $testdir/content
+
+    # Workdir does not exist on the image but is volume mounted.
+    run_podman run --rm --workdir /IamNotOnTheImage -v $testdir:/IamNotOnTheImage $IMAGE cat content
+    is "$output" "$randomcontent" "cat random content"
+
+    # Workdir does not exist on the image but is created by the runtime as it's
+    # a subdir of a volume.
+    run_podman run --rm --workdir /IamNotOntheImage -v $testdir/content:/IamNotOntheImage/foo $IMAGE cat foo
+    is "$output" "$randomcontent" "cat random content"
+}
+
 # vim: filetype=sh

test/system/070-build.bats

@@ -163,10 +163,12 @@ EOF
 https_proxy=https-proxy-in-env-file
 EOF
 
+    # NOTE: it's important to not create the workdir.
+    # Podman will make sure to create a missing workdir
+    # if needed. See #9040.
     cat >$tmpdir/Containerfile <<EOF
 FROM $IMAGE
 LABEL $label_name=$label_value
-RUN mkdir $workdir
 WORKDIR $workdir
 
 # Test for #7094 - chowning of invalid symlinks