Mount volumes before copying into a container

This solves several problems with copying into volumes on a
container that is not running.

The first, and most obvious, is that we were previously entirely
unable to copy into a volume that required mounting - like
image volumes, volume plugins, and volumes that specified mount
options.

The second is that this fixed several permissions and content
issues with a fresh volume and a container that has not been run
before. A copy-up will not have occurred, so permissions on the
volume root will not have been set and content will not have been
copied into the volume.

If the container is running, this is very low cost - we maintain
a mount counter for named volumes, so it's just an increment in
the DB if the volume actually needs mounting, and a no-op if it
doesn't.

Unfortunately, we also have to fix permissions, and that is
rather more complicated. This involves an ugly set of manual
edits to the volume state to ensure that the permissions fixes
actually worked, as the code was never meant to be used in this
way. It's really ugly, but necessary to reach full Docker
compatibility.

Fixes #24405

Signed-off-by: Matthew Heon <matthew.heon@pm.me>

libpod/container_copy_common.go

@@ -4,7 +4,9 @@ package libpod
 
 import (
 	"errors"
+	"fmt"
 	"io"
+	"os"
 	"path/filepath"
 	"strings"
 
@@ -12,9 +14,11 @@ import (
 	"github.com/containers/buildah/pkg/chrootuser"
 	"github.com/containers/buildah/util"
 	"github.com/containers/podman/v5/libpod/define"
+	"github.com/containers/podman/v5/libpod/shutdown"
 	"github.com/containers/podman/v5/pkg/rootless"
 	"github.com/containers/storage/pkg/archive"
 	"github.com/containers/storage/pkg/idtools"
+	"github.com/containers/storage/pkg/stringid"
 	"github.com/opencontainers/runtime-spec/specs-go"
 	"github.com/sirupsen/logrus"
 )
@@ -25,7 +29,9 @@ func (c *Container) copyFromArchive(path string, chown, noOverwriteDirNonDir boo
 		resolvedRoot string
 		resolvedPath string
 		unmount      func()
+		cleanupFuncs []func()
 		err          error
+		locked       bool = true
 	)
 
 	// Make sure that "/" copies the *contents* of the mount point and not
@@ -44,19 +50,146 @@ func (c *Container) copyFromArchive(path string, chown, noOverwriteDirNonDir boo
 		if err != nil {
 			return nil, err
 		}
+		c.state.Mountpoint = mountPoint
+		if err := c.save(); err != nil {
+			return nil, err
+		}
+
 		unmount = func() {
+			if !locked {
+				c.lock.Lock()
+				defer c.lock.Unlock()
+			}
+
+			if err := c.syncContainer(); err != nil {
+				logrus.Errorf("Unable to sync container %s state: %v", c.ID(), err)
+				return
+			}
+
+			// These have to be first, some of them rely on container rootfs still being mounted.
+			for _, cleanupFunc := range cleanupFuncs {
+				cleanupFunc()
+			}
 			if err := c.unmount(false); err != nil {
 				logrus.Errorf("Failed to unmount container: %v", err)
 			}
+
+			if c.ensureState(define.ContainerStateConfigured, define.ContainerStateExited) {
+				c.state.Mountpoint = ""
+				if err := c.save(); err != nil {
+					logrus.Errorf("Writing container %s state: %v", c.ID(), err)
+				}
+			}
+		}
+
+		// Before we proceed, mount all named volumes associated with the
+		// container.
+		// This solves two issues:
+		// Firstly, it ensures that if the volume actually requires a mount, we
+		// will mount it for safe use.
+		// (For example, image volumes, volume plugins).
+		// Secondly, it copies up into the volume if necessary.
+		// This ensures that permissions are correct for copies into volumes on
+		// containers that have never started.
+		if len(c.config.NamedVolumes) > 0 {
+			for _, v := range c.config.NamedVolumes {
+				vol, err := c.mountNamedVolume(v, mountPoint)
+				if err != nil {
+					unmount()
+					return nil, err
+				}
+
+				volUnmountName := fmt.Sprintf("volume unmount %s %s", vol.Name(), stringid.GenerateNonCryptoID()[0:12])
+
+				// The unmount function can be called in two places:
+				// First, from unmount(), our generic cleanup function that gets
+				// called on success or on failure by error.
+				// Second, from the shutdown handler on receipt of a SIGTERM
+				// or similar.
+				volUnmountFunc := func() error {
+					vol.lock.Lock()
+					defer vol.lock.Unlock()
+
+					if err := vol.unmount(false); err != nil {
+						return err
+					}
+
+					return nil
+				}
+
+				cleanupFuncs = append(cleanupFuncs, func() {
+					_ = shutdown.Unregister(volUnmountName)
+
+					if err := volUnmountFunc(); err != nil {
+						logrus.Errorf("Unmounting container %s volume %s: %v", c.ID(), vol.Name(), err)
+					}
+				})
+
+				if err := shutdown.Register(volUnmountName, func(_ os.Signal) error {
+					return volUnmountFunc()
+				}); err != nil && !errors.Is(err, shutdown.ErrHandlerExists) {
+					return nil, fmt.Errorf("adding shutdown handler for volume %s unmount: %w", vol.Name(), err)
+				}
+			}
 		}
 	}
 
-	resolvedRoot, resolvedPath, err = c.resolveCopyTarget(mountPoint, path)
+	resolvedRoot, resolvedPath, volume, err := c.resolveCopyTarget(mountPoint, path)
 	if err != nil {
 		unmount()
 		return nil, err
 	}
 
+	if volume != nil {
+		// This must be the first cleanup function so it fires before volume unmounts happen.
+		cleanupFuncs = append([]func(){func() {
+			// This is a gross hack to ensure correct permissions
+			// on a volume that was copied into that needed, but did
+			// not receive, a copy-up.
+			// Why do we need this?
+			// Basically: fixVolumePermissions is needed to ensure
+			// the volume has the right permissions.
+			// However, fixVolumePermissions only fires on a volume
+			// that is not empty iff a copy-up occurred.
+			// In this case, the volume is not empty as we just
+			// copied into it, so in order to get
+			// fixVolumePermissions to actually run, we must
+			// convince it that a copy-up occurred - even if it did
+			// not.
+			// At the same time, clear NeedsCopyUp as we just
+			// populated the volume and that will block a future
+			// copy-up.
+			volume.lock.Lock()
+
+			if err := volume.update(); err != nil {
+				logrus.Errorf("Unable to update volume %s status: %v", volume.Name(), err)
+				volume.lock.Unlock()
+				return
+			}
+
+			if volume.state.NeedsCopyUp && volume.state.NeedsChown {
+				volume.state.NeedsCopyUp = false
+				volume.state.CopiedUp = true
+				if err := volume.save(); err != nil {
+					logrus.Errorf("Unable to save volume %s state: %v", volume.Name(), err)
+					volume.lock.Unlock()
+					return
+				}
+
+				volume.lock.Unlock()
+
+				for _, namedVol := range c.config.NamedVolumes {
+					if namedVol.Name == volume.Name() {
+						if err := c.fixVolumePermissions(namedVol); err != nil {
+							logrus.Errorf("Unable to fix volume %s permissions: %v", volume.Name(), err)
+						}
+						return
+					}
+				}
+			}
+		}}, cleanupFuncs...)
+	}
+
 	var idPair *idtools.IDPair
 	if chown {
 		// Make sure we chown the files to the container's main user and group ID.
@@ -74,6 +207,8 @@ func (c *Container) copyFromArchive(path string, chown, noOverwriteDirNonDir boo
 		return nil, err
 	}
 
+	locked = false
+
 	logrus.Debugf("Container copy *to* %q (resolved: %q) on container %q (ID: %s)", path, resolvedPath, c.Name(), c.ID())
 
 	return func() error {

libpod/container_copy_freebsd.go

@@ -10,6 +10,6 @@ func (c *Container) joinMountAndExec(f func() error) error {
 
 // Similarly, we can just use resolvePath for both running and stopped
 // containers.
-func (c *Container) resolveCopyTarget(mountPoint string, containerPath string) (string, string, error) {
+func (c *Container) resolveCopyTarget(mountPoint string, containerPath string) (string, string, *Volume, error) {
 	return c.resolvePath(mountPoint, containerPath)
 }

libpod/container_copy_linux.go

@@ -79,12 +79,12 @@ func (c *Container) joinMountAndExec(f func() error) error {
 	return <-errChan
 }
 
-func (c *Container) resolveCopyTarget(mountPoint string, containerPath string) (string, string, error) {
+func (c *Container) resolveCopyTarget(mountPoint string, containerPath string) (string, string, *Volume, error) {
 	// If the container is running, we will execute the copy
 	// inside the container's mount namespace so we return a path
 	// relative to the container's root.
 	if c.state.State == define.ContainerStateRunning {
-		return "/", c.pathAbs(containerPath), nil
+		return "/", c.pathAbs(containerPath), nil, nil
 	}
 	return c.resolvePath(mountPoint, containerPath)
 }

libpod/container_internal_common.go

@@ -741,7 +741,7 @@ func (c *Container) isWorkDirSymlink(resolvedPath string) bool {
 			break
 		}
 		if resolvedSymlink != "" {
-			_, resolvedSymlinkWorkdir, err := c.resolvePath(c.state.Mountpoint, resolvedSymlink)
+			_, resolvedSymlinkWorkdir, _, err := c.resolvePath(c.state.Mountpoint, resolvedSymlink)
 			if isPathOnVolume(c, resolvedSymlinkWorkdir) || isPathOnMount(c, resolvedSymlinkWorkdir) {
 				// Resolved symlink exists on external volume or mount
 				return true
@@ -780,7 +780,7 @@ func (c *Container) resolveWorkDir() error {
 		return nil
 	}
 
-	_, resolvedWorkdir, err := c.resolvePath(c.state.Mountpoint, workdir)
+	_, resolvedWorkdir, _, err := c.resolvePath(c.state.Mountpoint, workdir)
 	if err != nil {
 		return err
 	}
@@ -2963,7 +2963,11 @@ func (c *Container) fixVolumePermissions(v *ContainerNamedVolume) error {
 			return nil
 		}
 
-		st, err := os.Lstat(filepath.Join(c.state.Mountpoint, v.Dest))
+		finalPath, err := securejoin.SecureJoin(c.state.Mountpoint, v.Dest)
+		if err != nil {
+			return err
+		}
+		st, err := os.Lstat(finalPath)
 		if err == nil {
 			if stat, ok := st.Sys().(*syscall.Stat_t); ok {
 				uid, gid := int(stat.Uid), int(stat.Gid)

libpod/container_path_resolution.go

@@ -33,8 +33,8 @@ func (c *Container) pathAbs(path string) string {
 // 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 (c *Container) resolvePath(mountPoint string, containerPath string) (string, string, error) {
+// the host). If the path is on a named volume, the volume is returned.
+func (c *Container) resolvePath(mountPoint string, containerPath string) (string, string, *Volume, error) {
 	// Let's first make sure we have a path relative to the mount point.
 	pathRelativeToContainerMountPoint := c.pathAbs(containerPath)
 	resolvedPathOnTheContainerMountPoint := filepath.Join(mountPoint, pathRelativeToContainerMountPoint)
@@ -54,21 +54,17 @@ func (c *Container) resolvePath(mountPoint string, containerPath string) (string
 	for {
 		volume, err := findVolume(c, searchPath)
 		if err != nil {
-			return "", "", err
+			return "", "", nil, 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
+				return "", "", nil, err
 			}
 			if mountPoint == "" {
-				return "", "", fmt.Errorf("volume %s is not mounted, cannot copy into it", volume.Name())
+				return "", "", nil, fmt.Errorf("volume %s is not mounted, cannot copy into it", volume.Name())
 			}
 
 			// We found a matching volume for searchPath.  We now
@@ -78,9 +74,9 @@ func (c *Container) resolvePath(mountPoint string, containerPath string) (string
 			pathRelativeToVolume := strings.TrimPrefix(pathRelativeToContainerMountPoint, searchPath)
 			absolutePathOnTheVolumeMount, err := securejoin.SecureJoin(mountPoint, pathRelativeToVolume)
 			if err != nil {
-				return "", "", err
+				return "", "", nil, err
 			}
-			return mountPoint, absolutePathOnTheVolumeMount, nil
+			return mountPoint, absolutePathOnTheVolumeMount, volume, nil
 		}
 
 		if mount := findBindMount(c, searchPath); mount != nil {
@@ -92,9 +88,9 @@ func (c *Container) resolvePath(mountPoint string, containerPath string) (string
 			pathRelativeToBindMount := strings.TrimPrefix(pathRelativeToContainerMountPoint, searchPath)
 			absolutePathOnTheBindMount, err := securejoin.SecureJoin(mount.Source, pathRelativeToBindMount)
 			if err != nil {
-				return "", "", err
+				return "", "", nil, err
 			}
-			return mount.Source, absolutePathOnTheBindMount, nil
+			return mount.Source, absolutePathOnTheBindMount, nil, nil
 		}
 
 		if searchPath == "/" {
@@ -106,7 +102,7 @@ func (c *Container) resolvePath(mountPoint string, containerPath string) (string
 	}
 
 	// No volume, no bind mount but just a normal path on the container.
-	return mountPoint, resolvedPathOnTheContainerMountPoint, nil
+	return mountPoint, resolvedPathOnTheContainerMountPoint, nil, nil
 }
 
 // findVolume checks if the specified containerPath matches the destination

libpod/container_stat_common.go

@@ -21,7 +21,7 @@ import (
 func (c *Container) statOnHost(mountPoint string, containerPath string) (*copier.StatForItem, string, string, error) {
 	// Now resolve the container's path.  It may hit a volume, it may hit a
 	// bind mount, it may be relative.
-	resolvedRoot, resolvedPath, err := c.resolvePath(mountPoint, containerPath)
+	resolvedRoot, resolvedPath, _, err := c.resolvePath(mountPoint, containerPath)
 	if err != nil {
 		return nil, "", "", err
 	}

test/e2e/cp_test.go

@@ -3,6 +3,7 @@
 package integration
 
 import (
+	"fmt"
 	"os"
 	"os/exec"
 	"os/user"
@@ -261,4 +262,40 @@ var _ = Describe("Podman cp", func() {
 		Expect(lsOutput).To(ContainSubstring("bin"))
 		Expect(lsOutput).To(ContainSubstring("usr"))
 	})
+
+	It("podman cp to volume in container that is not running", func() {
+		volName := "testvol"
+		ctrName := "testctr"
+		imgName := "testimg"
+		ctrVolPath := "/test/"
+
+		dockerfile := fmt.Sprintf(`FROM %s
+RUN mkdir %s
+RUN chown 9999:9999 %s`, ALPINE, ctrVolPath, ctrVolPath)
+
+		podmanTest.BuildImage(dockerfile, imgName, "false")
+
+		srcFile, err := os.CreateTemp("", "")
+		Expect(err).ToNot(HaveOccurred())
+		defer srcFile.Close()
+		defer os.Remove(srcFile.Name())
+
+		volCreate := podmanTest.Podman([]string{"volume", "create", volName})
+		volCreate.WaitWithDefaultTimeout()
+		Expect(volCreate).Should(ExitCleanly())
+
+		ctrCreate := podmanTest.Podman([]string{"create", "--name", ctrName, "-v", fmt.Sprintf("%s:%s", volName, ctrVolPath), imgName, "sh"})
+		ctrCreate.WaitWithDefaultTimeout()
+		Expect(ctrCreate).To(ExitCleanly())
+
+		cp := podmanTest.Podman([]string{"cp", srcFile.Name(), fmt.Sprintf("%s:%s", ctrName, ctrVolPath)})
+		cp.WaitWithDefaultTimeout()
+		Expect(cp).To(ExitCleanly())
+
+		ls := podmanTest.Podman([]string{"run", "-v", fmt.Sprintf("%s:%s", volName, ctrVolPath), ALPINE, "ls", "-al", ctrVolPath})
+		ls.WaitWithDefaultTimeout()
+		Expect(ls).To(ExitCleanly())
+		Expect(ls.OutputToString()).To(ContainSubstring("9999 9999"))
+		Expect(ls.OutputToString()).To(ContainSubstring(filepath.Base(srcFile.Name())))
+	})
 })