From: Michael Pratt Date: Wed, 24 May 2023 15:25:12 +0000 (-0400) Subject: os: explicitly check for invalid FD in NewFile X-Git-Tag: go1.21rc1~277 X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=a6fb97b6a7f42b8d4cbd8890672ccbf3cf0a929a;p=gostls13.git 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 Reviewed-by: Bryan Mills TryBot-Result: Gopher Robot Reviewed-by: Tobias Klauser Auto-Submit: Michael Pratt Reviewed-by: Ian Lance Taylor --- 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 }