From 7c75e84e22239a54d96eb2f32d218ccfea38ee87 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Thu, 9 Feb 2023 18:45:24 -0800 Subject: [PATCH 1/3] libc/int: add/use runContainerOk wrapper This is to de-duplicate the code that checks that err is nil and that the exit code is zero. Signed-off-by: Kir Kolyshkin --- libcontainer/integration/exec_test.go | 101 ++++------------------- libcontainer/integration/seccomp_test.go | 16 +--- libcontainer/integration/utils_test.go | 16 ++++ 3 files changed, 35 insertions(+), 98 deletions(-) diff --git a/libcontainer/integration/exec_test.go b/libcontainer/integration/exec_test.go index cb5c1b70f..b35457951 100644 --- a/libcontainer/integration/exec_test.go +++ b/libcontainer/integration/exec_test.go @@ -40,13 +40,7 @@ func testExecPS(t *testing.T, userns bool) { } config := newTemplateConfig(t, &tParam{userns: userns}) - buffers, exitCode, err := runContainer(t, config, "ps", "-o", "pid,user,comm") - if err != nil { - t.Fatalf("%s: %s", buffers, err) - } - if exitCode != 0 { - t.Fatalf("exit code not 0. code %d stderr %q", exitCode, buffers.Stderr) - } + buffers := runContainerOk(t, config, "ps", "-o", "pid,user,comm") lines := strings.Split(buffers.Stdout.String(), "\n") if len(lines) < 2 { t.Fatalf("more than one process running for output %q", buffers.Stdout.String()) @@ -67,12 +61,7 @@ func TestIPCPrivate(t *testing.T) { ok(t, err) config := newTemplateConfig(t, nil) - buffers, exitCode, err := runContainer(t, config, "readlink", "/proc/self/ns/ipc") - ok(t, err) - - if exitCode != 0 { - t.Fatalf("exit code not 0. code %d stderr %q", exitCode, buffers.Stderr) - } + buffers := runContainerOk(t, config, "readlink", "/proc/self/ns/ipc") if actual := strings.Trim(buffers.Stdout.String(), "\n"); actual == l { t.Fatalf("ipc link should be private to the container but equals host %q %q", actual, l) @@ -89,12 +78,7 @@ func TestIPCHost(t *testing.T) { config := newTemplateConfig(t, nil) config.Namespaces.Remove(configs.NEWIPC) - buffers, exitCode, err := runContainer(t, config, "readlink", "/proc/self/ns/ipc") - ok(t, err) - - if exitCode != 0 { - t.Fatalf("exit code not 0. code %d stderr %q", exitCode, buffers.Stderr) - } + buffers := runContainerOk(t, config, "readlink", "/proc/self/ns/ipc") if actual := strings.Trim(buffers.Stdout.String(), "\n"); actual != l { t.Fatalf("ipc link not equal to host link %q %q", actual, l) @@ -111,13 +95,7 @@ func TestIPCJoinPath(t *testing.T) { config := newTemplateConfig(t, nil) config.Namespaces.Add(configs.NEWIPC, "/proc/1/ns/ipc") - - buffers, exitCode, err := runContainer(t, config, "readlink", "/proc/self/ns/ipc") - ok(t, err) - - if exitCode != 0 { - t.Fatalf("exit code not 0. code %d stderr %q", exitCode, buffers.Stderr) - } + buffers := runContainerOk(t, config, "readlink", "/proc/self/ns/ipc") if actual := strings.Trim(buffers.Stdout.String(), "\n"); actual != l { t.Fatalf("ipc link not equal to host link %q %q", actual, l) @@ -163,8 +141,7 @@ func testRlimit(t *testing.T, userns bool) { Cur: 1024, })) - out, _, err := runContainer(t, config, "/bin/sh", "-c", "ulimit -n") - ok(t, err) + out := runContainerOk(t, config, "/bin/sh", "-c", "ulimit -n") if limit := strings.TrimSpace(out.Stdout.String()); limit != "1025" { t.Fatalf("expected rlimit to be 1025, got %s", limit) } @@ -560,27 +537,17 @@ func testPids(t *testing.T, systemd bool) { config := newTemplateConfig(t, &tParam{systemd: systemd}) config.Cgroups.Resources.PidsLimit = -1 - // Running multiple processes. - _, ret, err := runContainer(t, config, "/bin/sh", "-c", "/bin/true | /bin/true | /bin/true | /bin/true") - ok(t, err) - - if ret != 0 { - t.Fatalf("expected fork() to succeed with no pids limit") - } + // Running multiple processes, expecting it to succeed with no pids limit. + _ = runContainerOk(t, config, "/bin/sh", "-c", "/bin/true | /bin/true | /bin/true | /bin/true") // Enforce a permissive limit. This needs to be fairly hand-wavey due to the // issues with running Go binaries with pids restrictions (see below). config.Cgroups.Resources.PidsLimit = 64 - _, ret, err = runContainer(t, config, "/bin/sh", "-c", ` + _ = runContainerOk(t, config, "/bin/sh", "-c", ` /bin/true | /bin/true | /bin/true | /bin/true | /bin/true | /bin/true | bin/true | /bin/true | /bin/true | /bin/true | /bin/true | /bin/true | /bin/true | /bin/true | bin/true | /bin/true | /bin/true | /bin/true | /bin/true | /bin/true | /bin/true | /bin/true | bin/true | /bin/true | /bin/true | /bin/true | /bin/true | /bin/true | /bin/true | /bin/true | bin/true | /bin/true`) - ok(t, err) - - if ret != 0 { - t.Fatalf("expected fork() to succeed with permissive pids limit") - } // Enforce a restrictive limit. 64 * /bin/true + 1 * shell should cause this // to fail reliability. @@ -884,13 +851,8 @@ func TestMountCgroupRO(t *testing.T) { return } config := newTemplateConfig(t, nil) - buffers, exitCode, err := runContainer(t, config, "mount") - if err != nil { - t.Fatalf("%s: %s", buffers, err) - } - if exitCode != 0 { - t.Fatalf("exit code not 0. code %d stderr %q", exitCode, buffers.Stderr) - } + buffers := runContainerOk(t, config, "mount") + mountInfo := buffers.Stdout.String() lines := strings.Split(mountInfo, "\n") for _, l := range lines { @@ -931,13 +893,8 @@ func TestMountCgroupRW(t *testing.T) { } } - buffers, exitCode, err := runContainer(t, config, "mount") - if err != nil { - t.Fatalf("%s: %s", buffers, err) - } - if exitCode != 0 { - t.Fatalf("exit code not 0. code %d stderr %q", exitCode, buffers.Stderr) - } + buffers := runContainerOk(t, config, "mount") + mountInfo := buffers.Stdout.String() lines := strings.Split(mountInfo, "\n") for _, l := range lines { @@ -1148,11 +1105,7 @@ func TestSTDIOPermissions(t *testing.T) { } config := newTemplateConfig(t, nil) - buffers, exitCode, err := runContainer(t, config, "sh", "-c", "echo hi > /dev/stderr") - ok(t, err) - if exitCode != 0 { - t.Fatalf("exit code not 0. code %d stderr %q", exitCode, buffers.Stderr) - } + buffers := runContainerOk(t, config, "sh", "-c", "echo hi > /dev/stderr") if actual := strings.Trim(buffers.Stderr.String(), "\n"); actual != "hi" { t.Fatalf("stderr should equal be equal %q %q", actual, "hi") @@ -1395,12 +1348,7 @@ func TestPIDHost(t *testing.T) { config := newTemplateConfig(t, nil) config.Namespaces.Remove(configs.NEWPID) - buffers, exitCode, err := runContainer(t, config, "readlink", "/proc/self/ns/pid") - ok(t, err) - - if exitCode != 0 { - t.Fatalf("exit code not 0. code %d stderr %q", exitCode, buffers.Stderr) - } + buffers := runContainerOk(t, config, "readlink", "/proc/self/ns/pid") if actual := strings.Trim(buffers.Stdout.String(), "\n"); actual != l { t.Fatalf("ipc link not equal to host link %q %q", actual, l) @@ -1689,12 +1637,7 @@ func TestCGROUPPrivate(t *testing.T) { config := newTemplateConfig(t, nil) config.Namespaces.Add(configs.NEWCGROUP, "") - buffers, exitCode, err := runContainer(t, config, "readlink", "/proc/self/ns/cgroup") - ok(t, err) - - if exitCode != 0 { - t.Fatalf("exit code not 0. code %d stderr %q", exitCode, buffers.Stderr) - } + buffers := runContainerOk(t, config, "readlink", "/proc/self/ns/cgroup") if actual := strings.Trim(buffers.Stdout.String(), "\n"); actual == l { t.Fatalf("cgroup link should be private to the container but equals host %q %q", actual, l) @@ -1713,12 +1656,7 @@ func TestCGROUPHost(t *testing.T) { ok(t, err) config := newTemplateConfig(t, nil) - buffers, exitCode, err := runContainer(t, config, "readlink", "/proc/self/ns/cgroup") - ok(t, err) - - if exitCode != 0 { - t.Fatalf("exit code not 0. code %d stderr %q", exitCode, buffers.Stderr) - } + buffers := runContainerOk(t, config, "readlink", "/proc/self/ns/cgroup") if actual := strings.Trim(buffers.Stdout.String(), "\n"); actual != l { t.Fatalf("cgroup link not equal to host link %q %q", actual, l) @@ -1750,12 +1688,7 @@ func testFdLeaks(t *testing.T, systemd bool) { ok(t, err) config := newTemplateConfig(t, &tParam{systemd: systemd}) - buffers, exitCode, err := runContainer(t, config, "true") - ok(t, err) - - if exitCode != 0 { - t.Fatalf("exit code not 0. code %d stderr %q", exitCode, buffers.Stderr) - } + _ = runContainerOk(t, config, "true") fds1, err := pfd.Readdirnames(0) ok(t, err) diff --git a/libcontainer/integration/seccomp_test.go b/libcontainer/integration/seccomp_test.go index a7eeefb1b..31092a0a5 100644 --- a/libcontainer/integration/seccomp_test.go +++ b/libcontainer/integration/seccomp_test.go @@ -282,13 +282,7 @@ func TestSeccompPermitWriteMultipleConditions(t *testing.T) { }, } - buffers, exitCode, err := runContainer(t, config, "ls", "/") - if err != nil { - t.Fatalf("%s: %s", buffers, err) - } - if exitCode != 0 { - t.Fatalf("exit code not 0. code %d buffers %s", exitCode, buffers) - } + buffers := runContainerOk(t, config, "ls", "/") // We don't need to verify the actual thing printed // Just that something was written to stdout if len(buffers.Stdout.String()) == 0 { @@ -375,13 +369,7 @@ func TestSeccompMultipleConditionSameArgDeniesStdout(t *testing.T) { }, } - buffers, exitCode, err := runContainer(t, config, "ls", "/") - if err != nil { - t.Fatalf("%s: %s", buffers, err) - } - if exitCode != 0 { - t.Fatalf("exit code not 0. code %d buffers %s", exitCode, buffers) - } + buffers := runContainerOk(t, config, "ls", "/") // Verify that nothing was printed if len(buffers.Stdout.String()) != 0 { t.Fatalf("Something was written to stdout, write call succeeded!\n") diff --git a/libcontainer/integration/utils_test.go b/libcontainer/integration/utils_test.go index 3bacf93d0..780288ad0 100644 --- a/libcontainer/integration/utils_test.go +++ b/libcontainer/integration/utils_test.go @@ -212,6 +212,22 @@ func runContainer(t *testing.T, config *configs.Config, args ...string) (buffers return } +// runContainerOk is a wrapper for runContainer, simplifying its use for cases +// when the run is expected to succeed and return exit code of 0. +func runContainerOk(t *testing.T, config *configs.Config, args ...string) *stdBuffers { + buffers, exitCode, err := runContainer(t, config, args...) + + t.Helper() + if err != nil { + t.Fatalf("%s: %s", buffers, err) + } + if exitCode != 0 { + t.Fatalf("exit code not 0. code %d stderr %q", exitCode, buffers.Stderr) + } + + return buffers +} + func destroyContainer(container *libcontainer.Container) { _ = container.Destroy() } From be7e03940fc2e25466bc2724789f06f1150cf0fc Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Thu, 9 Feb 2023 18:50:15 -0800 Subject: [PATCH 2/3] libct/int: wording nits Signed-off-by: Kir Kolyshkin --- libcontainer/integration/exec_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/libcontainer/integration/exec_test.go b/libcontainer/integration/exec_test.go index b35457951..dd12267ce 100644 --- a/libcontainer/integration/exec_test.go +++ b/libcontainer/integration/exec_test.go @@ -514,7 +514,7 @@ func testCpuShares(t *testing.T, systemd bool) { config.Cgroups.Resources.CpuShares = 1 if _, _, err := runContainer(t, config, "ps"); err == nil { - t.Fatalf("runContainer should failed with invalid CpuShares") + t.Fatal("runContainer should fail with invalid CpuShares") } } @@ -549,8 +549,8 @@ func testPids(t *testing.T, systemd bool) { /bin/true | /bin/true | /bin/true | /bin/true | /bin/true | /bin/true | bin/true | /bin/true | /bin/true | /bin/true | /bin/true | /bin/true | /bin/true | /bin/true | bin/true | /bin/true`) - // Enforce a restrictive limit. 64 * /bin/true + 1 * shell should cause this - // to fail reliability. + // Enforce a restrictive limit. 64 * /bin/true + 1 * shell should cause + // this to fail reliably. config.Cgroups.Resources.PidsLimit = 64 out, _, err := runContainer(t, config, "/bin/sh", "-c", ` /bin/true | /bin/true | /bin/true | /bin/true | /bin/true | /bin/true | bin/true | /bin/true | From f2e71b085d923b978f4dd9ee78ed3d5f63035795 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Thu, 9 Feb 2023 19:00:00 -0800 Subject: [PATCH 3/3] libct/int: make TestFdLeaks more robust The purpose of this test is to check that there are no extra file descriptors left open after repeated calls to runContainer. In fact, the first call to runContainer leaves a few file descriptors opened, and this is by design. Previously, this test relied on two things: 1. some other tests were run before it (and thus all such opened-once file descriptors are already opened); 2. explicitly excluding fd opened to /sys/fs/cgroup. Now, if we run this test separately, it will fail (because of 1 above). The same may happen if the tests are run in a random order. To fix this, add a container run before collection the initial fd list, so those fds that are opened once are included and won't be reported. Signed-off-by: Kir Kolyshkin --- libcontainer/integration/exec_test.go | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/libcontainer/integration/exec_test.go b/libcontainer/integration/exec_test.go index dd12267ce..530faf1ae 100644 --- a/libcontainer/integration/exec_test.go +++ b/libcontainer/integration/exec_test.go @@ -1679,6 +1679,16 @@ func testFdLeaks(t *testing.T, systemd bool) { return } + config := newTemplateConfig(t, &tParam{systemd: systemd}) + // Run a container once to exclude file descriptors that are only + // opened once during the process lifetime by the library and are + // never closed. Those are not considered leaks. + // + // Examples of this open-once file descriptors are: + // - /sys/fs/cgroup dirfd opened by prepareOpenat2 in libct/cgroups; + // - dbus connection opened by getConnection in libct/cgroups/systemd. + _ = runContainerOk(t, config, "true") + pfd, err := os.Open("/proc/self/fd") ok(t, err) defer pfd.Close() @@ -1687,7 +1697,6 @@ func testFdLeaks(t *testing.T, systemd bool) { _, err = pfd.Seek(0, 0) ok(t, err) - config := newTemplateConfig(t, &tParam{systemd: systemd}) _ = runContainerOk(t, config, "true") fds1, err := pfd.Readdirnames(0) @@ -1699,7 +1708,6 @@ func testFdLeaks(t *testing.T, systemd bool) { // Show the extra opened files. excludedPaths := []string{ - "/sys/fs/cgroup", // opened once, see prepareOpenat2 "anon_inode:bpf-prog", // FIXME: see https://github.com/opencontainers/runc/issues/2366#issuecomment-776411392 }