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 <tobias.klauser@gmail.com>
Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
diff --git a/unix/syscall_internal_solaris_test.go b/unix/syscall_internal_solaris_test.go
new file mode 100644
index 0000000..715f304
--- /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 8c4e800..5c2003c 100644
--- a/unix/syscall_solaris.go
+++ b/unix/syscall_solaris.go
@@ -737,8 +737,20 @@
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 @@
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 (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 @@
return err
}
e.paths[path] = fCookie
+ e.cookies[&fCookie.cookie] = fCookie
return nil
}
@@ -830,11 +848,19 @@
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 @@
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 @@
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 @@
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 @@
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
}
