os: explicitly check for invalid FD in NewFile

CL 497075 refactored NewFile to unconditionally dereference the file
returned by newFile. However, newFile can return nil if passed a
negative FD, which now causes a crash.

Resolve this by moving the invalid check earlier in NewFile, which also
lets us avoid a useless fcntl syscall on a negative FD.

Since we convert to int to check sign, adjust newFile to take an int
rather than uintptr, which cleans up a lot of conversions.

Fixes #60406

Change-Id: I382a74e22f1cc01f7a2dcf1ff4efca6a79c4dd57
Reviewed-on: https://go-review.googlesource.com/c/go/+/497877
Run-TryBot: Michael Pratt <mpratt@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Tobias Klauser <tobias.klauser@gmail.com>
Auto-Submit: Michael Pratt <mpratt@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>

7 files changed, 39 insertions, 19 deletions

diff --git a/src/net/fd_unix.go b/src/net/fd_unix.go
index 198f606284..a8d3a253a9 100644
--- a/src/net/fd_unix.go
+++ b/src/net/fd_unix.go
@@ -191,7 +191,7 @@ func (fd *netFD) accept() (netfd *netFD, err error) {
 }
 
 // Defined in os package.
-func newUnixFile(fd uintptr, name string) *os.File
+func newUnixFile(fd int, name string) *os.File
 
 func (fd *netFD) dup() (f *os.File, err error) {
 	ns, call, err := fd.pfd.Dup()
@@ -202,5 +202,5 @@ func (fd *netFD) dup() (f *os.File, err error) {
 		return nil, err
 	}
 
-	return newUnixFile(uintptr(ns), fd.name()), nil
+	return newUnixFile(ns, fd.name()), nil
 }
diff --git a/src/os/file_unix.go b/src/os/file_unix.go
index a34de8333d..533a48404b 100644
--- a/src/os/file_unix.go
+++ b/src/os/file_unix.go
@@ -103,15 +103,20 @@ func (f *File) Fd() uintptr {
 // conditions described in the comments of the Fd method, and the same
 // constraints apply.
 func NewFile(fd uintptr, name string) *File {
+	fdi := int(fd)
+	if fdi < 0 {
+		return nil
+	}
+
 	kind := kindNewFile
 	appendMode := false
-	if flags, err := unix.Fcntl(int(fd), syscall.F_GETFL, 0); err == nil {
+	if flags, err := unix.Fcntl(fdi, syscall.F_GETFL, 0); err == nil {
 		if unix.HasNonblockFlag(flags) {
 			kind = kindNonBlock
 		}
 		appendMode = flags&syscall.O_APPEND != 0
 	}
-	f := newFile(fd, name, kind)
+	f := newFile(fdi, name, kind)
 	f.appendMode = appendMode
 	return f
 }
@@ -126,7 +131,11 @@ func NewFile(fd uintptr, name string) *File {
 // retain that behavior because existing code expects it and depends on it.
 //
 //go:linkname net_newUnixFile net.newUnixFile
-func net_newUnixFile(fd uintptr, name string) *File {
+func net_newUnixFile(fd int, name string) *File {
+	if fd < 0 {
+		panic("invalid FD")
+	}
+
 	f := newFile(fd, name, kindNonBlock)
 	f.nonblock = true // tell Fd to return blocking descriptor
 	return f
@@ -155,19 +164,15 @@ const (
 // newFile is like NewFile, but if called from OpenFile or Pipe
 // (as passed in the kind parameter) it tries to add the file to
 // the runtime poller.
-func newFile(fd uintptr, name string, kind newFileKind) *File {
-	fdi := int(fd)
-	if fdi < 0 {
-		return nil
-	}
+func newFile(fd int, name string, kind newFileKind) *File {
 	f := &File{&file{
 		pfd: poll.FD{
-			Sysfd:         fdi,
+			Sysfd:         fd,
 			IsStream:      true,
 			ZeroReadIsEOF: true,
 		},
 		name:        name,
-		stdoutOrErr: fdi == 1 || fdi == 2,
+		stdoutOrErr: fd == 1 || fd == 2,
 	}}
 
 	pollable := kind == kindOpenFile || kind == kindPipe || kind == kindNonBlock
@@ -180,7 +185,7 @@ func newFile(fd uintptr, name string, kind newFileKind) *File {
 		case "darwin", "ios", "dragonfly", "freebsd", "netbsd", "openbsd":
 			var st syscall.Stat_t
 			err := ignoringEINTR(func() error {
-				return syscall.Fstat(fdi, &st)
+				return syscall.Fstat(fd, &st)
 			})
 			typ := st.Mode & syscall.S_IFMT
 			// Don't try to use kqueue with regular files on *BSDs.
@@ -210,7 +215,7 @@ func newFile(fd uintptr, name string, kind newFileKind) *File {
 			// The descriptor is already in non-blocking mode.
 			// We only set f.nonblock if we put the file into
 			// non-blocking mode.
-		} else if err := syscall.SetNonblock(fdi, true); err == nil {
+		} else if err := syscall.SetNonblock(fd, true); err == nil {
 			f.nonblock = true
 			clearNonBlock = true
 		} else {
@@ -226,7 +231,7 @@ func newFile(fd uintptr, name string, kind newFileKind) *File {
 	// will show up in later I/O.
 	// We do restore the blocking behavior if it was set by us.
 	if pollErr := f.pfd.Init("file", pollable); pollErr != nil && clearNonBlock {
-		if err := syscall.SetNonblock(fdi, false); err == nil {
+		if err := syscall.SetNonblock(fd, false); err == nil {
 			f.nonblock = false
 		}
 	}
@@ -293,7 +298,7 @@ func openFileNolog(name string, flag int, perm FileMode) (*File, error) {
 		kind = kindNonBlock
 	}
 
-	f := newFile(uintptr(r), name, kind)
+	f := newFile(r, name, kind)
 	f.pfd.SysFile = s
 	return f, nil
 }
diff --git a/src/os/os_unix_test.go b/src/os/os_unix_test.go
index 73940f982f..98e7afd0f6 100644
--- a/src/os/os_unix_test.go
+++ b/src/os/os_unix_test.go
@@ -314,6 +314,14 @@ func TestNewFileNonBlock(t *testing.T) {
 	newFileTest(t, false)
 }
 
+func TestNewFileInvalid(t *testing.T) {
+	t.Parallel()
+	const negOne = ^uintptr(0)
+	if f := NewFile(negOne, "invalid"); f != nil {
+		t.Errorf("NewFile(-1) got %v want nil", f)
+	}
+}
+
 func TestSplitPath(t *testing.T) {
 	t.Parallel()
 	for _, tt := range []struct{ path, wantDir, wantBase string }{
diff --git a/src/os/os_windows_test.go b/src/os/os_windows_test.go
index fbc8cc1b9f..a0bfd991e3 100644
--- a/src/os/os_windows_test.go
+++ b/src/os/os_windows_test.go
@@ -1446,3 +1446,10 @@ func TestUTF16Alloc(t *testing.T) {
 		syscall.UTF16FromString("abc")
 	})
 }
+
+func TestNewFileInvalid(t *testing.T) {
+	t.Parallel()
+	if f := os.NewFile(uintptr(syscall.InvalidHandle), "invalid"); f != nil {
+		t.Errorf("NewFile(InvalidHandle) got %v want nil", f)
+	}
+}
diff --git a/src/os/pipe2_unix.go b/src/os/pipe2_unix.go
index 1e2e8ccb67..2d293fdb4d 100644
--- a/src/os/pipe2_unix.go
+++ b/src/os/pipe2_unix.go
@@ -18,5 +18,5 @@ func Pipe() (r *File, w *File, err error) {
 		return nil, nil, NewSyscallError("pipe2", e)
 	}
 
-	return newFile(uintptr(p[0]), "|0", kindPipe), newFile(uintptr(p[1]), "|1", kindPipe), nil
+	return newFile(p[0], "|0", kindPipe), newFile(p[1], "|1", kindPipe), nil
 }
diff --git a/src/os/pipe_unix.go b/src/os/pipe_unix.go
index a12412e0ca..2eb11a04cb 100644
--- a/src/os/pipe_unix.go
+++ b/src/os/pipe_unix.go
@@ -24,5 +24,5 @@ func Pipe() (r *File, w *File, err error) {
 	syscall.CloseOnExec(p[1])
 	syscall.ForkLock.RUnlock()
 
-	return newFile(uintptr(p[0]), "|0", kindPipe), newFile(uintptr(p[1]), "|1", kindPipe), nil
+	return newFile(p[0], "|0", kindPipe), newFile(p[1], "|1", kindPipe), nil
 }
diff --git a/src/os/removeall_at.go b/src/os/removeall_at.go
index 378733ffdb..8ea5df4117 100644
--- a/src/os/removeall_at.go
+++ b/src/os/removeall_at.go
@@ -195,5 +195,5 @@ func openFdAt(dirfd int, name string) (*File, error) {
 	}
 
 	// We use kindNoPoll because we know that this is a directory.
-	return newFile(uintptr(r), name, kindNoPoll), nil
+	return newFile(r, name, kindNoPoll), nil
 }