From 637d54b7ceb0f35dc40bd8aecd7e1abed0c88bcf Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 9 Jun 2020 09:59:50 -0700 Subject: [PATCH] cgroups/fs tests: unify TestInvalid*Cgroup* All the test cases are doing the same checks, only input differs, so we can unify those using a test data table. While at it: - use t.Fatalf where it makes sense (no further checks are possible); - remove the "XXX" comments as we won't get rid of cgroup Name/Parent. PS I tried using t.Parallel() as well but it did not result in any noticeable speedup, so I dropped it for simplicity. Signed-off-by: Kir Kolyshkin --- libcontainer/cgroups/fs/fs_test.go | 346 ++++++----------------------- 1 file changed, 71 insertions(+), 275 deletions(-) diff --git a/libcontainer/cgroups/fs/fs_test.go b/libcontainer/cgroups/fs/fs_test.go index 2467dff00f2..c899ff8f18f 100644 --- a/libcontainer/cgroups/fs/fs_test.go +++ b/libcontainer/cgroups/fs/fs_test.go @@ -15,284 +15,80 @@ func TestInvalidCgroupPath(t *testing.T) { if cgroups.IsCgroup2UnifiedMode() { t.Skip("cgroup v2 is not supported") } - root, err := getCgroupRoot() - if err != nil { - t.Errorf("couldn't get cgroup root: %v", err) - } - - config := &configs.Cgroup{ - Path: "../../../../../../../../../../some/path", - } - - data, err := getCgroupData(config, 0) - if err != nil { - t.Errorf("couldn't get cgroup data: %v", err) - } - - // Make sure the final innerPath doesn't go outside the cgroup mountpoint. - if strings.HasPrefix(data.innerPath, "..") { - t.Errorf("SECURITY: cgroup innerPath is outside cgroup mountpoint!") - } - - // Double-check, using an actual cgroup. - deviceRoot := filepath.Join(root, "devices") - devicePath, err := data.path("devices") - if err != nil { - t.Errorf("couldn't get cgroup path: %v", err) - } - if !strings.HasPrefix(devicePath, deviceRoot) { - t.Errorf("SECURITY: cgroup path() is outside cgroup mountpoint!") - } -} - -func TestInvalidAbsoluteCgroupPath(t *testing.T) { - if cgroups.IsCgroup2UnifiedMode() { - t.Skip("cgroup v2 is not supported") - } - root, err := getCgroupRoot() - if err != nil { - t.Errorf("couldn't get cgroup root: %v", err) - } - - config := &configs.Cgroup{ - Path: "/../../../../../../../../../../some/path", - } - - data, err := getCgroupData(config, 0) - if err != nil { - t.Errorf("couldn't get cgroup data: %v", err) - } - - // Make sure the final innerPath doesn't go outside the cgroup mountpoint. - if strings.HasPrefix(data.innerPath, "..") { - t.Errorf("SECURITY: cgroup innerPath is outside cgroup mountpoint!") - } - - // Double-check, using an actual cgroup. - deviceRoot := filepath.Join(root, "devices") - devicePath, err := data.path("devices") - if err != nil { - t.Errorf("couldn't get cgroup path: %v", err) - } - if !strings.HasPrefix(devicePath, deviceRoot) { - t.Errorf("SECURITY: cgroup path() is outside cgroup mountpoint!") - } -} - -// XXX: Remove me after we get rid of configs.Cgroup.Name and configs.Cgroup.Parent. -func TestInvalidCgroupParent(t *testing.T) { - if cgroups.IsCgroup2UnifiedMode() { - t.Skip("cgroup v2 is not supported") - } - root, err := getCgroupRoot() - if err != nil { - t.Errorf("couldn't get cgroup root: %v", err) - } - - config := &configs.Cgroup{ - Parent: "../../../../../../../../../../some/path", - Name: "name", - } - - data, err := getCgroupData(config, 0) - if err != nil { - t.Errorf("couldn't get cgroup data: %v", err) - } - - // Make sure the final innerPath doesn't go outside the cgroup mountpoint. - if strings.HasPrefix(data.innerPath, "..") { - t.Errorf("SECURITY: cgroup innerPath is outside cgroup mountpoint!") - } - - // Double-check, using an actual cgroup. - deviceRoot := filepath.Join(root, "devices") - devicePath, err := data.path("devices") - if err != nil { - t.Errorf("couldn't get cgroup path: %v", err) - } - if !strings.HasPrefix(devicePath, deviceRoot) { - t.Errorf("SECURITY: cgroup path() is outside cgroup mountpoint!") - } -} - -// XXX: Remove me after we get rid of configs.Cgroup.Name and configs.Cgroup.Parent. -func TestInvalidAbsoluteCgroupParent(t *testing.T) { - if cgroups.IsCgroup2UnifiedMode() { - t.Skip("cgroup v2 is not supported") - } - root, err := getCgroupRoot() - if err != nil { - t.Errorf("couldn't get cgroup root: %v", err) - } - - config := &configs.Cgroup{ - Parent: "/../../../../../../../../../../some/path", - Name: "name", - } - - data, err := getCgroupData(config, 0) - if err != nil { - t.Errorf("couldn't get cgroup data: %v", err) - } - - // Make sure the final innerPath doesn't go outside the cgroup mountpoint. - if strings.HasPrefix(data.innerPath, "..") { - t.Errorf("SECURITY: cgroup innerPath is outside cgroup mountpoint!") - } - - // Double-check, using an actual cgroup. - deviceRoot := filepath.Join(root, "devices") - devicePath, err := data.path("devices") - if err != nil { - t.Errorf("couldn't get cgroup path: %v", err) - } - if !strings.HasPrefix(devicePath, deviceRoot) { - t.Errorf("SECURITY: cgroup path() is outside cgroup mountpoint!") - } -} -// XXX: Remove me after we get rid of configs.Cgroup.Name and configs.Cgroup.Parent. -func TestInvalidCgroupName(t *testing.T) { - if cgroups.IsCgroup2UnifiedMode() { - t.Skip("cgroup v2 is not supported") - } - root, err := getCgroupRoot() - if err != nil { - t.Errorf("couldn't get cgroup root: %v", err) - } - - config := &configs.Cgroup{ - Parent: "parent", - Name: "../../../../../../../../../../some/path", - } - - data, err := getCgroupData(config, 0) - if err != nil { - t.Errorf("couldn't get cgroup data: %v", err) - } - - // Make sure the final innerPath doesn't go outside the cgroup mountpoint. - if strings.HasPrefix(data.innerPath, "..") { - t.Errorf("SECURITY: cgroup innerPath is outside cgroup mountpoint!") - } - - // Double-check, using an actual cgroup. - deviceRoot := filepath.Join(root, "devices") - devicePath, err := data.path("devices") - if err != nil { - t.Errorf("couldn't get cgroup path: %v", err) - } - if !strings.HasPrefix(devicePath, deviceRoot) { - t.Errorf("SECURITY: cgroup path() is outside cgroup mountpoint!") - } - -} - -// XXX: Remove me after we get rid of configs.Cgroup.Name and configs.Cgroup.Parent. -func TestInvalidAbsoluteCgroupName(t *testing.T) { - if cgroups.IsCgroup2UnifiedMode() { - t.Skip("cgroup v2 is not supported") - } - root, err := getCgroupRoot() - if err != nil { - t.Errorf("couldn't get cgroup root: %v", err) - } - - config := &configs.Cgroup{ - Parent: "parent", - Name: "/../../../../../../../../../../some/path", - } - - data, err := getCgroupData(config, 0) - if err != nil { - t.Errorf("couldn't get cgroup data: %v", err) - } - - // Make sure the final innerPath doesn't go outside the cgroup mountpoint. - if strings.HasPrefix(data.innerPath, "..") { - t.Errorf("SECURITY: cgroup innerPath is outside cgroup mountpoint!") - } - - // Double-check, using an actual cgroup. - deviceRoot := filepath.Join(root, "devices") - devicePath, err := data.path("devices") - if err != nil { - t.Errorf("couldn't get cgroup path: %v", err) - } - if !strings.HasPrefix(devicePath, deviceRoot) { - t.Errorf("SECURITY: cgroup path() is outside cgroup mountpoint!") - } -} - -// XXX: Remove me after we get rid of configs.Cgroup.Name and configs.Cgroup.Parent. -func TestInvalidCgroupNameAndParent(t *testing.T) { - if cgroups.IsCgroup2UnifiedMode() { - t.Skip("cgroup v2 is not supported") - } - root, err := getCgroupRoot() - if err != nil { - t.Errorf("couldn't get cgroup root: %v", err) - } - - config := &configs.Cgroup{ - Parent: "../../../../../../../../../../some/path", - Name: "../../../../../../../../../../some/path", - } - - data, err := getCgroupData(config, 0) - if err != nil { - t.Errorf("couldn't get cgroup data: %v", err) - } - - // Make sure the final innerPath doesn't go outside the cgroup mountpoint. - if strings.HasPrefix(data.innerPath, "..") { - t.Errorf("SECURITY: cgroup innerPath is outside cgroup mountpoint!") - } - - // Double-check, using an actual cgroup. - deviceRoot := filepath.Join(root, "devices") - devicePath, err := data.path("devices") - if err != nil { - t.Errorf("couldn't get cgroup path: %v", err) - } - if !strings.HasPrefix(devicePath, deviceRoot) { - t.Errorf("SECURITY: cgroup path() is outside cgroup mountpoint!") - } -} - -// XXX: Remove me after we get rid of configs.Cgroup.Name and configs.Cgroup.Parent. -func TestInvalidAbsoluteCgroupNameAndParent(t *testing.T) { - if cgroups.IsCgroup2UnifiedMode() { - t.Skip("cgroup v2 is not supported") - } root, err := getCgroupRoot() if err != nil { - t.Errorf("couldn't get cgroup root: %v", err) - } - - config := &configs.Cgroup{ - Parent: "/../../../../../../../../../../some/path", - Name: "/../../../../../../../../../../some/path", - } - - data, err := getCgroupData(config, 0) - if err != nil { - t.Errorf("couldn't get cgroup data: %v", err) - } - - // Make sure the final innerPath doesn't go outside the cgroup mountpoint. - if strings.HasPrefix(data.innerPath, "..") { - t.Errorf("SECURITY: cgroup innerPath is outside cgroup mountpoint!") - } - - // Double-check, using an actual cgroup. - deviceRoot := filepath.Join(root, "devices") - devicePath, err := data.path("devices") - if err != nil { - t.Errorf("couldn't get cgroup path: %v", err) - } - if !strings.HasPrefix(devicePath, deviceRoot) { - t.Errorf("SECURITY: cgroup path() is outside cgroup mountpoint!") + t.Fatalf("couldn't get cgroup root: %v", err) + } + + testCases := []struct { + test string + path, name, parent string + }{ + { + test: "invalid cgroup path", + path: "../../../../../../../../../../some/path", + }, + { + test: "invalid absolute cgroup path", + path: "/../../../../../../../../../../some/path", + }, + { + test: "invalid cgroup parent", + parent: "../../../../../../../../../../some/path", + name: "name", + }, + { + test: "invalid absolute cgroup parent", + parent: "/../../../../../../../../../../some/path", + name: "name", + }, + { + test: "invalid cgroup name", + parent: "parent", + name: "../../../../../../../../../../some/path", + }, + { + test: "invalid absolute cgroup name", + parent: "parent", + name: "/../../../../../../../../../../some/path", + }, + { + test: "invalid cgroup name and parent", + parent: "../../../../../../../../../../some/path", + name: "../../../../../../../../../../some/path", + }, + { + test: "invalid absolute cgroup name and parent", + parent: "/../../../../../../../../../../some/path", + name: "/../../../../../../../../../../some/path", + }, + } + + for _, tc := range testCases { + t.Run(tc.test, func(t *testing.T) { + config := &configs.Cgroup{Path: tc.path, Name: tc.name, Parent: tc.parent} + + data, err := getCgroupData(config, 0) + if err != nil { + t.Fatalf("couldn't get cgroup data: %v", err) + } + + // Make sure the final innerPath doesn't go outside the cgroup mountpoint. + if strings.HasPrefix(data.innerPath, "..") { + t.Errorf("SECURITY: cgroup innerPath is outside cgroup mountpoint!") + } + + // Double-check, using an actual cgroup. + deviceRoot := filepath.Join(root, "devices") + devicePath, err := data.path("devices") + if err != nil { + t.Fatalf("couldn't get cgroup path: %v", err) + } + if !strings.HasPrefix(devicePath, deviceRoot) { + t.Errorf("SECURITY: cgroup path() is outside cgroup mountpoint!") + } + }) } }