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>

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)
+	}
+}

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
 }

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)