From 44b0c24ca51f6390035c735a604b11752821fa50 Mon Sep 17 00:00:00 2001 From: Matt Heon Date: Mon, 25 Nov 2024 10:05:12 -0500 Subject: [PATCH 1/2] Revert "libpod: remove shutdown.Unregister()" This reverts commit 5de7b7c3f379987faa9cb06a58be8914f2d56cbe. We now require the Unregister shutdown handler function for handling unmounting named volumes after `podman cp` into a stopped container. Signed-off-by: Matt Heon --- libpod/shutdown/handler.go | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/libpod/shutdown/handler.go b/libpod/shutdown/handler.go index e06e818c7c..ac41899e0e 100644 --- a/libpod/shutdown/handler.go +++ b/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 +} From e66b788a514fb8df2c8b8d3c000e0d543bbd60df Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Fri, 22 Nov 2024 11:45:58 -0500 Subject: [PATCH 2/2] 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 --- libpod/container_copy_common.go | 137 +++++++++++++++++++++++++++- libpod/container_copy_freebsd.go | 2 +- libpod/container_copy_linux.go | 4 +- libpod/container_internal_common.go | 10 +- libpod/container_path_resolution.go | 24 ++--- libpod/container_stat_common.go | 2 +- test/e2e/cp_test.go | 37 ++++++++ 7 files changed, 194 insertions(+), 22 deletions(-) diff --git a/libpod/container_copy_common.go b/libpod/container_copy_common.go index e32f79dc09..0553cedf8a 100644 --- a/libpod/container_copy_common.go +++ b/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 { diff --git a/libpod/container_copy_freebsd.go b/libpod/container_copy_freebsd.go index 9697222d31..60d15646a6 100644 --- a/libpod/container_copy_freebsd.go +++ b/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) } diff --git a/libpod/container_copy_linux.go b/libpod/container_copy_linux.go index 149da9b381..eda2857330 100644 --- a/libpod/container_copy_linux.go +++ b/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) } diff --git a/libpod/container_internal_common.go b/libpod/container_internal_common.go index 4f94c22665..5ee01bdd19 100644 --- a/libpod/container_internal_common.go +++ b/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) diff --git a/libpod/container_path_resolution.go b/libpod/container_path_resolution.go index 068cd284ea..03f8e5a9c0 100644 --- a/libpod/container_path_resolution.go +++ b/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 diff --git a/libpod/container_stat_common.go b/libpod/container_stat_common.go index 9ad7acb4ee..5109c3ee1e 100644 --- a/libpod/container_stat_common.go +++ b/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 } diff --git a/test/e2e/cp_test.go b/test/e2e/cp_test.go index f71963a811..5c543c3e3d 100644 --- a/test/e2e/cp_test.go +++ b/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()))) + }) })