runtime: fix use after close race in Solaris network poller

The Solaris network poller uses event ports, which are
level-triggered. As such, it has to re-arm itself after each
wakeup. The arming mechanism (which runs in its own thread) raced
with the closing of a file descriptor happening in a different
thread. When a network file descriptor is about to be closed,
the network poller is awaken to give it a chance to remove its
association with the file descriptor. Because the poller always
re-armed itself, it raced with code that closed the descriptor.

This change makes the network poller check before re-arming if
the file descriptor is about to be closed, in which case it will
ignore the re-arming request. It uses the per-PollDesc lock in
order to serialize access to the PollDesc.

This change also adds extensive documentation describing the
Solaris implementation of the network poller.

Fixes issue 7410.

LGTM=dvyukov, iant
R=golang-codereviews, bradfitz, iant, dvyukov, aram.h, gobot
CC=golang-codereviews
https://codereview.appspot.com/69190044

Committer: Dmitriy Vyukov <dvyukov@google.com>

1 files changed, 111 insertions, 31 deletions

diff --git a/src/pkg/runtime/netpoll_solaris.c b/src/pkg/runtime/netpoll_solaris.c
index f745f2343..a2631a8ab 100644
--- a/src/pkg/runtime/netpoll_solaris.c
+++ b/src/pkg/runtime/netpoll_solaris.c
@@ -7,6 +7,67 @@
 #include "defs_GOOS_GOARCH.h"
 #include "os_GOOS.h"
 
+// Solaris runtime-integrated network poller.
+// 
+// Solaris uses event ports for scalable network I/O. Event
+// ports are level-triggered, unlike epoll and kqueue which
+// can be configured in both level-triggered and edge-triggered
+// mode. Level triggering means we have to keep track of a few things
+// ourselves. After we receive an event for a file descriptor,
+// it's our responsibility to ask again to be notified for future
+// events for that descriptor. When doing this we must keep track of
+// what kind of events the goroutines are currently interested in,
+// for example a fd may be open both for reading and writing.
+// 
+// A description of the high level operation of this code
+// follows. Networking code will get a file descriptor by some means
+// and will register it with the netpolling mechanism by a code path
+// that eventually calls runtime·netpollopen. runtime·netpollopen
+// calls port_associate with an empty event set. That means that we
+// will not receive any events at this point. The association needs
+// to be done at this early point because we need to process the I/O
+// readiness notification at some point in the future. If I/O becomes
+// ready when nobody is listening, when we finally care about it,
+// nobody will tell us anymore.
+// 
+// Beside calling runtime·netpollopen, the networking code paths
+// will call runtime·netpollarm each time goroutines are interested
+// in doing network I/O. Because now we know what kind of I/O we
+// are interested in (reading/writting), we can call port_associate
+// passing the correct type of event set (POLLIN/POLLOUT). As we made
+// sure to have already associated the file descriptor with the port,
+// when we now call port_associate, we will unblock the main poller
+// loop (in runtime·netpoll) right away if the socket is actually
+// ready for I/O.
+// 
+// The main poller loop runs in its own thread waiting for events
+// using port_getn. When an event happens, it will tell the scheduler
+// about it using runtime·netpollready. Besides doing this, it must
+// also re-associate the events that were not part of this current
+// notification with the file descriptor. Failing to do this would
+// mean each notification will prevent concurrent code using the
+// same file descriptor in parallel.
+// 
+// The logic dealing with re-associations is encapsulated in
+// runtime·netpollupdate. This function takes care to associate the
+// descriptor only with the subset of events that were previously
+// part of the association, except the one that just happened. We
+// can't re-associate with that right away, because event ports
+// are level triggered so it would cause a busy loop. Instead, that
+// association is effected only by the runtime·netpollarm code path,
+// when Go code actually asks for I/O.
+// 
+// The open and arming mechanisms are serialized using the lock
+// inside PollDesc. This is required because the netpoll loop runs
+// asynchonously in respect to other Go code and by the time we get
+// to call port_associate to update the association in the loop, the
+// file descriptor might have been closed and reopened already. The
+// lock allows runtime·netpollupdate to be called synchronously from
+// the loop thread while preventing other threads operating to the
+// same PollDesc, so once we unblock in the main loop, until we loop
+// again we know for sure we are always talking about the same file
+// descriptor and can safely access the data we want (the event set).
+
 #pragma dynimport libc·fcntl fcntl "libc.so"
 #pragma dynimport libc·port_create port_create "libc.so"
 #pragma dynimport libc·port_associate port_associate "libc.so"
@@ -71,10 +132,19 @@ runtime·netpollinit(void)
 int32
 runtime·netpollopen(uintptr fd, PollDesc *pd)
 {
-	uint32 events = POLLIN | POLLOUT;
-	*runtime·netpolluser(pd) = (void*)events;
-
-	return runtime·port_associate(portfd, PORT_SOURCE_FD, fd, events, (uintptr)pd);
+	int32 r;
+
+	runtime·netpolllock(pd);
+	// We don't register for any specific type of events yet, that's
+	// netpollarm's job. We merely ensure we call port_associate before
+	// asynchonous connect/accept completes, so when we actually want
+	// to do any I/O, the call to port_associate (from netpollarm,
+	// with the interested event set) will unblock port_getn right away
+	// because of the I/O readiness notification.
+	*runtime·netpolluser(pd) = 0;
+	r = runtime·port_associate(portfd, PORT_SOURCE_FD, fd, 0, (uintptr)pd);
+	runtime·netpollunlock(pd);
+	return r;
 }
 
 int32
@@ -83,6 +153,9 @@ runtime·netpollclose(uintptr fd)
 	return runtime·port_dissociate(portfd, PORT_SOURCE_FD, fd);
 }
 
+// Updates the association with a new set of interested events. After
+// this call, port_getn will return one and only one event for that
+// particular descriptor, so this function needs to be called again.
 void
 runtime·netpollupdate(PollDesc* pd, uint32 set, uint32 clear)
 {
@@ -90,22 +163,26 @@ runtime·netpollupdate(PollDesc* pd, uint32 set, uint32 clear)
 	uintptr fd = runtime·netpollfd(pd);
 	ep = (uint32*)runtime·netpolluser(pd);
 
-	do {
-		old = *ep;
-		events = (old & ~clear) | set;
-		if(old == events)
-			return;
+	if(runtime·netpollclosing(pd))
+		return;
 
-		if(events && runtime·port_associate(portfd, PORT_SOURCE_FD, fd, events, (uintptr)pd) != 0) {
-			runtime·printf("netpollupdate: failed to associate (%d)\n", errno);
-			runtime·throw("netpollupdate: failed to associate");
-		}
-	} while(runtime·cas(ep, old, events) != events);
+	old = *ep;
+	events = (old & ~clear) | set;
+	if(old == events)
+		return;
+
+	if(events && runtime·port_associate(portfd, PORT_SOURCE_FD, fd, events, (uintptr)pd) != 0) {
+		runtime·printf("netpollupdate: failed to associate (%d)\n", errno);
+		runtime·throw("netpollupdate: failed to associate");
+	} 
+	*ep = events;
 }
 
+// subscribe the fd to the port such that port_getn will return one event.
 void
 runtime·netpollarm(PollDesc* pd, int32 mode)
 {
+	runtime·netpolllock(pd);
 	switch(mode) {
 	case 'r':
 		runtime·netpollupdate(pd, POLLIN, 0);
@@ -116,6 +193,7 @@ runtime·netpollarm(PollDesc* pd, int32 mode)
 	default:
 		runtime·throw("netpollarm: bad mode");
 	}
+	runtime·netpollunlock(pd);
 }
 
 // polls for ready network connections
@@ -126,7 +204,7 @@ runtime·netpoll(bool block)
 	static int32 lasterr;
 	PortEvent events[128], *ev;
 	PollDesc *pd;
-	int32 i, mode;
+	int32 i, mode, clear;
 	uint32 n;
 	Timespec *wait = nil, zero;
 	G *gp;
@@ -142,41 +220,43 @@ runtime·netpoll(bool block)
 
 retry:
 	n = 1;
-
 	if(runtime·port_getn(portfd, events, nelem(events), &n, wait) < 0) {
 		if(errno != EINTR && errno != lasterr) {
 			lasterr = errno;
-			runtime·printf("runtime: port_getn on fd %d "
-			    "failed with %d\n", portfd, errno);
+			runtime·printf("runtime: port_getn on fd %d failed with %d\n", portfd, errno);
 		}
 		goto retry;
 	}
 
 	gp = nil;
-
 	for(i = 0; i < n; i++) {
 		ev = &events[i];
 
 		if(ev->portev_events == 0)
 			continue;
-
-		if((pd = (PollDesc *)ev->portev_user) == nil)
-			continue;
+		pd = (PollDesc *)ev->portev_user;
 
 		mode = 0;
-
-		if(ev->portev_events & (POLLIN|POLLHUP|POLLERR))
+		clear = 0;
+		if(ev->portev_events & (POLLIN|POLLHUP|POLLERR)) {
 			mode += 'r';
-
-		if(ev->portev_events & (POLLOUT|POLLHUP|POLLERR))
+			clear |= POLLIN;
+		}
+		if(ev->portev_events & (POLLOUT|POLLHUP|POLLERR)) {
 			mode += 'w';
-
-		//
+			clear |= POLLOUT;
+		}
 		// To effect edge-triggered events, we need to be sure to
 		// update our association with whatever events were not
-		// set with the event.
-		//
-		runtime·netpollupdate(pd, 0, ev->portev_events & (POLLIN|POLLOUT));
+		// set with the event. For example if we are registered
+		// for POLLIN|POLLOUT, and we get POLLIN, besides waking
+		// the goroutine interested in POLLIN we have to not forget
+		// about the one interested in POLLOUT.
+		if(clear != 0) {
+			runtime·netpolllock(pd);
+			runtime·netpollupdate(pd, 0, clear);
+			runtime·netpollunlock(pd);
+		}
 
 		if(mode)
 			runtime·netpollready(&gp, pd, mode);