Safely use unsafe.Pointer to avoid crashes on ARM (#8027)

Refactor the Dirent parsing code such that when we
calculate offsets are correct based on the platform
This PR fixes a silent potential crash on ARM
architecture.

cmd/posix-dirent_fileino.go

@@ -1,7 +1,7 @@
-// +build darwin linux
+// +build freebsd openbsd netbsd
 
 /*
- * MinIO Cloud Storage, (C) 2016 MinIO, Inc.
+ * MinIO Cloud Storage, (C) 2019 MinIO, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -20,7 +20,6 @@ package cmd
 
 import "syscall"
 
-// True if dirent is absent in directory.
-func isEmptyDirent(dirent *syscall.Dirent) bool {
-	return dirent.Ino == 0
+func direntInode(dirent *syscall.Dirent) uint64 {
+	return uint64(dirent.Fileno)
 }

cmd/posix-dirent_ino.go

@@ -1,7 +1,8 @@
-// +build openbsd netbsd freebsd dragonfly
+// +build linux darwin
+// +build !appengine
 
 /*
- * MinIO Cloud Storage, (C) 2016 MinIO, Inc.
+ * MinIO Cloud Storage, (C) 2019 MinIO, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -20,7 +21,6 @@ package cmd
 
 import "syscall"
 
-// True if dirent is absent in directory.
-func isEmptyDirent(dirent *syscall.Dirent) bool {
-	return dirent.Fileno == 0
+func direntInode(dirent *syscall.Dirent) uint64 {
+	return uint64(dirent.Ino)
 }

cmd/posix-dirent_namelen_bsd.go

@@ -0,0 +1,25 @@
+// +build darwin freebsd openbsd netbsd
+
+/*
+ * MinIO Cloud Storage, (C) 2019 MinIO, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package cmd
+
+import "syscall"
+
+func direntNamlen(dirent *syscall.Dirent) (uint64, error) {
+	return uint64(dirent.Namlen), nil
+}

cmd/posix-dirent_namelen_linux.go

@@ -0,0 +1,43 @@
+// +build linux,!appengine
+
+/*
+ * MinIO Cloud Storage, (C) 2019 MinIO, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package cmd
+
+import (
+	"bytes"
+	"fmt"
+	"syscall"
+	"unsafe"
+)
+
+func direntNamlen(dirent *syscall.Dirent) (uint64, error) {
+	const fixedHdr = uint16(unsafe.Offsetof(syscall.Dirent{}.Name))
+	nameBuf := (*[unsafe.Sizeof(dirent.Name)]byte)(unsafe.Pointer(&dirent.Name[0]))
+	const nameBufLen = uint16(len(nameBuf))
+	limit := dirent.Reclen - fixedHdr
+	if limit > nameBufLen {
+		limit = nameBufLen
+	}
+	// Avoid bugs in long file names
+	// https://github.com/golang/tools/commit/5f9a5413737ba4b4f692214aebee582b47c8be74
+	nameLen := bytes.IndexByte(nameBuf[:limit], 0)
+	if nameLen < 0 {
+		return 0, fmt.Errorf("failed to find terminating 0 byte in dirent")
+	}
+	return uint64(nameLen), nil
+}

cmd/posix-list-dir_test.go

@@ -223,7 +223,7 @@ func TestReadDirN(t *testing.T) {
 	}{
 		{0, 0, 0},
 		{0, 1, 0},
-		{1, 0, 1},
+		{1, 0, 0},
 		{0, -1, 0},
 		{1, -1, 1},
 		{10, -1, 10},
@@ -236,19 +236,24 @@ func TestReadDirN(t *testing.T) {
 
 	for i, testCase := range testCases {
 		dir := mustSetupDir(t)
-		defer os.RemoveAll(dir)
 
 		for c := 1; c <= testCase.numFiles; c++ {
-			if err := ioutil.WriteFile(filepath.Join(dir, fmt.Sprintf("%d", c)), []byte{}, os.ModePerm); err != nil {
+			err := ioutil.WriteFile(filepath.Join(dir, fmt.Sprintf("%d", c)), []byte{}, os.ModePerm)
+			if err != nil {
+				os.RemoveAll(dir)
 				t.Fatalf("Unable to create a file, %s", err)
 			}
 		}
 		entries, err := readDirN(dir, testCase.n)
 		if err != nil {
+			os.RemoveAll(dir)
 			t.Fatalf("Unable to read entries, %s", err)
 		}
 		if len(entries) != testCase.expectedNum {
-			t.Fatalf("Test %d: unexpected number of entries, waiting for %d, but found %d", i+1, testCase.expectedNum, len(entries))
+			os.RemoveAll(dir)
+			t.Fatalf("Test %d: unexpected number of entries, waiting for %d, but found %d",
+				i+1, testCase.expectedNum, len(entries))
 		}
+		os.RemoveAll(dir)
 	}
 }

cmd/posix-list-dir_unix.go

@@ -1,4 +1,4 @@
-// +build darwin dragonfly freebsd linux nacl netbsd openbsd
+// +build linux,!appengine darwin freebsd netbsd openbsd
 
 /*
  * MinIO Cloud Storage, (C) 2016, 2017, 2018 MinIO, Inc.
@@ -19,93 +19,56 @@
 package cmd
 
 import (
+	"fmt"
 	"os"
-	"runtime"
-	"sync"
 	"syscall"
 	"unsafe"
 )
 
-const (
-	// readDirentBufSize for syscall.ReadDirent() to hold multiple
-	// directory entries in one buffer. golang source uses 4096 as
-	// buffer size whereas we want 64 times larger to save lots of
-	// entries to avoid multiple syscall.ReadDirent() call.
-	readDirentBufSize = 4096 * 64
-)
-
-// actual length of the byte array from the c - world.
-func clen(n []byte) int {
-	for i := 0; i < len(n); i++ {
-		if n[i] == 0 {
-			return i
-		}
-	}
-	return len(n)
-}
+// The buffer must be at least a block long.
+// refer https://github.com/golang/go/issues/24015
+const blockSize = 8 << 10
 
-// parseDirents - inspired from
-// https://golang.org/src/syscall/syscall_<os>.go
-func parseDirents(dirPath string, buf []byte) (entries []string, err error) {
-	bufidx := 0
-	for bufidx < len(buf) {
-		dirent := (*syscall.Dirent)(unsafe.Pointer(&buf[bufidx]))
-		// On non-Linux operating systems for rec length of zero means
-		// we have reached EOF break out.
-		if runtime.GOOS != "linux" && dirent.Reclen == 0 {
-			break
-		}
-		bufidx += int(dirent.Reclen)
-		// Skip if they are absent in directory.
-		if isEmptyDirent(dirent) {
-			continue
-		}
-		bytes := (*[10000]byte)(unsafe.Pointer(&dirent.Name[0]))
-		var name = string(bytes[0:clen(bytes[:])])
-		// Reserved names skip them.
-		if name == "." || name == ".." {
-			continue
-		}
+// unexpectedFileMode is a sentinel (and bogus) os.FileMode
+// value used to represent a syscall.DT_UNKNOWN Dirent.Type.
+const unexpectedFileMode os.FileMode = os.ModeNamedPipe | os.ModeSocket | os.ModeDevice
 
-		switch dirent.Type {
-		case syscall.DT_DIR:
-			entries = append(entries, name+SlashSeparator)
-		case syscall.DT_REG:
-			entries = append(entries, name)
-		case syscall.DT_LNK, syscall.DT_UNKNOWN:
-			// If its symbolic link, follow the link using os.Stat()
+func parseDirEnt(buf []byte) (consumed int, name string, typ os.FileMode, err error) {
+	// golang.org/issue/15653
+	dirent := (*syscall.Dirent)(unsafe.Pointer(&buf[0]))
+	if v := unsafe.Offsetof(dirent.Reclen) + unsafe.Sizeof(dirent.Reclen); uintptr(len(buf)) < v {
+		return consumed, name, typ, fmt.Errorf("buf size of %d smaller than dirent header size %d", len(buf), v)
+	}
+	if len(buf) < int(dirent.Reclen) {
+		return consumed, name, typ, fmt.Errorf("buf size %d < record length %d", len(buf), dirent.Reclen)
+	}
+	consumed = int(dirent.Reclen)
+	if direntInode(dirent) == 0 { // File absent in directory.
+		return
+	}
+	switch dirent.Type {
+	case syscall.DT_REG:
+		typ = 0
+	case syscall.DT_DIR:
+		typ = os.ModeDir
+	case syscall.DT_LNK:
+		typ = os.ModeSymlink
+	default:
+		// Skip all other file types. Revisit if/when this code needs
+		// to handle such files, MinIO is only interested in
+		// files and directories.
+		typ = unexpectedFileMode
+		return
+	}
 
-			// On Linux XFS does not implement d_type for on disk
-			// format << v5. Fall back to OsStat().
-			var fi os.FileInfo
-			fi, err = os.Stat(pathJoin(dirPath, name))
-			if err != nil {
-				// If file does not exist, we continue and skip it.
-				// Could happen if it was deleted in the middle while
-				// this list was being performed.
-				if os.IsNotExist(err) {
-					continue
-				}
-				return nil, err
-			}
-			if fi.IsDir() {
-				entries = append(entries, name+SlashSeparator)
-			} else if fi.Mode().IsRegular() {
-				entries = append(entries, name)
-			}
-		default:
-			// Skip entries which are not file or directory.
-			continue
-		}
+	nameBuf := (*[unsafe.Sizeof(dirent.Name)]byte)(unsafe.Pointer(&dirent.Name[0]))
+	nameLen, err := direntNamlen(dirent)
+	if err != nil {
+		return consumed, name, typ, err
 	}
-	return entries, nil
-}
 
-var readDirBufPool = sync.Pool{
-	New: func() interface{} {
-		b := make([]byte, readDirentBufSize)
-		return &b
-	},
+	name = string(nameBuf[:nameLen])
+	return consumed, name, typ, nil
 }
 
 // Return all the entries at the directory dirPath.
@@ -116,11 +79,7 @@ func readDir(dirPath string) (entries []string, err error) {
 // Return count entries at the directory dirPath and all entries
 // if count is set to -1
 func readDirN(dirPath string, count int) (entries []string, err error) {
-	bufp := readDirBufPool.Get().(*[]byte)
-	buf := *bufp
-	defer readDirBufPool.Put(bufp)
-
-	d, err := os.Open(dirPath)
+	fd, err := syscall.Open(dirPath, 0, 0)
 	if err != nil {
 		if os.IsNotExist(err) || isSysErrNotDir(err) {
 			return nil, errFileNotFound
@@ -130,36 +89,54 @@ func readDirN(dirPath string, count int) (entries []string, err error) {
 		}
 		return nil, err
 	}
-	defer d.Close()
-
-	fd := int(d.Fd())
+	defer syscall.Close(fd)
 
-	remaining := count
-	done := false
+	buf := make([]byte, blockSize) // stack-allocated; doesn't escape
+	boff := 0                      // starting read position in buf
+	nbuf := 0                      // end valid data in buf
 
-	for !done {
-		nbuf, err := syscall.ReadDirent(fd, buf)
-		if err != nil {
-			if isSysErrNotDir(err) {
-				return nil, errFileNotFound
+	for count != 0 {
+		if boff >= nbuf {
+			boff = 0
+			nbuf, err = syscall.ReadDirent(fd, buf)
+			if err != nil {
+				if isSysErrNotDir(err) {
+					return nil, errFileNotFound
+				}
+				return nil, err
+			}
+			if nbuf <= 0 {
+				break
 			}
-			return nil, err
-		}
-		if nbuf <= 0 {
-			break
 		}
-		var tmpEntries []string
-		if tmpEntries, err = parseDirents(dirPath, buf[:nbuf]); err != nil {
+		consumed, name, typ, err := parseDirEnt(buf[boff:nbuf])
+		if err != nil {
 			return nil, err
 		}
-		if count > 0 {
-			if remaining <= len(tmpEntries) {
-				tmpEntries = tmpEntries[:remaining]
-				done = true
+		boff += consumed
+		if name == "" || name == "." || name == ".." {
+			continue
+		}
+		// Fallback for filesystems (like old XFS) that don't
+		// support Dirent.Type and have DT_UNKNOWN (0) there
+		// instead.
+		if typ == unexpectedFileMode || typ&os.ModeSymlink == os.ModeSymlink {
+			fi, err := os.Stat(pathJoin(dirPath, name))
+			if err != nil {
+				// It got deleted in the meantime.
+				if os.IsNotExist(err) {
+					continue
+				}
+				return nil, err
 			}
-			remaining -= len(tmpEntries)
+			typ = fi.Mode() & os.ModeType
+		}
+		if typ.IsRegular() {
+			entries = append(entries, name)
+		} else if typ.IsDir() {
+			entries = append(entries, name+SlashSeparator)
 		}
-		entries = append(entries, tmpEntries...)
+		count--
 	}
 	return
 }

cmd/posix-list-dir_windows.go

@@ -57,9 +57,7 @@ func readDirN(dirPath string, count int) (entries []string, err error) {
 
 	data := &syscall.Win32finddata{}
 
-	remaining := count
-	done := false
-	for !done {
+	for count != 0 {
 		e := syscall.FindNextFile(syscall.Handle(d.Fd()), data)
 		if e != nil {
 			if e == syscall.ERROR_NO_MORE_FILES {
@@ -74,7 +72,7 @@ func readDirN(dirPath string, count int) (entries []string, err error) {
 			}
 		}
 		name := syscall.UTF16ToString(data.FileName[0:])
-		if name == "." || name == ".." { // Useless names
+		if name == "" || name == "." || name == ".." { // Useless names
 			continue
 		}
 		switch {
@@ -101,10 +99,7 @@ func readDirN(dirPath string, count int) (entries []string, err error) {
 		default:
 			entries = append(entries, name)
 		}
-		if remaining > 0 {
-			remaining--
-			done = remaining == 0
-		}
+		count--
 	}
 	return entries, nil
 }