From d36dd1e792a5a7a2ccd5bbebe0ca2300f2d18542 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Thu, 8 Mar 2018 12:24:39 -0800 Subject: [PATCH 1/3] daemon/oci_linux_test: add TestIpcPrivateVsReadonly The test case checks that in case of IpcMode: private and ReadonlyRootfs: true (as in "docker run --ipc private --read-only") the resulting /dev/shm mount is NOT made read-only. Signed-off-by: Kir Kolyshkin Upstream-commit: 33dd562e3acff71ee18a2543d14fcbecf9bf0e62 Component: engine --- components/engine/daemon/oci_linux_test.go | 38 ++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/components/engine/daemon/oci_linux_test.go b/components/engine/daemon/oci_linux_test.go index 4af0ba96d0..f6bda79745 100644 --- a/components/engine/daemon/oci_linux_test.go +++ b/components/engine/daemon/oci_linux_test.go @@ -48,3 +48,41 @@ func TestTmpfsDevShmNoDupMount(t *testing.T) { err = setMounts(&d, &s, c, ms) assert.NoError(t, err) } + +// TestIpcPrivateVsReadonly checks that in case of IpcMode: private +// and ReadonlyRootfs: true (as in "docker run --ipc private --read-only") +// the resulting /dev/shm mount is NOT made read-only. +// https://github.com/moby/moby/issues/36503 +func TestIpcPrivateVsReadonly(t *testing.T) { + d := Daemon{ + // some empty structs to avoid getting a panic + // caused by a null pointer dereference + idMappings: &idtools.IDMappings{}, + configStore: &config.Config{}, + } + c := &container.Container{ + HostConfig: &containertypes.HostConfig{ + IpcMode: containertypes.IpcMode("private"), + ReadonlyRootfs: true, + }, + } + + // We can't call createSpec() so mimick the minimal part + // of its code flow, just enough to reproduce the issue. + ms, err := d.setupMounts(c) + assert.NoError(t, err) + + s := oci.DefaultSpec() + s.Root.Readonly = c.HostConfig.ReadonlyRootfs + + err = setMounts(&d, &s, c, ms) + assert.NoError(t, err) + + // Find the /dev/shm mount in ms, check it does not have ro + for _, m := range s.Mounts { + if m.Destination != "/dev/shm" { + continue + } + assert.Equal(t, false, inSlice(m.Options, "ro")) + } +} From 11d01cef448eec5b985ca2893f658c6e3d252aa7 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Wed, 7 Mar 2018 20:14:16 -0800 Subject: [PATCH 2/3] daemon/setMounts(): do not make /dev/shm ro It has been pointed out that if --read-only flag is given, /dev/shm also becomes read-only in case of --ipc private. This happens because in this case the mount comes from OCI spec (since commit 7120976d74195), and is a regression caused by that commit. The meaning of --read-only flag is to only have a "main" container filesystem read-only, not the auxiliary stuff (that includes /dev/shm, other mounts and volumes, --tmpfs, /proc, /dev and so on). So, let's make sure /dev/shm that comes from OCI spec is not made read-only. Fixes: 7120976d74195 ("Implement none, private, and shareable ipc modes") Signed-off-by: Kir Kolyshkin Upstream-commit: cad74056c09f6276b0f4a996a1511553177cd3d7 Component: engine --- components/engine/daemon/oci_linux.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/components/engine/daemon/oci_linux.go b/components/engine/daemon/oci_linux.go index 15bcb705bf..8d5eebb885 100644 --- a/components/engine/daemon/oci_linux.go +++ b/components/engine/daemon/oci_linux.go @@ -667,7 +667,7 @@ func setMounts(daemon *Daemon, s *specs.Spec, c *container.Container, mounts []c if s.Root.Readonly { for i, m := range s.Mounts { switch m.Destination { - case "/proc", "/dev/pts", "/dev/mqueue", "/dev": + case "/proc", "/dev/pts", "/dev/shm", "/dev/mqueue", "/dev": continue } if _, ok := userMounts[m.Destination]; !ok { From 1eac9f0c3c4e04a220ad60bb879e17027dad5102 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Fri, 9 Mar 2018 12:27:47 -0500 Subject: [PATCH 3/3] Remove unnecessary diff tests Signed-off-by: Daniel Nephin Upstream-commit: 038f3add5191240058c7a4154556553c5493ea44 Component: engine --- .../engine/integration/container/diff_test.go | 73 +++---------------- 1 file changed, 9 insertions(+), 64 deletions(-) diff --git a/components/engine/integration/container/diff_test.go b/components/engine/integration/container/diff_test.go index df60236998..de5ff4e21a 100644 --- a/components/engine/integration/container/diff_test.go +++ b/components/engine/integration/container/diff_test.go @@ -10,13 +10,11 @@ import ( "github.com/docker/docker/integration/internal/request" "github.com/docker/docker/pkg/archive" "github.com/gotestyourself/gotestyourself/poll" - "github.com/gotestyourself/gotestyourself/skip" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) -// ensure that an added file shows up in docker diff -func TestDiffFilenameShownInOutput(t *testing.T) { +func TestDiff(t *testing.T) { defer setupTest(t)() client := request.NewAPIClient(t) ctx := context.Background() @@ -27,72 +25,19 @@ func TestDiffFilenameShownInOutput(t *testing.T) { // it will take a few seconds to exit. Also there's no way in Windows to // differentiate between an Add or a Modify, and all files are under // a "Files/" prefix. - lookingFor := containertypes.ContainerChangeResponseItem{Kind: archive.ChangeAdd, Path: "/foo/bar"} + expected := []containertypes.ContainerChangeResponseItem{ + {Kind: archive.ChangeAdd, Path: "/foo"}, + {Kind: archive.ChangeAdd, Path: "/foo/bar"}, + } if testEnv.OSType == "windows" { poll.WaitOn(t, container.IsInState(ctx, client, cID, "exited"), poll.WithDelay(100*time.Millisecond), poll.WithTimeout(60*time.Second)) - lookingFor = containertypes.ContainerChangeResponseItem{Kind: archive.ChangeModify, Path: "Files/foo/bar"} - } - - items, err := client.ContainerDiff(ctx, cID) - require.NoError(t, err) - assert.Contains(t, items, lookingFor) -} - -// test to ensure GH #3840 doesn't occur any more -func TestDiffEnsureInitLayerFilesAreIgnored(t *testing.T) { - skip.If(t, testEnv.DaemonInfo.OSType != "linux") - - defer setupTest(t)() - client := request.NewAPIClient(t) - ctx := context.Background() - - // this is a list of files which shouldn't show up in `docker diff` - initLayerFiles := []string{"/etc/resolv.conf", "/etc/hostname", "/etc/hosts", "/.dockerenv"} - containerCount := 5 - - // we might not run into this problem from the first run, so start a few containers - for i := 0; i < containerCount; i++ { - cID := container.Run(t, ctx, client, container.WithCmd("sh", "-c", `echo foo > /root/bar`)) - - items, err := client.ContainerDiff(ctx, cID) - require.NoError(t, err) - for _, item := range items { - assert.NotContains(t, initLayerFiles, item.Path) + expected = []containertypes.ContainerChangeResponseItem{ + {Kind: archive.ChangeModify, Path: "Files/foo"}, + {Kind: archive.ChangeModify, Path: "Files/foo/bar"}, } } -} - -func TestDiffEnsureDefaultDevs(t *testing.T) { - skip.If(t, testEnv.DaemonInfo.OSType != "linux") - - defer setupTest(t)() - client := request.NewAPIClient(t) - ctx := context.Background() - - cID := container.Run(t, ctx, client, container.WithCmd("sleep", "0")) items, err := client.ContainerDiff(ctx, cID) require.NoError(t, err) - - expected := []containertypes.ContainerChangeResponseItem{ - {Kind: archive.ChangeModify, Path: "/dev"}, - {Kind: archive.ChangeAdd, Path: "/dev/full"}, // busybox - {Kind: archive.ChangeModify, Path: "/dev/ptmx"}, // libcontainer - {Kind: archive.ChangeAdd, Path: "/dev/mqueue"}, - {Kind: archive.ChangeAdd, Path: "/dev/kmsg"}, - {Kind: archive.ChangeAdd, Path: "/dev/fd"}, - {Kind: archive.ChangeAdd, Path: "/dev/ptmx"}, - {Kind: archive.ChangeAdd, Path: "/dev/null"}, - {Kind: archive.ChangeAdd, Path: "/dev/random"}, - {Kind: archive.ChangeAdd, Path: "/dev/stdout"}, - {Kind: archive.ChangeAdd, Path: "/dev/stderr"}, - {Kind: archive.ChangeAdd, Path: "/dev/tty1"}, - {Kind: archive.ChangeAdd, Path: "/dev/stdin"}, - {Kind: archive.ChangeAdd, Path: "/dev/tty"}, - {Kind: archive.ChangeAdd, Path: "/dev/urandom"}, - } - - for _, item := range items { - assert.Contains(t, expected, item) - } + assert.Equal(t, expected, items) }