Repository: opencontainers/runc
Pull request #2635 (2020-10-06 00:06:00)

libct/cg: introduce and use fscommon.OpenFile
_This is part III from https://github.com/opencontainers/runc/pull/2598, preparing the code to use `openat2()`_

Move the functionality of opening a cgroup file into a separate
function, `OpenFile`, which, similar to `ReadFile` and `WriteFile`,
use separate dir and file arguments.
    
Change `ReadFile` and `WriteFile` to rely on `OpenFile`, and use lower-level
read and write instead of ones from ioutil.
    
It changes the semantics of `WriteFile` a bit -- it no longer uses
`O_CREAT` flag. This is good for real cgroup as there is no need to try
creating the files in there, but can potentially break WriteFile users
-- previously, `EPERM` error was returned for non-existing files, and
now it's `ENOENT`.
    
This also breaks the fs/fs2 unit tests since they write to pseudo-cgroup
files inside a test directory (not to a real cgroup fs), and now
`fscommon.WriteFile` do not create or truncate files, so we have to add a
variable that is set by the unit tests to emulate the old behavior.

Use the new interface where possible.



Patch:
diff --git a/libcontainer/cgroups/fs/blkio.go b/libcontainer/cgroups/fs/blkio.go
index 031a6bbd..4d5e5beb 100644
--- a/libcontainer/cgroups/fs/blkio.go
+++ b/libcontainer/cgroups/fs/blkio.go
@@ -6,7 +6,6 @@ import (
 	"bufio"
 	"fmt"
 	"os"
-	"path/filepath"
 	"strconv"
 	"strings"
 
@@ -105,9 +104,9 @@ func splitBlkioStatLine(r rune) bool {
 	return r == ' ' || r == ':'
 }
 
-func getBlkioStat(path string) ([]cgroups.BlkioStatEntry, error) {
+func getBlkioStat(dir, file string) ([]cgroups.BlkioStatEntry, error) {
 	var blkioStats []cgroups.BlkioStatEntry
-	f, err := os.Open(path)
+	f, err := fscommon.OpenFile(dir, file, os.O_RDONLY)
 	if err != nil {
 		if os.IsNotExist(err) {
 			return blkioStats, nil
@@ -125,7 +124,7 @@ func getBlkioStat(path string) ([]cgroups.BlkioStatEntry, error) {
 				// skip total line
 				continue
 			} else {
-				return nil, fmt.Errorf("Invalid line found while parsing %s: %s", path, sc.Text())
+				return nil, fmt.Errorf("Invalid line found while parsing %s/%s: %s", dir, file, sc.Text())
 			}
 		}
 
@@ -159,7 +158,7 @@ func getBlkioStat(path string) ([]cgroups.BlkioStatEntry, error) {
 
 func (s *BlkioGroup) GetStats(path string, stats *cgroups.Stats) error {
 	// Try to read CFQ stats available on all CFQ enabled kernels first
-	if blkioStats, err := getBlkioStat(filepath.Join(path, "blkio.io_serviced_recursive")); err == nil && blkioStats != nil {
+	if blkioStats, err := getBlkioStat(path, "blkio.io_serviced_recursive"); err == nil && blkioStats != nil {
 		return getCFQStats(path, stats)
 	}
 	return getStats(path, stats) // Use generic stats as fallback
@@ -169,42 +168,42 @@ func getCFQStats(path string, stats *cgroups.Stats) error {
 	var blkioStats []cgroups.BlkioStatEntry
 	var err error
 
-	if blkioStats, err = getBlkioStat(filepath.Join(path, "blkio.sectors_recursive")); err != nil {
+	if blkioStats, err = getBlkioStat(path, "blkio.sectors_recursive"); err != nil {
 		return err
 	}
 	stats.BlkioStats.SectorsRecursive = blkioStats
 
-	if blkioStats, err = getBlkioStat(filepath.Join(path, "blkio.io_service_bytes_recursive")); err != nil {
+	if blkioStats, err = getBlkioStat(path, "blkio.io_service_bytes_recursive"); err != nil {
 		return err
 	}
 	stats.BlkioStats.IoServiceBytesRecursive = blkioStats
 
-	if blkioStats, err = getBlkioStat(filepath.Join(path, "blkio.io_serviced_recursive")); err != nil {
+	if blkioStats, err = getBlkioStat(path, "blkio.io_serviced_recursive"); err != nil {
 		return err
 	}
 	stats.BlkioStats.IoServicedRecursive = blkioStats
 
-	if blkioStats, err = getBlkioStat(filepath.Join(path, "blkio.io_queued_recursive")); err != nil {
+	if blkioStats, err = getBlkioStat(path, "blkio.io_queued_recursive"); err != nil {
 		return err
 	}
 	stats.BlkioStats.IoQueuedRecursive = blkioStats
 
-	if blkioStats, err = getBlkioStat(filepath.Join(path, "blkio.io_service_time_recursive")); err != nil {
+	if blkioStats, err = getBlkioStat(path, "blkio.io_service_time_recursive"); err != nil {
 		return err
 	}
 	stats.BlkioStats.IoServiceTimeRecursive = blkioStats
 
-	if blkioStats, err = getBlkioStat(filepath.Join(path, "blkio.io_wait_time_recursive")); err != nil {
+	if blkioStats, err = getBlkioStat(path, "blkio.io_wait_time_recursive"); err != nil {
 		return err
 	}
 	stats.BlkioStats.IoWaitTimeRecursive = blkioStats
 
-	if blkioStats, err = getBlkioStat(filepath.Join(path, "blkio.io_merged_recursive")); err != nil {
+	if blkioStats, err = getBlkioStat(path, "blkio.io_merged_recursive"); err != nil {
 		return err
 	}
 	stats.BlkioStats.IoMergedRecursive = blkioStats
 
-	if blkioStats, err = getBlkioStat(filepath.Join(path, "blkio.time_recursive")); err != nil {
+	if blkioStats, err = getBlkioStat(path, "blkio.time_recursive"); err != nil {
 		return err
 	}
 	stats.BlkioStats.IoTimeRecursive = blkioStats
@@ -216,12 +215,12 @@ func getStats(path string, stats *cgroups.Stats) error {
 	var blkioStats []cgroups.BlkioStatEntry
 	var err error
 
-	if blkioStats, err = getBlkioStat(filepath.Join(path, "blkio.throttle.io_service_bytes")); err != nil {
+	if blkioStats, err = getBlkioStat(path, "blkio.throttle.io_service_bytes"); err != nil {
 		return err
 	}
 	stats.BlkioStats.IoServiceBytesRecursive = blkioStats
 
-	if blkioStats, err = getBlkioStat(filepath.Join(path, "blkio.throttle.io_serviced")); err != nil {
+	if blkioStats, err = getBlkioStat(path, "blkio.throttle.io_serviced"); err != nil {
 		return err
 	}
 	stats.BlkioStats.IoServicedRecursive = blkioStats
diff --git a/libcontainer/cgroups/fs/cpu.go b/libcontainer/cgroups/fs/cpu.go
index 6fb1c207..1d5f455d 100644
--- a/libcontainer/cgroups/fs/cpu.go
+++ b/libcontainer/cgroups/fs/cpu.go
@@ -6,7 +6,6 @@ import (
 	"bufio"
 	"fmt"
 	"os"
-	"path/filepath"
 	"strconv"
 
 	"github.com/opencontainers/runc/libcontainer/cgroups"
@@ -87,7 +86,7 @@ func (s *CpuGroup) Set(path string, cgroup *configs.Cgroup) error {
 }
 
 func (s *CpuGroup) GetStats(path string, stats *cgroups.Stats) error {
-	f, err := os.Open(filepath.Join(path, "cpu.stat"))
+	f, err := fscommon.OpenFile(path, "cpu.stat", os.O_RDONLY)
 	if err != nil {
 		if os.IsNotExist(err) {
 			return nil
diff --git a/libcontainer/cgroups/fs/cpuacct.go b/libcontainer/cgroups/fs/cpuacct.go
index 9ee1119c..129de6a0 100644
--- a/libcontainer/cgroups/fs/cpuacct.go
+++ b/libcontainer/cgroups/fs/cpuacct.go
@@ -135,7 +135,7 @@ func getPercpuUsageInModes(path string) ([]uint64, []uint64, error) {
 	usageKernelMode := []uint64{}
 	usageUserMode := []uint64{}
 
-	file, err := os.Open(filepath.Join(path, cgroupCpuacctUsageAll))
+	file, err := fscommon.OpenFile(path, cgroupCpuacctUsageAll, os.O_RDONLY)
 	if os.IsNotExist(err) {
 		return usageKernelMode, usageUserMode, nil
 	} else if err != nil {
diff --git a/libcontainer/cgroups/fs/memory.go b/libcontainer/cgroups/fs/memory.go
index 2a2f0dd6..7fde663d 100644
--- a/libcontainer/cgroups/fs/memory.go
+++ b/libcontainer/cgroups/fs/memory.go
@@ -7,7 +7,6 @@ import (
 	"fmt"
 	"math"
 	"os"
-	"path"
 	"path/filepath"
 	"strconv"
 	"strings"
@@ -160,7 +159,7 @@ func (s *MemoryGroup) Set(path string, cgroup *configs.Cgroup) error {
 
 func (s *MemoryGroup) GetStats(path string, stats *cgroups.Stats) error {
 	// Set stats from memory.stat.
-	statsFile, err := os.Open(filepath.Join(path, "memory.stat"))
+	statsFile, err := fscommon.OpenFile(path, "memory.stat", os.O_RDONLY)
 	if err != nil {
 		if os.IsNotExist(err) {
 			return nil
@@ -280,7 +279,7 @@ func getMemoryData(path, name string) (cgroups.MemoryData, error) {
 func getPageUsageByNUMA(cgroupPath string) (cgroups.PageUsageByNUMA, error) {
 	stats := cgroups.PageUsageByNUMA{}
 
-	file, err := os.Open(path.Join(cgroupPath, cgroupMemoryPagesByNuma))
+	file, err := fscommon.OpenFile(cgroupPath, cgroupMemoryPagesByNuma, os.O_RDONLY)
 	if os.IsNotExist(err) {
 		return stats, nil
 	} else if err != nil {
diff --git a/libcontainer/cgroups/fs2/cpu.go b/libcontainer/cgroups/fs2/cpu.go
index edf1060e..0dffe664 100644
--- a/libcontainer/cgroups/fs2/cpu.go
+++ b/libcontainer/cgroups/fs2/cpu.go
@@ -5,7 +5,6 @@ package fs2
 import (
 	"bufio"
 	"os"
-	"path/filepath"
 	"strconv"
 
 	"github.com/opencontainers/runc/libcontainer/cgroups"
@@ -50,7 +49,7 @@ func setCpu(dirPath string, cgroup *configs.Cgroup) error {
 	return nil
 }
 func statCpu(dirPath string, stats *cgroups.Stats) error {
-	f, err := os.Open(filepath.Join(dirPath, "cpu.stat"))
+	f, err := fscommon.OpenFile(dirPath, "cpu.stat", os.O_RDONLY)
 	if err != nil {
 		return err
 	}
diff --git a/libcontainer/cgroups/fs2/io.go b/libcontainer/cgroups/fs2/io.go
index bbe3ac06..e01ea001 100644
--- a/libcontainer/cgroups/fs2/io.go
+++ b/libcontainer/cgroups/fs2/io.go
@@ -5,7 +5,6 @@ package fs2
 import (
 	"bufio"
 	"os"
-	"path/filepath"
 	"strconv"
 	"strings"
 
@@ -60,8 +59,7 @@ func setIo(dirPath string, cgroup *configs.Cgroup) error {
 
 func readCgroup2MapFile(dirPath string, name string) (map[string][]string, error) {
 	ret := map[string][]string{}
-	p := filepath.Join(dirPath, name)
-	f, err := os.Open(p)
+	f, err := fscommon.OpenFile(dirPath, name, os.O_RDONLY)
 	if err != nil {
 		return nil, err
 	}
diff --git a/libcontainer/cgroups/fs2/memory.go b/libcontainer/cgroups/fs2/memory.go
index 68f2546c..1c6913bf 100644
--- a/libcontainer/cgroups/fs2/memory.go
+++ b/libcontainer/cgroups/fs2/memory.go
@@ -5,7 +5,6 @@ package fs2
 import (
 	"bufio"
 	"os"
-	"path/filepath"
 	"strconv"
 
 	"github.com/opencontainers/runc/libcontainer/cgroups"
@@ -75,7 +74,7 @@ func setMemory(dirPath string, cgroup *configs.Cgroup) error {
 
 func statMemory(dirPath string, stats *cgroups.Stats) error {
 	// Set stats from memory.stat.
-	statsFile, err := os.Open(filepath.Join(dirPath, "memory.stat"))
+	statsFile, err := fscommon.OpenFile(dirPath, "memory.stat", os.O_RDONLY)
 	if err != nil {
 		return err
 	}
diff --git a/libcontainer/cgroups/fscommon/fscommon.go b/libcontainer/cgroups/fscommon/fscommon.go
index 8f7298c8..d19673f7 100644
--- a/libcontainer/cgroups/fscommon/fscommon.go
+++ b/libcontainer/cgroups/fscommon/fscommon.go
@@ -3,10 +3,9 @@
 package fscommon
 
 import (
-	"io/ioutil"
+	"bytes"
 	"os"
 
-	securejoin "github.com/cyphar/filepath-securejoin"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
 	"golang.org/x/sys/unix"
@@ -15,14 +14,12 @@ import (
 // WriteFile writes data to a cgroup file in dir.
 // It is supposed to be used for cgroup files only.
 func WriteFile(dir, file, data string) error {
-	if dir == "" {
-		return errors.Errorf("no directory specified for %s", file)
-	}
-	path, err := securejoin.SecureJoin(dir, file)
+	fd, err := OpenFile(dir, file, unix.O_WRONLY)
 	if err != nil {
 		return err
 	}
-	if err := retryingWriteFile(path, []byte(data), 0700); err != nil {
+	defer fd.Close()
+	if err := retryingWriteFile(fd, data); err != nil {
 		return errors.Wrapf(err, "failed to write %q", data)
 	}
 	return nil
@@ -31,22 +28,21 @@ func WriteFile(dir, file, data string) error {
 // ReadFile reads data from a cgroup file in dir.
 // It is supposed to be used for cgroup files only.
 func ReadFile(dir, file string) (string, error) {
-	if dir == "" {
-		return "", errors.Errorf("no directory specified for %s", file)
-	}
-	path, err := securejoin.SecureJoin(dir, file)
+	fd, err := OpenFile(dir, file, unix.O_RDONLY)
 	if err != nil {
 		return "", err
 	}
-	data, err := ioutil.ReadFile(path)
-	return string(data), err
+	var buf bytes.Buffer
+
+	_, err = buf.ReadFrom(fd)
+	return buf.String(), err
 }
 
-func retryingWriteFile(filename string, data []byte, perm os.FileMode) error {
+func retryingWriteFile(fd *os.File, data string) error {
 	for {
-		err := ioutil.WriteFile(filename, data, perm)
+		_, err := fd.Write([]byte(data))
 		if errors.Is(err, unix.EINTR) {
-			logrus.Infof("interrupted while writing %s to %s", string(data), filename)
+			logrus.Infof("interrupted while writing %s to %s", data, fd.Name())
 			continue
 		}
 		return err
diff --git a/libcontainer/cgroups/fscommon/open.go b/libcontainer/cgroups/fscommon/open.go
new file mode 100644
index 00000000..90560f54
--- /dev/null
+++ b/libcontainer/cgroups/fscommon/open.go
@@ -0,0 +1,33 @@
+package fscommon
+
+import (
+	"os"
+
+	securejoin "github.com/cyphar/filepath-securejoin"
+	"github.com/pkg/errors"
+)
+
+var (
+	// Set to true by fs unit tests
+	TestMode bool
+)
+
+// OpenFile opens a cgroup file in a given dir with given flags.
+// It is supposed to be used for cgroup files only.
+func OpenFile(dir, file string, flags int) (*os.File, error) {
+	if dir == "" {
+		return nil, errors.Errorf("no directory specified for %s", file)
+	}
+	mode := os.FileMode(0)
+	if TestMode && flags&os.O_WRONLY != 0 {
+		// "emulate" cgroup fs for unit tests
+		flags |= os.O_TRUNC | os.O_CREATE
+		mode = 0o600
+	}
+	path, err := securejoin.SecureJoin(dir, file)
+	if err != nil {
+		return nil, err
+	}
+
+	return os.OpenFile(path, flags, mode)
+}
diff --git a/libcontainer/cgroups/utils.go b/libcontainer/cgroups/utils.go
index c26be8cd..6329e605 100644
--- a/libcontainer/cgroups/utils.go
+++ b/libcontainer/cgroups/utils.go
@@ -367,14 +367,14 @@ func WriteCgroupProc(dir string, pid int) error {
 		return nil
 	}
 
-	cgroupProcessesFile, err := os.OpenFile(filepath.Join(dir, CgroupProcesses), os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0700)
+	file, err := fscommon.OpenFile(dir, CgroupProcesses, os.O_WRONLY)
 	if err != nil {
 		return fmt.Errorf("failed to write %v to %v: %v", pid, CgroupProcesses, err)
 	}
-	defer cgroupProcessesFile.Close()
+	defer file.Close()
 
 	for i := 0; i < 5; i++ {
-		_, err = cgroupProcessesFile.WriteString(strconv.Itoa(pid))
+		_, err = file.WriteString(strconv.Itoa(pid))
 		if err == nil {
 			return nil
 		}


Test patch:
diff --git a/libcontainer/cgroups/fs/util_test.go b/libcontainer/cgroups/fs/util_test.go
index 2c50d6f3..32cbdfc9 100644
--- a/libcontainer/cgroups/fs/util_test.go
+++ b/libcontainer/cgroups/fs/util_test.go
@@ -17,6 +17,10 @@ import (
 	"github.com/opencontainers/runc/libcontainer/configs"
 )
 
+func init() {
+	fscommon.TestMode = true
+}
+
 type cgroupTestUtil struct {
 	// cgroup data to use in tests.
 	CgroupData *cgroupData