Merge pull request #24655 from mheon/fix_volume_perms_cp

Mount volumes before copying into a container

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

@@ -766,7 +766,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
@@ -805,7 +805,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
 	}
@@ -2988,7 +2988,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).
+// the host). If the path is on a named volume, the volume is returned.
-func (c *Container) resolvePath(mountPoint string, containerPath string) (string, string, error) {
+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
 	}

libpod/shutdown/handler.go

@@ -140,3 +140,29 @@ func Register(name string, handler func(os.Signal) error) error {
 
 	return nil
 }
+
+// Unregister un-registers a given shutdown handler.
+func Unregister(name string) error {
+	handlerLock.Lock()
+	defer handlerLock.Unlock()
+
+	if handlers == nil {
+		return nil
+	}
+
+	if _, ok := handlers[name]; !ok {
+		return nil
+	}
+
+	delete(handlers, name)
+
+	newOrder := []string{}
+	for _, checkName := range handlerOrder {
+		if checkName != name {
+			newOrder = append(newOrder, checkName)
+		}
+	}
+	handlerOrder = newOrder
+
+	return nil
+}

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())))
+	})
 })