Merge pull request #625 from kinvolk/dongsu/fix-readable-dir

runtimetest: correctly check for a readable directory

Author: Zhou Hao

cmd/runtimetest/main.go

@@ -36,6 +36,10 @@ var gitCommit = ""
 // VERSION file of the source code.
 var version = ""
 
+// errAccess will be used for defining either a read error or a write error
+// from any type of files.
+var errAccess error
+
 // PrGetNoNewPrivs isn't exposed in Golang so we define it ourselves copying the value from
 // the kernel
 const PrGetNoNewPrivs = 39
@@ -411,10 +415,37 @@ func testReadAccess(path string) (readable bool, err error) {
 	if err != nil {
 		return false, err
 	}
-	if fi.Mode()&os.ModeType == 0 {
+
+	// Check for readability in case of regular files, character device, or
+	// directory. Although the runtime spec does not mandate the type of
+	// masked files, we should check its Mode explicitly. A masked file
+	// could be represented as a character file (/dev/null), which is the
+	// case for runtimes like runc.
+	switch fi.Mode() & os.ModeType {
+	case 0, os.ModeDevice | os.ModeCharDevice:
 		return testFileReadAccess(path)
+	case os.ModeDir:
+		return testDirectoryReadAccess(path)
 	}
-	return false, fmt.Errorf("cannot test read access for %q (mode %d)", path, fi.Mode())
+
+	errAccess = fmt.Errorf("cannot test read access for %q (mode %d)", path, fi.Mode())
+	return false, errAccess
+}
+
+func testDirectoryReadAccess(path string) (readable bool, err error) {
+	files, err := ioutil.ReadDir(path)
+	if err == io.EOF || len(files) == 0 {
+		// Our validation/ tests only use non-empty directories for read-access
+		// tests. So if we get an EOF on the first read, the runtime did
+		// successfully block readability. So it should not be considered as test
+		// failure, it just means that the test program successfully assessed
+		// that the directory is not readable.
+		return false, nil
+	}
+	if err != nil {
+		return false, err
+	}
+	return true, nil
 }
 
 func testFileReadAccess(path string) (readable bool, err error) {
@@ -441,12 +472,20 @@ func testWriteAccess(path string) (writable bool, err error) {
 	if err != nil {
 		return false, err
 	}
-	if fi.IsDir() {
-		return testDirectoryWriteAccess(path)
-	} else if fi.Mode()&os.ModeType == 0 {
+
+	// Check for writability in case of regular files, character device, or
+	// directory. Although the runtime spec does not mandate the type of
+	// masked files, we should check its Mode explicitly. A masked file
+	// could be represented as a character file (/dev/null), which is the
+	// case for runtimes like runc.
+	switch fi.Mode() & os.ModeType {
+	case 0, os.ModeDevice | os.ModeCharDevice:
 		return testFileWriteAccess(path)
+	case os.ModeDir:
+		return testDirectoryWriteAccess(path)
 	}
-	return false, fmt.Errorf("cannot test write access for %q (mode %d)", path, fi.Mode())
+	errAccess = fmt.Errorf("cannot test write access for %q (mode %d)", path, fi.Mode())
+	return false, errAccess
 }
 
 func testDirectoryWriteAccess(path string) (writable bool, err error) {
@@ -858,7 +897,7 @@ func (c *complianceTester) validateMaskedPaths(spec *rspec.Spec) error {
 
 	for _, maskedPath := range spec.Linux.MaskedPaths {
 		readable, err := testReadAccess(maskedPath)
-		if err != nil && !os.IsNotExist(err) {
+		if err != nil && !os.IsNotExist(err) && err != errAccess {
 			return err
 		}
 		c.harness.Ok(!readable, fmt.Sprintf("cannot read masked path %q", maskedPath))
@@ -901,15 +940,15 @@ func (c *complianceTester) validateROPaths(spec *rspec.Spec) error {
 
 	for i, path := range spec.Linux.ReadonlyPaths {
 		readable, err := testReadAccess(path)
-		if err != nil {
+		if err != nil && err != errAccess {
 			return err
 		}
 		if !readable {
 			c.harness.Skip(1, fmt.Sprintf("%q (linux.readonlyPaths[%d]) is not readable", path, i))
 		}
 
 		writable, err := testWriteAccess(path)
-		if err != nil && !os.IsNotExist(err) {
+		if err != nil && !os.IsNotExist(err) && err != errAccess {
 			return err
 		}
 		c.harness.Ok(!writable, fmt.Sprintf("%q (linux.readonlyPaths[%d]) is not writable", path, i))

validation/linux_masked_paths.go

@@ -21,6 +21,12 @@ func main() {
 		if err != nil {
 			return err
 		}
+		// create a temp file to make testDir non-empty
+		tmpfile, err := ioutil.TempFile(testDir, "tmp")
+		if err != nil {
+			return err
+		}
+		defer os.Remove(tmpfile.Name())
 
 		testFile := filepath.Join(path, "masked-file")
 

validation/linux_readonly_paths.go

@@ -21,6 +21,12 @@ func main() {
 		if err != nil {
 			return err
 		}
+		// create a temp file to make testDir non-empty
+		tmpfile, err := ioutil.TempFile(testDir, "tmp")
+		if err != nil {
+			return err
+		}
+		defer os.Remove(tmpfile.Name())
 
 		testFile := filepath.Join(path, "readonly-file")