From a30fd8f6107780f13f0274e5c5c8ed0ae9aaa363 Mon Sep 17 00:00:00 2001
From: Valentin Rothberg <rothberg@redhat.com>
Date: Mon, 8 Feb 2021 13:51:31 +0100
Subject: make `podman rmi` more robust
The c/storage library is subject to TOCTOUs as the central container and
image storage may be shared by many instances of many tools. As shown
in #6510, it's fairly easy to have multiple instances of Podman running
in parallel and yield image-lookup errors when removing them.
The underlying issue is the TOCTOU of removal being split into multiple
stages of first reading the local images and then removing them. Some
images may already have been removed in between the two stages. To make
image removal more robust, handle errors at stage two when a given image
is not present (anymore) in the storage.
Fixes: #6510
Signed-off-by: Valentin Rothberg <rothberg@redhat.com>
---
test/e2e/rmi_test.go | 30 ++++++++++++++++++++++++++++++
1 file changed, 30 insertions(+)
(limited to 'test')
diff --git a/test/e2e/rmi_test.go b/test/e2e/rmi_test.go
index 1f40e4928..c1fbe0440 100644
--- a/test/e2e/rmi_test.go
+++ b/test/e2e/rmi_test.go
@@ -1,7 +1,9 @@
package integration
import (
+ "fmt"
"os"
+ "sync"
. "github.com/containers/podman/v2/test/utils"
. "github.com/onsi/ginkgo"
@@ -276,4 +278,32 @@ RUN find $LOCAL
match, _ := session.ErrorGrepString("image name or ID must be specified")
Expect(match).To(BeTrue())
})
+
+ It("podman image rm - concurrent with shared layers", func() {
+ // #6510 has shown a fairly simple reproducer to force storage
+ // errors during parallel image removal. Since it's subject to
+ // a race, we may not hit the condition a 100 percent of times
+ // but ocal reproducers hit it all the time.
+
+ var wg sync.WaitGroup
+
+ buildAndRemove := func(i int) {
+ defer GinkgoRecover()
+ defer wg.Done()
+ imageName := fmt.Sprintf("rmtest:%d", i)
+ containerfile := `FROM quay.io/libpod/cirros:latest
+RUN ` + fmt.Sprintf("touch %s", imageName)
+
+ podmanTest.BuildImage(containerfile, imageName, "false")
+ session := podmanTest.Podman([]string{"rmi", "-f", imageName})
+ session.WaitWithDefaultTimeout()
+ Expect(session).Should(Exit(0))
+ }
+
+ wg.Add(10)
+ for i := 0; i < 10; i++ {
+ go buildAndRemove(i)
+ }
+ wg.Wait()
+ })
})
--
cgit v1.2.3-54-g00ecf
From bb82839492b5226c8c5b04906a05da311177f713 Mon Sep 17 00:00:00 2001
From: baude <bbaude@redhat.com>
Date: Wed, 10 Feb 2021 13:39:09 -0600
Subject: Display correct value for unlimited ulimit
When doing a container inspect on a container with unlimited ulimits,
the value should be -1. But because the OCI spec requires the ulimit
value to be uint64, we were displaying the inspect values as a uint64 as
well. Simple change to display as an int64.
Fixes: #9303
Signed-off-by: baude <bbaude@redhat.com>
---
libpod/container_inspect.go | 4 ++--
libpod/define/container_inspect.go | 4 ++--
test/e2e/inspect_test.go | 24 ++++++++++++++++++++++++
3 files changed, 28 insertions(+), 4 deletions(-)
(limited to 'test')
diff --git a/libpod/container_inspect.go b/libpod/container_inspect.go
index ac7eae56b..cc8b75472 100644
--- a/libpod/container_inspect.go
+++ b/libpod/container_inspect.go
@@ -864,8 +864,8 @@ func (c *Container) generateInspectContainerHostConfig(ctrSpec *spec.Spec, named
for _, limit := range ctrSpec.Process.Rlimits {
newLimit := define.InspectUlimit{}
newLimit.Name = limit.Type
- newLimit.Soft = limit.Soft
- newLimit.Hard = limit.Hard
+ newLimit.Soft = int64(limit.Soft)
+ newLimit.Hard = int64(limit.Hard)
hostConfig.Ulimits = append(hostConfig.Ulimits, newLimit)
}
}
diff --git a/libpod/define/container_inspect.go b/libpod/define/container_inspect.go
index 9a93e2ffd..7d4b8a46f 100644
--- a/libpod/define/container_inspect.go
+++ b/libpod/define/container_inspect.go
@@ -120,9 +120,9 @@ type InspectUlimit struct {
// Name is the name (type) of the ulimit.
Name string `json:"Name"`
// Soft is the soft limit that will be applied.
- Soft uint64 `json:"Soft"`
+ Soft int64 `json:"Soft"`
// Hard is the hard limit that will be applied.
- Hard uint64 `json:"Hard"`
+ Hard int64 `json:"Hard"`
}
// InspectDevice is a single device that will be mounted into the container.
diff --git a/test/e2e/inspect_test.go b/test/e2e/inspect_test.go
index 8fc9721f9..12bc886a8 100644
--- a/test/e2e/inspect_test.go
+++ b/test/e2e/inspect_test.go
@@ -466,4 +466,28 @@ var _ = Describe("Podman inspect", func() {
Expect(len(inspect)).To(Equal(1))
Expect(len(inspect[0].NetworkSettings.Networks)).To(Equal(1))
})
+
+ It("Container inspect with unlimited uilimits should be -1", func() {
+ ctrName := "testctr"
+ session := podmanTest.Podman([]string{"run", "-d", "--ulimit", "core=-1:-1", "--name", ctrName, ALPINE, "top"})
+ session.WaitWithDefaultTimeout()
+ Expect(session.ExitCode()).To(BeZero())
+
+ inspect := podmanTest.Podman([]string{"inspect", ctrName})
+ inspect.WaitWithDefaultTimeout()
+ Expect(inspect.ExitCode()).To(BeZero())
+
+ data := inspect.InspectContainerToJSON()
+ ulimits := data[0].HostConfig.Ulimits
+ Expect(len(ulimits)).To(BeNumerically(">", 0))
+ found := false
+ for _, ulimit := range ulimits {
+ if ulimit.Name == "RLIMIT_CORE" {
+ found = true
+ Expect(ulimit.Soft).To(BeNumerically("==", -1))
+ Expect(ulimit.Hard).To(BeNumerically("==", -1))
+ }
+ }
+ Expect(found).To(BeTrue())
+ })
})
--
cgit v1.2.3-54-g00ecf
From 9de1a68fff2ea6353823365ad860827a5af4aa99 Mon Sep 17 00:00:00 2001
From: Matthew Heon <mheon@redhat.com>
Date: Wed, 10 Feb 2021 09:46:12 -0500
Subject: Rewrite copy-up to use buildah Copier
The old copy-up implementation was very unhappy with symlinks,
which could cause containers to fail to start for unclear reasons
when a directory we wanted to copy-up contained one. Rewrite to
use the Buildah Copier, which is more recent and should be both
safer and less likely to blow up over links.
At the same time, fix a deadlock in copy-up for volumes requiring
mounting - the Mountpoint() function tried to take the
already-acquired volume lock.
Fixes #6003
Signed-off-by: Matthew Heon <mheon@redhat.com>
---
libpod/container_internal.go | 75 ++++++++++++++++++++++++++------------
libpod/container_internal_linux.go | 17 ---------
libpod/volume.go | 11 +++++-
test/e2e/run_volume_test.go | 18 +++++++++
4 files changed, 78 insertions(+), 43 deletions(-)
(limited to 'test')
diff --git a/libpod/container_internal.go b/libpod/container_internal.go
index 5a61f7fe6..15958471f 100644
--- a/libpod/container_internal.go
+++ b/libpod/container_internal.go
@@ -13,6 +13,7 @@ import (
"strings"
"time"
+ "github.com/containers/buildah/copier"
"github.com/containers/podman/v2/libpod/define"
"github.com/containers/podman/v2/libpod/events"
"github.com/containers/podman/v2/pkg/cgroups"
@@ -1580,18 +1581,8 @@ func (c *Container) mountNamedVolume(v *ContainerNamedVolume, mountpoint string)
return nil, err
}
- // HACK HACK HACK - copy up into a volume driver is 100% broken
- // right now.
- if vol.UsesVolumeDriver() {
- logrus.Infof("Not copying up into volume %s as it uses a volume driver", vol.Name())
- return vol, nil
- }
-
// If the volume is not empty, we should not copy up.
- volMount, err := vol.MountPoint()
- if err != nil {
- return nil, err
- }
+ volMount := vol.mountPoint()
contents, err := ioutil.ReadDir(volMount)
if err != nil {
return nil, errors.Wrapf(err, "error listing contents of volume %s mountpoint when copying up from container %s", vol.Name(), c.ID())
@@ -1607,8 +1598,55 @@ func (c *Container) mountNamedVolume(v *ContainerNamedVolume, mountpoint string)
if err != nil {
return nil, errors.Wrapf(err, "error calculating destination path to copy up container %s volume %s", c.ID(), vol.Name())
}
- if err := c.copyWithTarFromImage(srcDir, volMount); err != nil && !os.IsNotExist(err) {
- return nil, errors.Wrapf(err, "error copying content from container %s into volume %s", c.ID(), vol.Name())
+ // Do a manual stat on the source directory to verify existence.
+ // Skip the rest if it exists.
+ // TODO: Should this be stat or lstat? I'm using lstat because I
+ // think copy-up doesn't happen when the source is a link.
+ srcStat, err := os.Lstat(srcDir)
+ if err != nil {
+ if os.IsNotExist(err) {
+ // Source does not exist, don't bother copying
+ // up.
+ return vol, nil
+ }
+ return nil, errors.Wrapf(err, "error identifying source directory for copy up into volume %s", vol.Name())
+ }
+ // If it's not a directory we're mounting over it.
+ if !srcStat.IsDir() {
+ return vol, nil
+ }
+
+ // Buildah Copier accepts a reader, so we'll need a pipe.
+ reader, writer := io.Pipe()
+ defer reader.Close()
+
+ errChan := make(chan error, 1)
+
+ logrus.Infof("About to copy up into volume %s", vol.Name())
+
+ // Copy, container side: get a tar archive of what needs to be
+ // streamed into the volume.
+ go func() {
+ defer writer.Close()
+ getOptions := copier.GetOptions{
+ KeepDirectoryNames: false,
+ }
+ errChan <- copier.Get(mountpoint, "", getOptions, []string{v.Dest + "/."}, writer)
+ }()
+
+ // Copy, volume side: stream what we've written to the pipe, into
+ // the volume.
+ copyOpts := copier.PutOptions{}
+ if err := copier.Put(volMount, "", copyOpts, reader); err != nil {
+ err2 := <-errChan
+ if err2 != nil {
+ logrus.Errorf("Error streaming contents of container %s directory for volume copy-up: %v", c.ID(), err2)
+ }
+ return nil, errors.Wrapf(err, "error copying up to volume %s", vol.Name())
+ }
+
+ if err := <-errChan; err != nil {
+ return nil, errors.Wrapf(err, "error streaming container content for copy up into volume %s", vol.Name())
}
}
return vol, nil
@@ -2058,17 +2096,6 @@ func (c *Container) unmount(force bool) error {
return nil
}
-// this should be from chrootarchive.
-// Container MUST be mounted before calling.
-func (c *Container) copyWithTarFromImage(source, dest string) error {
- mappings := idtools.NewIDMappingsFromMaps(c.config.IDMappings.UIDMap, c.config.IDMappings.GIDMap)
- a := archive.NewArchiver(mappings)
- if err := c.copyOwnerAndPerms(source, dest); err != nil {
- return err
- }
- return a.CopyWithTar(source, dest)
-}
-
// checkReadyForRemoval checks whether the given container is ready to be
// removed.
// These checks are only used if force-remove is not specified.
diff --git a/libpod/container_internal_linux.go b/libpod/container_internal_linux.go
index ba85a1f47..952cc42d1 100644
--- a/libpod/container_internal_linux.go
+++ b/libpod/container_internal_linux.go
@@ -2261,23 +2261,6 @@ func (c *Container) generatePasswdAndGroup() (string, string, error) {
return passwdPath, groupPath, nil
}
-func (c *Container) copyOwnerAndPerms(source, dest string) error {
- info, err := os.Stat(source)
- if err != nil {
- if os.IsNotExist(err) {
- return nil
- }
- return err
- }
- if err := os.Chmod(dest, info.Mode()); err != nil {
- return err
- }
- if err := os.Chown(dest, int(info.Sys().(*syscall.Stat_t).Uid), int(info.Sys().(*syscall.Stat_t).Gid)); err != nil {
- return err
- }
- return nil
-}
-
// Get cgroup path in a format suitable for the OCI spec
func (c *Container) getOCICgroupPath() (string, error) {
unified, err := cgroups.IsCgroup2UnifiedMode()
diff --git a/libpod/volume.go b/libpod/volume.go
index 4c137cb8e..5cc5e7e40 100644
--- a/libpod/volume.go
+++ b/libpod/volume.go
@@ -130,11 +130,18 @@ func (v *Volume) MountPoint() (string, error) {
if err := v.update(); err != nil {
return "", err
}
+ }
+
+ return v.mountPoint(), nil
+}
- return v.state.MountPoint, nil
+// Internal-only helper for volume mountpoint
+func (v *Volume) mountPoint() string {
+ if v.UsesVolumeDriver() {
+ return v.state.MountPoint
}
- return v.config.MountPoint, nil
+ return v.config.MountPoint
}
// Options return the volume's options
diff --git a/test/e2e/run_volume_test.go b/test/e2e/run_volume_test.go
index bc89b59de..19d82c974 100644
--- a/test/e2e/run_volume_test.go
+++ b/test/e2e/run_volume_test.go
@@ -304,6 +304,24 @@ var _ = Describe("Podman run with volumes", func() {
Expect(separateVolumeSession.OutputToString()).To(Equal(baselineOutput))
})
+ It("podman named volume copyup symlink", func() {
+ imgName := "testimg"
+ dockerfile := `FROM alpine
+RUN touch /testfile
+RUN sh -c "cd /etc/apk && ln -s ../../testfile"`
+ podmanTest.BuildImage(dockerfile, imgName, "false")
+
+ baselineSession := podmanTest.Podman([]string{"run", "--rm", "-t", "-i", imgName, "ls", "/etc/apk/"})
+ baselineSession.WaitWithDefaultTimeout()
+ Expect(baselineSession.ExitCode()).To(Equal(0))
+ baselineOutput := baselineSession.OutputToString()
+
+ outputSession := podmanTest.Podman([]string{"run", "-t", "-i", "-v", "/etc/apk/", imgName, "ls", "/etc/apk/"})
+ outputSession.WaitWithDefaultTimeout()
+ Expect(outputSession.ExitCode()).To(Equal(0))
+ Expect(outputSession.OutputToString()).To(Equal(baselineOutput))
+ })
+
It("podman read-only tmpfs conflict with volume", func() {
session := podmanTest.Podman([]string{"run", "--rm", "-t", "-i", "--read-only", "-v", "tmp_volume:" + dest, ALPINE, "touch", dest + "/a"})
session.WaitWithDefaultTimeout()
--
cgit v1.2.3-54-g00ecf