devices: correctly check device types

cyphar · cyphar · commit 95a59bf206f8 · 2020-07-28T19:04:30.000+10:00
(mode&S_IFCHR == S_IFCHR) is the wrong way of checking the type of an inode because the S_IF* bits are actually not a bitmask and instead must be checked using S_IF*. This bug was neatly hidden behind a (major == 0) sanity-check but that was removed by [1]. In addition, add a test that makes sure that HostDevices() doesn't give rubbish results -- because we broke this and fixed this before[2]. [1]: 24388be ("configs: use different types for .Devices and .Resources.Devices") [2]: 3ed492a ("Handle non-devices correctly in DeviceFromPath") Fixes: b0d014d ("libcontainer: one more switch from syscall to x/sys/unix") Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
diff --git a/libcontainer/devices/devices.go b/libcontainer/devices/devices.go
@@ -37,12 +37,12 @@ func DeviceFromPath(path, permissions string) (*configs.Device, error) {
 		major     = unix.Major(devNumber)
 		minor     = unix.Minor(devNumber)
 	)
-	switch {
-	case mode&unix.S_IFBLK == unix.S_IFBLK:
+	switch mode & unix.S_IFMT {
+	case unix.S_IFBLK:
 		devType = configs.BlockDevice
-	case mode&unix.S_IFCHR == unix.S_IFCHR:
+	case unix.S_IFCHR:
 		devType = configs.CharDevice
-	case mode&unix.S_IFIFO == unix.S_IFIFO:
+	case unix.S_IFIFO:
 		devType = configs.FifoDevice
 	default:
 		return nil, ErrNotADevice
@@ -104,6 +104,9 @@ func GetDevices(path string) ([]*configs.Device, error) {
 			}
 			return nil, err
 		}
+		if device.Type == configs.FifoDevice {
+			continue
+		}
 		out = append(out, device)
 	}
 	return out, nil
diff --git a/libcontainer/devices/devices_test.go b/libcontainer/devices/devices_test.go
@@ -2,19 +2,27 @@ package devices
 
 import (
 	"errors"
+	"io/ioutil"
 	"os"
 	"testing"
 
+	"github.com/opencontainers/runc/libcontainer/configs"
 	"golang.org/x/sys/unix"
 )
 
+func cleanupTest() {
+	unixLstat = unix.Lstat
+	ioutilReadDir = ioutil.ReadDir
+}
+
 func TestDeviceFromPathLstatFailure(t *testing.T) {
 	testError := errors.New("test error")
 
 	// Override unix.Lstat to inject error.
 	unixLstat = func(path string, stat *unix.Stat_t) error {
 		return testError
 	}
+	defer cleanupTest()
 
 	_, err := DeviceFromPath("", "")
 	if err != testError {
@@ -29,6 +37,7 @@ func TestHostDevicesIoutilReadDirFailure(t *testing.T) {
 	ioutilReadDir = func(dirname string) ([]os.FileInfo, error) {
 		return nil, testError
 	}
+	defer cleanupTest()
 
 	_, err := HostDevices()
 	if err != testError {
@@ -55,9 +64,41 @@ func TestHostDevicesIoutilReadDirDeepFailure(t *testing.T) {
 
 		return []os.FileInfo{fi}, nil
 	}
+	defer cleanupTest()
 
 	_, err := HostDevices()
 	if err != testError {
 		t.Fatalf("Unexpected error %v, expected %v", err, testError)
 	}
 }
+
+func TestHostDevicesAllValid(t *testing.T) {
+	devices, err := HostDevices()
+	if err != nil {
+		t.Fatalf("failed to get host devices: %v", err)
+	}
+
+	for _, device := range devices {
+		// Devices can't have major number 0.
+		if device.Major == 0 {
+			t.Errorf("device entry %+v has zero major number", device)
+		}
+		// Devices should only have file modes that correspond to their type.
+		var expectedType os.FileMode
+		switch device.Type {
+		case configs.BlockDevice:
+			expectedType = unix.S_IFBLK
+		case configs.CharDevice:
+			expectedType = unix.S_IFCHR
+		case configs.FifoDevice:
+			t.Logf("fifo devices shouldn't show up from HostDevices")
+			fallthrough
+		default:
+			t.Errorf("device entry %+v has unexpected type %v", device, device.Type)
+		}
+		gotType := device.FileMode & unix.S_IFMT
+		if expectedType != gotType {
+			t.Errorf("device entry %+v has mismatched types (expected %#x, got %#x)", device, expectedType, gotType)
+		}
+	}
+}
