From 27713097b9563e84e4e03a2ed9652ef9fe62263a Mon Sep 17 00:00:00 2001 From: Nahum Shalman Date: Wed, 10 Aug 2022 02:07:30 +0000 Subject: [PATCH] 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 TryBot-Result: Gopher Robot Run-TryBot: Tobias Klauser Reviewed-by: Michael Knyszek Auto-Submit: Tobias Klauser Reviewed-by: Ian Lance Taylor --- unix/syscall_internal_solaris_test.go | 57 +++++++++++++ unix/syscall_solaris.go | 111 ++++++++++++++++---------- unix/syscall_solaris_test.go | 11 ++- 3 files changed, 129 insertions(+), 50 deletions(-) diff --git a/unix/syscall_internal_solaris_test.go b/unix/syscall_internal_solaris_test.go index 715f3043..a92753a1 100644 --- a/unix/syscall_internal_solaris_test.go +++ b/unix/syscall_internal_solaris_test.go @@ -177,3 +177,60 @@ func TestEventPortDissociateAlreadyGone(t *testing.T) { // 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 03fa546e..8c6f4092 100644 --- a/unix/syscall_solaris.go +++ b/unix/syscall_solaris.go @@ -750,8 +750,8 @@ type EventPort struct { // 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 @@ func NewEventPort() (*EventPort, error) { 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 @@ func (e *EventPort) Close() error { } e.fds = nil e.paths = nil + e.cookies = nil return nil } @@ -826,17 +827,16 @@ func (e *EventPort) AssociatePath(path string, stat os.FileInfo, events int, coo 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 @@ func (e *EventPort) DissociatePath(path string) error { 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 @@ func (e *EventPort) AssociateFd(fd uintptr, events int, cookie interface{}) erro 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 @@ func (e *EventPort) DissociateFd(fd uintptr) error { 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 @@ func (e *EventPort) GetOne(t *Timespec) (*PortEvent, error) { 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 @@ func (e *EventPort) Get(s []PortEvent, min int, timeout *Timespec) (int, error) 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 @@ func (e *EventPort) Get(s []PortEvent, min int, timeout *Timespec) (int, error) } 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 c2b28be2..93f2732e 100644 --- a/unix/syscall_solaris_test.go +++ b/unix/syscall_solaris_test.go @@ -9,7 +9,6 @@ package unix_test import ( "fmt" - "io/ioutil" "os" "os/exec" "runtime" @@ -49,7 +48,7 @@ func TestSysconf(t *testing.T) { // 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 TestEventPortFds(t *testing.T) { } 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 @@ func TestEventPortErrors(t *testing.T) { 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 @@ func TestPortEventSlices(t *testing.T) { } // 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 @@ func TestPortEventSlices(t *testing.T) { } 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)