unix: fix event port panic after close
Fixes golang/go#54363
Change-Id: I38813aefaf58ef91605e0a6e3e6622c8a39d6a8d
Reviewed-on: https://go-review.googlesource.com/c/sys/+/422338
Reviewed-by: Tobias Klauser <tobias.klauser@gmail.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Auto-Submit: Tobias Klauser <tobias.klauser@gmail.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
diff --git a/unix/syscall_internal_solaris_test.go b/unix/syscall_internal_solaris_test.go
index 715f304..a92753a 100644
--- a/unix/syscall_internal_solaris_test.go
+++ b/unix/syscall_internal_solaris_test.go
@@ -177,3 +177,60 @@
// The maps should be empty and there should be no pending events
port.checkInternals(t, 0, 0, 0, 0)
}
+
+// Regression test for spuriously triggering a panic about memory mismanagement
+// that can be triggered by an event processing thread trying to process an event
+// after a different thread has already called port.Close().
+// Implemented as an internal test so that we can just simulate the Close()
+// because if you call close first in the same thread, things work properly
+// anyway.
+func TestEventPortGetAfterClose(t *testing.T) {
+ port, err := NewEventPort()
+ if err != nil {
+ t.Fatalf("NewEventPort failed: %v", err)
+ }
+ // Create, associate, and delete 2 files
+ for i := 0; i < 2; i++ {
+ tmpfile, err := os.CreateTemp("", "eventport")
+ if err != nil {
+ t.Fatalf("unable to create tempfile: %v", err)
+ }
+ path := tmpfile.Name()
+ stat, err := os.Stat(path)
+ if err != nil {
+ t.Fatalf("unable to stat tempfile: %v", err)
+ }
+ err = port.AssociatePath(path, stat, FILE_MODIFIED, nil)
+ if err != nil {
+ t.Fatalf("unable to AssociatePath tempfile: %v", err)
+ }
+ err = os.Remove(path)
+ if err != nil {
+ t.Fatalf("unable to Remove tempfile: %v", err)
+ }
+ }
+ n, err := port.Pending()
+ if err != nil {
+ t.Errorf("Pending failed: %v", err)
+ }
+ if n != 2 {
+ t.Errorf("expected 2 pending events, got %d", n)
+ }
+ // Simulate a close from a different thread
+ port.fds = nil
+ port.paths = nil
+ port.cookies = nil
+ // Ensure that we get back reasonable errors rather than panic
+ _, err = port.GetOne(nil)
+ if err == nil || err.Error() != "this EventPort is already closed" {
+ t.Errorf("didn't receive expected error of 'this EventPort is already closed'; got: %v", err)
+ }
+ events := make([]PortEvent, 2)
+ n, err = port.Get(events, 1, nil)
+ if n != 0 {
+ t.Errorf("expected to get back 0 events, got %d", n)
+ }
+ if err == nil || err.Error() != "this EventPort is already closed" {
+ t.Errorf("didn't receive expected error of 'this EventPort is already closed'; got: %v", err)
+ }
+}
diff --git a/unix/syscall_solaris.go b/unix/syscall_solaris.go
index 03fa546..8c6f409 100644
--- a/unix/syscall_solaris.go
+++ b/unix/syscall_solaris.go
@@ -750,8 +750,8 @@
// we should handle things gracefully. To do so, we need to keep an extra
// reference to the cookie around until the event is processed
// thus the otherwise seemingly extraneous "cookies" map
- // The key of this map is a pointer to the corresponding &fCookie.cookie
- cookies map[*interface{}]*fileObjCookie
+ // The key of this map is a pointer to the corresponding fCookie
+ cookies map[*fileObjCookie]struct{}
}
// PortEvent is an abstraction of the port_event C struct.
@@ -778,7 +778,7 @@
port: port,
fds: make(map[uintptr]*fileObjCookie),
paths: make(map[string]*fileObjCookie),
- cookies: make(map[*interface{}]*fileObjCookie),
+ cookies: make(map[*fileObjCookie]struct{}),
}
return e, nil
}
@@ -799,6 +799,7 @@
}
e.fds = nil
e.paths = nil
+ e.cookies = nil
return nil
}
@@ -826,17 +827,16 @@
if _, found := e.paths[path]; found {
return fmt.Errorf("%v is already associated with this Event Port", path)
}
- fobj, err := createFileObj(path, stat)
+ fCookie, err := createFileObjCookie(path, stat, cookie)
if err != nil {
return err
}
- fCookie := &fileObjCookie{fobj, cookie}
- _, err = port_associate(e.port, PORT_SOURCE_FILE, uintptr(unsafe.Pointer(fobj)), events, (*byte)(unsafe.Pointer(&fCookie.cookie)))
+ _, err = port_associate(e.port, PORT_SOURCE_FILE, uintptr(unsafe.Pointer(fCookie.fobj)), events, (*byte)(unsafe.Pointer(fCookie)))
if err != nil {
return err
}
e.paths[path] = fCookie
- e.cookies[&fCookie.cookie] = fCookie
+ e.cookies[fCookie] = struct{}{}
return nil
}
@@ -858,7 +858,7 @@
if err == nil {
// dissociate was successful, safe to delete the cookie
fCookie := e.paths[path]
- delete(e.cookies, &fCookie.cookie)
+ delete(e.cookies, fCookie)
}
delete(e.paths, path)
return err
@@ -871,13 +871,16 @@
if _, found := e.fds[fd]; found {
return fmt.Errorf("%v is already associated with this Event Port", fd)
}
- fCookie := &fileObjCookie{nil, cookie}
- _, err := port_associate(e.port, PORT_SOURCE_FD, fd, events, (*byte)(unsafe.Pointer(&fCookie.cookie)))
+ fCookie, err := createFileObjCookie("", nil, cookie)
+ if err != nil {
+ return err
+ }
+ _, err = port_associate(e.port, PORT_SOURCE_FD, fd, events, (*byte)(unsafe.Pointer(fCookie)))
if err != nil {
return err
}
e.fds[fd] = fCookie
- e.cookies[&fCookie.cookie] = fCookie
+ e.cookies[fCookie] = struct{}{}
return nil
}
@@ -896,27 +899,31 @@
if err == nil {
// dissociate was successful, safe to delete the cookie
fCookie := e.fds[fd]
- delete(e.cookies, &fCookie.cookie)
+ delete(e.cookies, fCookie)
}
delete(e.fds, fd)
return err
}
-func createFileObj(name string, stat os.FileInfo) (*fileObj, error) {
- fobj := new(fileObj)
- bs, err := ByteSliceFromString(name)
- if err != nil {
- return nil, err
+func createFileObjCookie(name string, stat os.FileInfo, cookie interface{}) (*fileObjCookie, error) {
+ fCookie := new(fileObjCookie)
+ fCookie.cookie = cookie
+ if name != "" && stat != nil {
+ fCookie.fobj = new(fileObj)
+ bs, err := ByteSliceFromString(name)
+ if err != nil {
+ return nil, err
+ }
+ fCookie.fobj.Name = (*int8)(unsafe.Pointer(&bs[0]))
+ s := stat.Sys().(*syscall.Stat_t)
+ fCookie.fobj.Atim.Sec = s.Atim.Sec
+ fCookie.fobj.Atim.Nsec = s.Atim.Nsec
+ fCookie.fobj.Mtim.Sec = s.Mtim.Sec
+ fCookie.fobj.Mtim.Nsec = s.Mtim.Nsec
+ fCookie.fobj.Ctim.Sec = s.Ctim.Sec
+ fCookie.fobj.Ctim.Nsec = s.Ctim.Nsec
}
- fobj.Name = (*int8)(unsafe.Pointer(&bs[0]))
- s := stat.Sys().(*syscall.Stat_t)
- fobj.Atim.Sec = s.Atim.Sec
- fobj.Atim.Nsec = s.Atim.Nsec
- fobj.Mtim.Sec = s.Mtim.Sec
- fobj.Mtim.Nsec = s.Mtim.Nsec
- fobj.Ctim.Sec = s.Ctim.Sec
- fobj.Ctim.Nsec = s.Ctim.Nsec
- return fobj, nil
+ return fCookie, nil
}
// GetOne wraps port_get(3c) and returns a single PortEvent.
@@ -929,44 +936,50 @@
p := new(PortEvent)
e.mu.Lock()
defer e.mu.Unlock()
- e.peIntToExt(pe, p)
+ err = e.peIntToExt(pe, p)
+ if err != nil {
+ return nil, err
+ }
return p, nil
}
// peIntToExt converts a cgo portEvent struct into the friendlier PortEvent
// NOTE: Always call this function while holding the e.mu mutex
-func (e *EventPort) peIntToExt(peInt *portEvent, peExt *PortEvent) {
+func (e *EventPort) peIntToExt(peInt *portEvent, peExt *PortEvent) error {
+ if e.cookies == nil {
+ return fmt.Errorf("this EventPort is already closed")
+ }
peExt.Events = peInt.Events
peExt.Source = peInt.Source
- cookie := (*interface{})(unsafe.Pointer(peInt.User))
- peExt.Cookie = *cookie
+ fCookie := (*fileObjCookie)(unsafe.Pointer(peInt.User))
+ _, found := e.cookies[fCookie]
+
+ if !found {
+ panic("unexpected event port address; may be due to kernel bug; see https://go.dev/issue/54254")
+ }
+ peExt.Cookie = fCookie.cookie
+ delete(e.cookies, fCookie)
+
switch peInt.Source {
case PORT_SOURCE_FD:
- delete(e.cookies, cookie)
peExt.Fd = uintptr(peInt.Object)
// Only remove the fds entry if it exists and this cookie matches
if fobj, ok := e.fds[peExt.Fd]; ok {
- if &fobj.cookie == cookie {
+ if fobj == fCookie {
delete(e.fds, peExt.Fd)
}
}
case PORT_SOURCE_FILE:
- if fCookie, ok := e.cookies[cookie]; ok && uintptr(unsafe.Pointer(fCookie.fobj)) == uintptr(peInt.Object) {
- // Use our stashed reference rather than using unsafe on what we got back
- // the unsafe version would be (*fileObj)(unsafe.Pointer(uintptr(peInt.Object)))
- peExt.fobj = fCookie.fobj
- } else {
- panic("unexpected event port address; may be due to kernel bug; see https://go.dev/issue/54254")
- }
- delete(e.cookies, cookie)
+ peExt.fobj = fCookie.fobj
peExt.Path = BytePtrToString((*byte)(unsafe.Pointer(peExt.fobj.Name)))
// Only remove the paths entry if it exists and this cookie matches
if fobj, ok := e.paths[peExt.Path]; ok {
- if &fobj.cookie == cookie {
+ if fobj == fCookie {
delete(e.paths, peExt.Path)
}
}
}
+ return nil
}
// Pending wraps port_getn(3c) and returns how many events are pending.
@@ -990,7 +1003,7 @@
got := uint32(min)
max := uint32(len(s))
var err error
- ps := make([]portEvent, max, max)
+ ps := make([]portEvent, max)
_, err = port_getn(e.port, &ps[0], max, &got, timeout)
// got will be trustworthy with ETIME, but not any other error.
if err != nil && err != ETIME {
@@ -998,8 +1011,18 @@
}
e.mu.Lock()
defer e.mu.Unlock()
+ valid := 0
for i := 0; i < int(got); i++ {
- e.peIntToExt(&ps[i], &s[i])
+ err2 := e.peIntToExt(&ps[i], &s[i])
+ if err2 != nil {
+ if valid == 0 && err == nil {
+ // If err2 is the only error and there are no valid events
+ // to return, return it to the caller.
+ err = err2
+ }
+ break
+ }
+ valid = i + 1
}
- return int(got), err
+ return valid, err
}
diff --git a/unix/syscall_solaris_test.go b/unix/syscall_solaris_test.go
index c2b28be..93f2732 100644
--- a/unix/syscall_solaris_test.go
+++ b/unix/syscall_solaris_test.go
@@ -9,7 +9,6 @@
import (
"fmt"
- "io/ioutil"
"os"
"os/exec"
"runtime"
@@ -49,7 +48,7 @@
// Event Ports
func TestBasicEventPort(t *testing.T) {
- tmpfile, err := ioutil.TempFile("", "eventport")
+ tmpfile, err := os.CreateTemp("", "eventport")
if err != nil {
t.Fatalf("unable to create a tempfile: %v", err)
}
@@ -162,7 +161,7 @@
}
func TestEventPortErrors(t *testing.T) {
- tmpfile, err := ioutil.TempFile("", "eventport")
+ tmpfile, err := os.CreateTemp("", "eventport")
if err != nil {
t.Errorf("unable to create a tempfile: %v", err)
}
@@ -189,7 +188,7 @@
if err == nil {
t.Errorf("unexpected success dissociating unassociated fd")
}
- events := make([]unix.PortEvent, 4, 4)
+ events := make([]unix.PortEvent, 4)
_, err = port.Get(events, 5, nil)
if err == nil {
t.Errorf("unexpected success calling Get with min greater than len of slice")
@@ -248,7 +247,7 @@
}
// Create, associate, and delete 6 files
for i := 0; i < 6; i++ {
- tmpfile, err := ioutil.TempFile("", "eventport")
+ tmpfile, err := os.CreateTemp("", "eventport")
if err != nil {
t.Fatalf("unable to create tempfile: %v", err)
}
@@ -275,7 +274,7 @@
}
timeout := new(unix.Timespec)
timeout.Nsec = 1
- events := make([]unix.PortEvent, 4, 4)
+ events := make([]unix.PortEvent, 4)
n, err = port.Get(events, 3, timeout)
if err != nil {
t.Errorf("Get failed: %v", err)
