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>

diff --git a/src/net/fd_unix.go b/src/net/fd_unix.go
index 198f606284db22e7c523cca1fa8dc56f68627481..a8d3a253a97b108945306f99dddaf5a025a44f71 100644 (file)
--- 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 a34de8333da49b57aeccf07b358cad9e0be670ee..533a48404b9d090e390823222f2566348e8478e0 100644 (file)
--- 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 73940f982f442e56d1355f056da69c3b74c7b6ba..98e7afd0f6ba0a8b55c73a092d4cf6cf6ce0cfb3 100644 (file)
--- 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 fbc8cc1b9f8fdcd85fd970fd98e3c634720034a6..a0bfd991e33c1ea5e8554117c62e0ff1391e0146 100644 (file)
--- 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 1e2e8ccb67cb78510589597ee1ed038575ed251d..2d293fdb4d9665fca9034e989e9bf127bc6f6d0a 100644 (file)
--- 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 a12412e0ca798722f08b0e748ecf020c017bd0c6..2eb11a04cb2d929ac9f11769f0a18a59ab8b1cba 100644 (file)
--- 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 378733ffdb4d636011a1bf143f34e676e1d29c55..8ea5df411740fd7dfbb5025b11b478502ca1ff71 100644 (file)
--- 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
 }