From 594fa53f0001ef3fa9e016f148230ddd59727649 Mon Sep 17 00:00:00 2001 From: Nahum Shalman Date: Fri, 21 Jan 2022 02:04:56 +0000 Subject: [PATCH] unix: solaris/illumos Event Ports ENOENT cleanup When we receive ENOENT from port_dissociate, we should clean up our map reference. To do so safely, we need a place to track the user cookies. This work is in support of fsnotify/fsnotify#371 Change-Id: Iae53ccc508d533941ad19df9051ca046262095ef Reviewed-on: https://go-review.googlesource.com/c/sys/+/380034 Trust: Tobias Klauser Run-TryBot: Tobias Klauser TryBot-Result: Gopher Robot Reviewed-by: Ian Lance Taylor --- unix/syscall_internal_solaris_test.go | 179 ++++++++++++++++++++++++++ unix/syscall_solaris.go | 124 ++++++++++++------ 2 files changed, 261 insertions(+), 42 deletions(-) create mode 100644 unix/syscall_internal_solaris_test.go diff --git a/unix/syscall_internal_solaris_test.go b/unix/syscall_internal_solaris_test.go new file mode 100644 index 00000000..715f3043 --- /dev/null +++ b/unix/syscall_internal_solaris_test.go @@ -0,0 +1,179 @@ +// Copyright 2022 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +//go:build solaris +// +build solaris + +package unix + +import ( + "fmt" + "os" + "runtime" + "testing" +) + +func (e *EventPort) checkInternals(t *testing.T, fds, paths, cookies, pending int) { + t.Helper() + p, err := e.Pending() + if err != nil { + t.Fatalf("failed to query how many events are pending") + } + if len(e.fds) != fds || len(e.paths) != paths || len(e.cookies) != cookies || p != pending { + format := "| fds: %d | paths: %d | cookies: %d | pending: %d |" + expected := fmt.Sprintf(format, fds, paths, cookies, pending) + got := fmt.Sprintf(format, len(e.fds), len(e.paths), len(e.cookies), p) + t.Errorf("Internal state mismatch\nfound: %s\nexpected: %s", got, expected) + } +} + +// Regression test for DissociatePath returning ENOENT +// This test is intended to create a linear worst +// case scenario of events being associated and +// fired but not consumed before additional +// calls to dissociate and associate happen +// This needs to be an internal test so that +// we can validate the state of the private maps +func TestEventPortDissociateAlreadyGone(t *testing.T) { + port, err := NewEventPort() + if err != nil { + t.Fatalf("failed to create an EventPort") + } + defer port.Close() + dir := t.TempDir() + tmpfile, err := os.CreateTemp(dir, "eventport") + if err != nil { + t.Fatalf("unable to create a tempfile: %v", err) + } + path := tmpfile.Name() + stat, err := os.Stat(path) + if err != nil { + t.Fatalf("unexpected failure to Stat file: %v", err) + } + err = port.AssociatePath(path, stat, FILE_MODIFIED, "cookie1") + if err != nil { + t.Fatalf("unexpected failure associating file: %v", err) + } + // We should have 1 path registered and 1 cookie in the jar + port.checkInternals(t, 0, 1, 1, 0) + // The path is associated, let's delete it. + err = os.Remove(path) + if err != nil { + t.Fatalf("unexpected failure deleting file: %v", err) + } + // The file has been deleted, some sort of pending event is probably + // queued in the kernel waiting for us to get it AND the kernel is + // no longer watching for events on it. BUT... Because we haven't + // consumed the event, this API thinks it's still watched: + watched := port.PathIsWatched(path) + if !watched { + t.Errorf("unexpected result from PathIsWatched") + } + // Ok, let's dissociate the file even before reading the event. + // Oh, ENOENT. I guess it's not associated any more + err = port.DissociatePath(path) + if err != ENOENT { + t.Errorf("unexpected result dissociating a seemingly associated path we know isn't: %v", err) + } + // As established by the return value above, this should clearly be false now: + // Narrator voice: in the first version of this API, it wasn't. + watched = port.PathIsWatched(path) + if watched { + t.Errorf("definitively unwatched file still in the map") + } + // We should have nothing registered, but 1 pending event corresponding + // to the cookie in the jar + port.checkInternals(t, 0, 0, 1, 1) + f, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE, 0666) + if err != nil { + t.Fatalf("creating test file failed: %s", err) + } + err = f.Close() + if err != nil { + t.Fatalf("unexpected failure closing file: %v", err) + } + stat, err = os.Stat(path) + if err != nil { + t.Fatalf("unexpected failure to Stat file: %v", err) + } + c := "cookie2" // c is for cookie, that's good enough for me + err = port.AssociatePath(path, stat, FILE_MODIFIED, c) + if err != nil { + t.Errorf("unexpected failure associating file: %v", err) + } + // We should have 1 registered path and its cookie + // as well as a second cookie corresponding to the pending event + port.checkInternals(t, 0, 1, 2, 1) + + // Fire another event + err = os.Remove(path) + if err != nil { + t.Fatalf("unexpected failure deleting file: %v", err) + } + port.checkInternals(t, 0, 1, 2, 2) + err = port.DissociatePath(path) + if err != ENOENT { + t.Errorf("unexpected result dissociating a seemingly associated path we know isn't: %v", err) + } + // By dissociating this path after deletion we ensure that the paths map is now empty + // If we're not careful we could trigger a nil pointer exception + port.checkInternals(t, 0, 0, 2, 2) + + f, err = os.OpenFile(path, os.O_WRONLY|os.O_CREATE, 0666) + if err != nil { + t.Fatalf("creating test file failed: %s", err) + } + err = f.Close() + if err != nil { + t.Fatalf("unexpected failure closing file: %v", err) + } + stat, err = os.Stat(path) + if err != nil { + t.Fatalf("unexpected failure to Stat file: %v", err) + } + // Put a seemingly duplicate cookie in the jar to see if we can trigger an incorrect removal from the paths map + err = port.AssociatePath(path, stat, FILE_MODIFIED, c) + if err != nil { + t.Errorf("unexpected failure associating file: %v", err) + } + port.checkInternals(t, 0, 1, 3, 2) + + // run the garbage collector so that if we messed up it should be painfully clear + runtime.GC() + + // Before the fix, this would cause a nil pointer exception + e, err := port.GetOne(nil) + if err != nil { + t.Errorf("failed to get an event: %v", err) + } + port.checkInternals(t, 0, 1, 2, 1) + if e.Cookie != "cookie1" { + t.Errorf(`expected "cookie1", got "%v"`, e.Cookie) + } + // Make sure that a cookie of the same value doesn't cause removal from the paths map incorrectly + e, err = port.GetOne(nil) + if err != nil { + t.Errorf("failed to get an event: %v", err) + } + port.checkInternals(t, 0, 1, 1, 0) + if e.Cookie != "cookie2" { + t.Errorf(`expected "cookie2", got "%v"`, e.Cookie) + } + + err = os.Remove(path) + if err != nil { + t.Fatalf("unexpected failure deleting file: %v", err) + } + // Event has fired, but until processed it should still be in the map + port.checkInternals(t, 0, 1, 1, 1) + e, err = port.GetOne(nil) + if err != nil { + t.Errorf("failed to get an event: %v", err) + } + if e.Cookie != "cookie2" { + t.Errorf(`expected "cookie2", got "%v"`, e.Cookie) + } + // The maps should be empty and there should be no pending events + port.checkInternals(t, 0, 0, 0, 0) +} diff --git a/unix/syscall_solaris.go b/unix/syscall_solaris.go index 8c4e8006..5c2003ce 100644 --- a/unix/syscall_solaris.go +++ b/unix/syscall_solaris.go @@ -737,8 +737,20 @@ type fileObjCookie struct { type EventPort struct { port int mu sync.Mutex - fds map[uintptr]interface{} + fds map[uintptr]*fileObjCookie paths map[string]*fileObjCookie + // The user cookie presents an interesting challenge from a memory management perspective. + // There are two paths by which we can discover that it is no longer in use: + // 1. The user calls port_dissociate before any events fire + // 2. An event fires and we return it to the user + // The tricky situation is if the event has fired in the kernel but + // the user hasn't requested/received it yet. + // If the user wants to port_dissociate before the event has been processed, + // 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 } // PortEvent is an abstraction of the port_event C struct. @@ -762,9 +774,10 @@ func NewEventPort() (*EventPort, error) { return nil, err } e := &EventPort{ - port: port, - fds: make(map[uintptr]interface{}), - paths: make(map[string]*fileObjCookie), + port: port, + fds: make(map[uintptr]*fileObjCookie), + paths: make(map[string]*fileObjCookie), + cookies: make(map[*interface{}]*fileObjCookie), } return e, nil } @@ -779,9 +792,13 @@ func NewEventPort() (*EventPort, error) { func (e *EventPort) Close() error { e.mu.Lock() defer e.mu.Unlock() + err := Close(e.port) + if err != nil { + return err + } e.fds = nil e.paths = nil - return Close(e.port) + return nil } // PathIsWatched checks to see if path is associated with this EventPort. @@ -818,6 +835,7 @@ func (e *EventPort) AssociatePath(path string, stat os.FileInfo, events int, coo return err } e.paths[path] = fCookie + e.cookies[&fCookie.cookie] = fCookie return nil } @@ -830,11 +848,19 @@ func (e *EventPort) DissociatePath(path string) error { return fmt.Errorf("%v is not associated with this Event Port", path) } _, err := port_dissociate(e.port, PORT_SOURCE_FILE, uintptr(unsafe.Pointer(f.fobj))) - if err != nil { + // If the path is no longer associated with this event port (ENOENT) + // we should delete it from our map. We can still return ENOENT to the caller. + // But we need to save the cookie + if err != nil && err != ENOENT { return err } + if err == nil { + // dissociate was successful, safe to delete the cookie + fCookie := e.paths[path] + delete(e.cookies, &fCookie.cookie) + } delete(e.paths, path) - return nil + return err } // AssociateFd wraps calls to port_associate(3c) on file descriptors. @@ -844,12 +870,13 @@ 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) } - pcookie := &cookie - _, err := port_associate(e.port, PORT_SOURCE_FD, fd, events, (*byte)(unsafe.Pointer(pcookie))) + fCookie := &fileObjCookie{nil, cookie} + _, err := port_associate(e.port, PORT_SOURCE_FD, fd, events, (*byte)(unsafe.Pointer(&fCookie.cookie))) if err != nil { return err } - e.fds[fd] = pcookie + e.fds[fd] = fCookie + e.cookies[&fCookie.cookie] = fCookie return nil } @@ -862,11 +889,16 @@ func (e *EventPort) DissociateFd(fd uintptr) error { return fmt.Errorf("%v is not associated with this Event Port", fd) } _, err := port_dissociate(e.port, PORT_SOURCE_FD, fd) - if err != nil { + if err != nil && err != ENOENT { return err } + if err == nil { + // dissociate was successful, safe to delete the cookie + fCookie := e.fds[fd] + delete(e.cookies, &fCookie.cookie) + } delete(e.fds, fd) - return nil + return err } func createFileObj(name string, stat os.FileInfo) (*fileObj, error) { @@ -894,26 +926,48 @@ func (e *EventPort) GetOne(t *Timespec) (*PortEvent, error) { return nil, err } p := new(PortEvent) - p.Events = pe.Events - p.Source = pe.Source e.mu.Lock() defer e.mu.Unlock() - switch pe.Source { - case PORT_SOURCE_FD: - p.Fd = uintptr(pe.Object) - cookie := (*interface{})(unsafe.Pointer(pe.User)) - p.Cookie = *cookie - delete(e.fds, p.Fd) - case PORT_SOURCE_FILE: - p.fobj = (*fileObj)(unsafe.Pointer(uintptr(pe.Object))) - p.Path = BytePtrToString((*byte)(unsafe.Pointer(p.fobj.Name))) - cookie := (*interface{})(unsafe.Pointer(pe.User)) - p.Cookie = *cookie - delete(e.paths, p.Path) - } + e.peIntToExt(pe, p) 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) { + peExt.Events = peInt.Events + peExt.Source = peInt.Source + cookie := (*interface{})(unsafe.Pointer(peInt.User)) + peExt.Cookie = *cookie + 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 { + 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("mismanaged memory") + } + delete(e.cookies, cookie) + 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 { + delete(e.paths, peExt.Path) + } + } + } +} + // Pending wraps port_getn(3c) and returns how many events are pending. func (e *EventPort) Pending() (int, error) { var n uint32 = 0 @@ -944,21 +998,7 @@ func (e *EventPort) Get(s []PortEvent, min int, timeout *Timespec) (int, error) e.mu.Lock() defer e.mu.Unlock() for i := 0; i < int(got); i++ { - s[i].Events = ps[i].Events - s[i].Source = ps[i].Source - switch ps[i].Source { - case PORT_SOURCE_FD: - s[i].Fd = uintptr(ps[i].Object) - cookie := (*interface{})(unsafe.Pointer(ps[i].User)) - s[i].Cookie = *cookie - delete(e.fds, s[i].Fd) - case PORT_SOURCE_FILE: - s[i].fobj = (*fileObj)(unsafe.Pointer(uintptr(ps[i].Object))) - s[i].Path = BytePtrToString((*byte)(unsafe.Pointer(s[i].fobj.Name))) - cookie := (*interface{})(unsafe.Pointer(ps[i].User)) - s[i].Cookie = *cookie - delete(e.paths, s[i].Path) - } + e.peIntToExt(&ps[i], &s[i]) } return int(got), err }