Merge pull request #4633 from kolyshkin/libct-cg-sep

libct/cg: eliminate remaining libct deps

Author: lifubang

libcontainer/cgroups/devices/devicefilter_test.go

@@ -5,7 +5,6 @@ import (
 	"testing"
 
 	devices "github.com/opencontainers/runc/libcontainer/cgroups/devices/config"
-	"github.com/opencontainers/runc/libcontainer/specconv"
 )
 
 func hash(s, comm string) string {
@@ -53,6 +52,88 @@ block-0:
 }
 
 func TestDeviceFilter_BuiltInAllowList(t *testing.T) {
+	// This is a copy of all rules from
+	// github.com/opencontainers/runc/libcontainer/specconv.AllowedDevices.
+	devices := []*devices.Rule{
+		{
+			Type:        devices.CharDevice,
+			Major:       devices.Wildcard,
+			Minor:       devices.Wildcard,
+			Permissions: "m",
+			Allow:       true,
+		},
+		{
+			Type:        devices.BlockDevice,
+			Major:       devices.Wildcard,
+			Minor:       devices.Wildcard,
+			Permissions: "m",
+			Allow:       true,
+		},
+		{
+			Type:        devices.CharDevice,
+			Major:       1,
+			Minor:       3,
+			Permissions: "rwm",
+			Allow:       true,
+		},
+		{
+			Type:        devices.CharDevice,
+			Major:       1,
+			Minor:       8,
+			Permissions: "rwm",
+			Allow:       true,
+		},
+		{
+			Type:        devices.CharDevice,
+			Major:       1,
+			Minor:       7,
+			Permissions: "rwm",
+			Allow:       true,
+		},
+		{
+			Type:        devices.CharDevice,
+			Major:       5,
+			Minor:       0,
+			Permissions: "rwm",
+			Allow:       true,
+		},
+		{
+			Type:        devices.CharDevice,
+			Major:       1,
+			Minor:       5,
+			Permissions: "rwm",
+			Allow:       true,
+		},
+		{
+			Type:        devices.CharDevice,
+			Major:       1,
+			Minor:       9,
+			Permissions: "rwm",
+			Allow:       true,
+		},
+		{
+			Type:        devices.CharDevice,
+			Major:       136,
+			Minor:       devices.Wildcard,
+			Permissions: "rwm",
+			Allow:       true,
+		},
+		{
+			Type:        devices.CharDevice,
+			Major:       5,
+			Minor:       2,
+			Permissions: "rwm",
+			Allow:       true,
+		},
+		{
+			Type:        devices.CharDevice,
+			Major:       10,
+			Minor:       200,
+			Permissions: "rwm",
+			Allow:       true,
+		},
+	}
+
 	expected := `
 // load parameters into registers
         0: LdXMemW dst: r2 src: r1 off: 0 imm: 0
@@ -136,10 +217,6 @@ block-11:
         62: MovImm32 dst: r0 imm: 0
         63: Exit
 `
-	var devices []*devices.Rule
-	for _, device := range specconv.AllowedDevices {
-		devices = append(devices, &device.Rule)
-	}
 	testDeviceFilter(t, devices, expected)
 }
 

libcontainer/cgroups/file.go

@@ -5,12 +5,11 @@ import (
 	"errors"
 	"fmt"
 	"os"
-	"path"
+	"path/filepath"
 	"strconv"
 	"strings"
 	"sync"
 
-	"github.com/opencontainers/runc/libcontainer/utils"
 	"github.com/sirupsen/logrus"
 	"golang.org/x/sys/unix"
 )
@@ -147,7 +146,10 @@ func openFile(dir, file string, flags int) (*os.File, error) {
 		flags |= os.O_TRUNC | os.O_CREATE
 		mode = 0o600
 	}
-	path := path.Join(dir, utils.CleanPath(file))
+	// NOTE it is important to use filepath.Clean("/"+file) here
+	// (see https://github.com/opencontainers/runc/issues/4103)!
+	path := filepath.Join(dir, filepath.Clean("/"+file))
+
 	if prepareOpenat2() != nil {
 		return openFallback(path, flags, mode)
 	}
@@ -171,10 +173,8 @@ func openFile(dir, file string, flags int) (*os.File, error) {
 		//
 		// TODO: if such usage will ever be common, amend this
 		// to reopen cgroupRootHandle and retry openat2.
-		fdPath, closer := utils.ProcThreadSelf("fd/" + strconv.Itoa(int(cgroupRootHandle.Fd())))
-		defer closer()
-		fdDest, _ := os.Readlink(fdPath)
-		if fdDest != cgroupfsDir {
+		fdDest, fdErr := os.Readlink("/proc/thread-self/fd/" + strconv.Itoa(int(cgroupRootHandle.Fd())))
+		if fdErr == nil && fdDest != cgroupfsDir {
 			// Wrap the error so it is clear that cgroupRootHandle
 			// is opened to an unexpected/wrong directory.
 			err = fmt.Errorf("cgroupRootHandle %d unexpectedly opened to %s != %s: %w",

libcontainer/cgroups/fs/paths.go

@@ -9,7 +9,7 @@ import (
 	"golang.org/x/sys/unix"
 
 	"github.com/opencontainers/runc/libcontainer/cgroups"
-	"github.com/opencontainers/runc/libcontainer/utils"
+	"github.com/opencontainers/runc/libcontainer/cgroups/internal/path"
 )
 
 // The absolute path to the root of the cgroup hierarchies.
@@ -26,7 +26,7 @@ func initPaths(cg *cgroups.Cgroup) (map[string]string, error) {
 		return nil, err
 	}
 
-	inner, err := innerPath(cg)
+	inner, err := path.Inner(cg)
 	if err != nil {
 		return nil, err
 	}
@@ -135,22 +135,6 @@ func rootPath() (string, error) {
 	return cgroupRoot, nil
 }
 
-func innerPath(c *cgroups.Cgroup) (string, error) {
-	if (c.Name != "" || c.Parent != "") && c.Path != "" {
-		return "", errors.New("cgroup: either Path or Name and Parent should be used")
-	}
-
-	// XXX: Do not remove CleanPath. Path safety is important! -- cyphar
-	innerPath := utils.CleanPath(c.Path)
-	if innerPath == "" {
-		cgParent := utils.CleanPath(c.Parent)
-		cgName := utils.CleanPath(c.Name)
-		innerPath = filepath.Join(cgParent, cgName)
-	}
-
-	return innerPath, nil
-}
-
 func subsysPath(root, inner, subsystem string) (string, error) {
 	// If the cgroup name/path is absolute do not look relative to the cgroup of the init process.
 	if filepath.IsAbs(inner) {

libcontainer/cgroups/fs/paths_test.go

@@ -6,6 +6,7 @@ import (
 	"testing"
 
 	"github.com/opencontainers/runc/libcontainer/cgroups"
+	"github.com/opencontainers/runc/libcontainer/cgroups/internal/path"
 )
 
 func TestInvalidCgroupPath(t *testing.T) {
@@ -66,7 +67,7 @@ func TestInvalidCgroupPath(t *testing.T) {
 		t.Run(tc.test, func(t *testing.T) {
 			config := &cgroups.Cgroup{Path: tc.path, Name: tc.name, Parent: tc.parent}
 
-			inner, err := innerPath(config)
+			inner, err := path.Inner(config)
 			if err != nil {
 				t.Fatalf("couldn't get cgroup data: %v", err)
 			}

libcontainer/cgroups/fs2/defaultpath.go

@@ -19,40 +19,25 @@ package fs2
 import (
 	"bufio"
 	"errors"
-	"fmt"
 	"io"
 	"os"
 	"path/filepath"
 	"strings"
 
 	"github.com/opencontainers/runc/libcontainer/cgroups"
-	"github.com/opencontainers/runc/libcontainer/utils"
+	"github.com/opencontainers/runc/libcontainer/cgroups/internal/path"
 )
 
 const UnifiedMountpoint = "/sys/fs/cgroup"
 
 func defaultDirPath(c *cgroups.Cgroup) (string, error) {
-	if (c.Name != "" || c.Parent != "") && c.Path != "" {
-		return "", fmt.Errorf("cgroup: either Path or Name and Parent should be used, got %+v", c)
-	}
-
-	return _defaultDirPath(UnifiedMountpoint, c.Path, c.Parent, c.Name)
-}
-
-func _defaultDirPath(root, cgPath, cgParent, cgName string) (string, error) {
-	if (cgName != "" || cgParent != "") && cgPath != "" {
-		return "", errors.New("cgroup: either Path or Name and Parent should be used")
+	innerPath, err := path.Inner(c)
+	if err != nil {
+		return "", err
 	}
 
-	// XXX: Do not remove CleanPath. Path safety is important! -- cyphar
-	innerPath := utils.CleanPath(cgPath)
-	if innerPath == "" {
-		cgParent := utils.CleanPath(cgParent)
-		cgName := utils.CleanPath(cgName)
-		innerPath = filepath.Join(cgParent, cgName)
-	}
 	if filepath.IsAbs(innerPath) {
-		return filepath.Join(root, innerPath), nil
+		return filepath.Join(UnifiedMountpoint, innerPath), nil
 	}
 
 	// we don't need to use /proc/thread-self here because runc always runs
@@ -67,7 +52,7 @@ func _defaultDirPath(root, cgPath, cgParent, cgName string) (string, error) {
 	// A parent cgroup (with no tasks in it) is what we need.
 	ownCgroup = filepath.Dir(ownCgroup)
 
-	return filepath.Join(root, ownCgroup, innerPath), nil
+	return filepath.Join(UnifiedMountpoint, ownCgroup, innerPath), nil
 }
 
 // parseCgroupFile parses /proc/PID/cgroup file and return string

libcontainer/cgroups/fs2/defaultpath_test.go

@@ -76,7 +76,13 @@ func TestDefaultDirPath(t *testing.T) {
 		},
 	}
 	for _, c := range cases {
-		got, err := _defaultDirPath(UnifiedMountpoint, c.cgPath, c.cgParent, c.cgName)
+		cg := &cgroups.Cgroup{
+			Path:   c.cgPath,
+			Parent: c.cgParent,
+			Name:   c.cgName,
+		}
+
+		got, err := defaultDirPath(cg)
 		if err != nil {
 			t.Fatal(err)
 		}

libcontainer/cgroups/internal/path/path.go

@@ -0,0 +1,52 @@
+package path
+
+import (
+	"errors"
+	"os"
+	"path/filepath"
+
+	"github.com/opencontainers/runc/libcontainer/cgroups"
+)
+
+// Inner returns a path to cgroup relative to a cgroup mount point, based
+// on cgroup configuration, or an error, if cgroup configuration is invalid.
+// To be used only by fs cgroup managers (systemd has different path rules).
+func Inner(c *cgroups.Cgroup) (string, error) {
+	if (c.Name != "" || c.Parent != "") && c.Path != "" {
+		return "", errors.New("cgroup: either Path or Name and Parent should be used")
+	}
+
+	// XXX: Do not remove cleanPath. Path safety is important! -- cyphar
+	innerPath := cleanPath(c.Path)
+	if innerPath == "" {
+		cgParent := cleanPath(c.Parent)
+		cgName := cleanPath(c.Name)
+		innerPath = filepath.Join(cgParent, cgName)
+	}
+
+	return innerPath, nil
+}
+
+// cleanPath is a copy of github.com/opencontainers/runc/libcontainer/utils.CleanPath.
+func cleanPath(path string) string {
+	// Deal with empty strings nicely.
+	if path == "" {
+		return ""
+	}
+
+	// Ensure that all paths are cleaned (especially problematic ones like
+	// "/../../../../../" which can cause lots of issues).
+
+	if filepath.IsAbs(path) {
+		return filepath.Clean(path)
+	}
+
+	// If the path isn't absolute, we need to do more processing to fix paths
+	// such as "../../../../<etc>/some/path". We also shouldn't convert absolute
+	// paths to relative ones.
+	path = filepath.Clean(string(os.PathSeparator) + path)
+	// This can't fail, as (by definition) all paths are relative to root.
+	path, _ = filepath.Rel(string(os.PathSeparator), path)
+
+	return path
+}