From a8296445ad62d12d449f6a47e991e88b22b5e4d2 Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Fri, 9 Aug 2019 08:54:11 -0700 Subject: [PATCH] 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. --- ...-dirent-ino.go => posix-dirent_fileino.go} | 9 +- ...x-dirent-fileno.go => posix-dirent_ino.go} | 10 +- cmd/posix-dirent_namelen_bsd.go | 25 +++ cmd/posix-dirent_namelen_linux.go | 43 ++++ cmd/posix-list-dir_test.go | 13 +- cmd/posix-list-dir_unix.go | 189 ++++++++---------- cmd/posix-list-dir_windows.go | 11 +- 7 files changed, 172 insertions(+), 128 deletions(-) rename cmd/{posix-dirent-ino.go => posix-dirent_fileino.go} (76%) rename cmd/{posix-dirent-fileno.go => posix-dirent_ino.go} (74%) create mode 100644 cmd/posix-dirent_namelen_bsd.go create mode 100644 cmd/posix-dirent_namelen_linux.go diff --git a/cmd/posix-dirent-ino.go b/cmd/posix-dirent_fileino.go similarity index 76% rename from cmd/posix-dirent-ino.go rename to cmd/posix-dirent_fileino.go index 8f433d9d3..2a9104b4d 100644 --- a/cmd/posix-dirent-ino.go +++ b/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) } diff --git a/cmd/posix-dirent-fileno.go b/cmd/posix-dirent_ino.go similarity index 74% rename from cmd/posix-dirent-fileno.go rename to cmd/posix-dirent_ino.go index 805858671..aa7ce457d 100644 --- a/cmd/posix-dirent-fileno.go +++ b/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) } diff --git a/cmd/posix-dirent_namelen_bsd.go b/cmd/posix-dirent_namelen_bsd.go new file mode 100644 index 000000000..6286f2e8b --- /dev/null +++ b/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 +} diff --git a/cmd/posix-dirent_namelen_linux.go b/cmd/posix-dirent_namelen_linux.go new file mode 100644 index 000000000..00e56aac7 --- /dev/null +++ b/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 +} diff --git a/cmd/posix-list-dir_test.go b/cmd/posix-list-dir_test.go index a84dbda6c..8cc711ce6 100644 --- a/cmd/posix-list-dir_test.go +++ b/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) } } diff --git a/cmd/posix-list-dir_unix.go b/cmd/posix-list-dir_unix.go index 8433191ba..13df56471 100644 --- a/cmd/posix-list-dir_unix.go +++ b/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 -) +// The buffer must be at least a block long. +// refer https://github.com/golang/go/issues/24015 +const blockSize = 8 << 10 -// 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 - } +// 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 + +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) } - return len(n) -} - -// parseDirents - inspired from -// https://golang.org/src/syscall/syscall_.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 - } - - 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() - - // 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 - } + 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 } - return entries, nil -} -var readDirBufPool = sync.Pool{ - New: func() interface{} { - b := make([]byte, readDirentBufSize) - return &b - }, + nameBuf := (*[unsafe.Sizeof(dirent.Name)]byte)(unsafe.Pointer(&dirent.Name[0])) + nameLen, err := direntNamlen(dirent) + if err != nil { + return consumed, name, typ, err + } + + 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() + defer syscall.Close(fd) - fd := int(d.Fd()) + buf := make([]byte, blockSize) // stack-allocated; doesn't escape + boff := 0 // starting read position in buf + nbuf := 0 // end valid data in buf - remaining := count - done := false - - for !done { - nbuf, err := syscall.ReadDirent(fd, buf) + 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 + } + } + consumed, name, typ, err := parseDirEnt(buf[boff:nbuf]) if err != nil { - if isSysErrNotDir(err) { - return nil, errFileNotFound - } return nil, err } - if nbuf <= 0 { - break + boff += consumed + if name == "" || name == "." || name == ".." { + continue } - var tmpEntries []string - if tmpEntries, err = parseDirents(dirPath, buf[:nbuf]); err != nil { - return nil, err - } - if count > 0 { - if remaining <= len(tmpEntries) { - tmpEntries = tmpEntries[:remaining] - done = true + // 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 } - entries = append(entries, tmpEntries...) + if typ.IsRegular() { + entries = append(entries, name) + } else if typ.IsDir() { + entries = append(entries, name+SlashSeparator) + } + count-- } return } diff --git a/cmd/posix-list-dir_windows.go b/cmd/posix-list-dir_windows.go index b6b98434b..f04a0fe89 100644 --- a/cmd/posix-list-dir_windows.go +++ b/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 }