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