2406. [bug] Sockets could be closed too early, leading to

inconsistent states in the socket module. [RT #18298]

CHANGES

@@ -1,3 +1,6 @@
+2406.	[bug]		Sockets could be closed too early, leading to
+			inconsistent states in the socket module. [RT #18298]
+
 2405.   [cleanup]       The default value for dnssec-validation was changed to
                         "yes" in 9.5.0-P1 and all subsequent releases; this
                         was inadvertently omitted from CHANGES at the time.

lib/isc/unix/socket.c

@@ -15,7 +15,7 @@
  * PERFORMANCE OF THIS SOFTWARE.
  */
 
-/* $Id: socket.c,v 1.294 2008/07/24 09:50:21 fdupont Exp $ */
+/* $Id: socket.c,v 1.295 2008/08/01 19:04:02 jinmei Exp $ */
 
 /*! \file */
 
@@ -333,7 +333,6 @@ static isc_socketmgr_t *socketmgr = NULL;
 #define CLOSED			0	/* this one must be zero */
 #define MANAGED			1
 #define CLOSE_PENDING		2
-#define MANAGER_CLOSE_PENDING	3
 
 /*
  * send() and recv() iovec counts
@@ -589,7 +588,6 @@ unwatch_fd(isc_socketmgr_t *manager, int fd, int msg) {
 static void
 wakeup_socket(isc_socketmgr_t *manager, int fd, int msg) {
 	isc_result_t result;
-	isc_boolean_t needclose;
 	int lockid = FDLOCK_ID(fd);
 
 	/*
@@ -600,11 +598,18 @@ wakeup_socket(isc_socketmgr_t *manager, int fd, int msg) {
 
 	INSIST(fd >= 0 && fd < (int)manager->maxsocks);
 
-	LOCK(&manager->fdlock[lockid]);
-	if (manager->fdstate[fd] == CLOSE_PENDING
-	    || manager->fdstate[fd] == MANAGER_CLOSE_PENDING) {
-		needclose = ISC_TF(manager->fdstate[fd] == CLOSE_PENDING);
+	if (msg == SELECT_POKE_CLOSE) {
+		/* No one should be updating fdstate, so no need to lock it */
+		INSIST(manager->fdstate[fd] == CLOSE_PENDING);
 		manager->fdstate[fd] = CLOSED;
+		(void)unwatch_fd(manager, fd, SELECT_POKE_READ);
+		(void)unwatch_fd(manager, fd, SELECT_POKE_WRITE);
+		(void)close(fd);
+		return;
+	}
+
+	LOCK(&manager->fdlock[lockid]);
+	if (manager->fdstate[fd] == CLOSE_PENDING) {
 		UNLOCK(&manager->fdlock[lockid]);
 
 		/*
@@ -617,8 +622,6 @@ wakeup_socket(isc_socketmgr_t *manager, int fd, int msg) {
 		 */
 		(void)unwatch_fd(manager, fd, SELECT_POKE_READ);
 		(void)unwatch_fd(manager, fd, SELECT_POKE_WRITE);
-		if (needclose)
-			(void)close(fd);
 		return;
 	}
 	if (manager->fdstate[fd] != MANAGED) {
@@ -1520,11 +1523,24 @@ closesocket(isc_socketmgr_t *manager, isc_sockettype_t type, int fd) {
 	LOCK(&manager->fdlock[lockid]);
 	manager->fds[fd] = NULL;
 	if (type == isc_sockettype_fdwatch)
-		manager->fdstate[fd] = MANAGER_CLOSE_PENDING;
+		manager->fdstate[fd] = CLOSED;
 	else
 		manager->fdstate[fd] = CLOSE_PENDING;
 	UNLOCK(&manager->fdlock[lockid]);
-	select_poke(manager, fd, SELECT_POKE_CLOSE);
+	if (type == isc_sockettype_fdwatch) {
+		/*
+		 * The caller may close the socket once this function returns,
+		 * and `fd' may be reassigned for a new socket.  So we do
+		 * unwatch_fd() here, rather than defer it via select_poke().
+		 * Note: this may complicate data protection among threads and
+		 * may reduce performance due to additional locks.  One way to
+		 * solve this would be to dup() the watched descriptor, but we
+		 * take a simpler approach at this moment.
+		 */
+		(void)unwatch_fd(manager, fd, SELECT_POKE_READ);
+		(void)unwatch_fd(manager, fd, SELECT_POKE_WRITE);
+	} else
+		select_poke(manager, fd, SELECT_POKE_CLOSE);
 
 	/*
 	 * update manager->maxfd here (XXX: this should be implemented more
@@ -2255,15 +2271,7 @@ dispatch_recv(isc_socket_t *sock) {
 	isc_socketevent_t *ev;
 	isc_task_t *sender;
 
-#if 0
-	/*
-	 * XXXJT: this assertion seems to strong, but leave it here for
-	 * reference.
-	 */
 	INSIST(!sock->pending_recv);
-#endif
-	if (sock->pending_recv != 0)
-		return;
 
 	if (sock->type != isc_sockettype_fdwatch) {
 		ev = ISC_LIST_HEAD(sock->recv_list);
@@ -2880,23 +2888,17 @@ process_fd(isc_socketmgr_t *manager, int fd, isc_boolean_t readable,
 {
 	isc_socket_t *sock;
 	isc_boolean_t unlock_sock;
-	isc_boolean_t needclose;
 	int lockid = FDLOCK_ID(fd);
 
 	/*
-	 * If we need to close the socket, do it now.
+	 * If the socket is going to be closed, don't do more I/O.
 	 */
 	LOCK(&manager->fdlock[lockid]);
-	if (manager->fdstate[fd] == CLOSE_PENDING
-	    || manager->fdstate[fd] == MANAGER_CLOSE_PENDING) {
-		needclose = ISC_TF(manager->fdstate[fd] == CLOSE_PENDING);
-		manager->fdstate[fd] = CLOSED;
+	if (manager->fdstate[fd] == CLOSE_PENDING) {
 		UNLOCK(&manager->fdlock[lockid]);
 
 		(void)unwatch_fd(manager, fd, SELECT_POKE_READ);
 		(void)unwatch_fd(manager, fd, SELECT_POKE_WRITE);
-		if (needclose)
-			(void)close(fd);
 		return;
 	}
 
@@ -2946,6 +2948,9 @@ process_fds(isc_socketmgr_t *manager, struct kevent *events, int nevents) {
 	int i;
 	isc_boolean_t readable, writable;
 	isc_boolean_t done = ISC_FALSE;
+#ifdef ISC_PLATFORM_USETHREADS
+	isc_boolean_t have_ctlevent = ISC_FALSE;
+#endif
 
 	if (nevents == manager->nevents) {
 		/*
@@ -2963,7 +2968,7 @@ process_fds(isc_socketmgr_t *manager, struct kevent *events, int nevents) {
 		REQUIRE(events[i].ident < manager->maxsocks);
 #ifdef ISC_PLATFORM_USETHREADS
 		if (events[i].ident == (uintptr_t)manager->pipe_fds[0]) {
-			done = process_ctlfd(manager);
+			have_ctlevent = ISC_TRUE;
 			continue;
 		}
 #endif
@@ -2972,6 +2977,11 @@ process_fds(isc_socketmgr_t *manager, struct kevent *events, int nevents) {
 		process_fd(manager, events[i].ident, readable, writable);
 	}
 
+#ifdef ISC_PLATFORM_USETHREADS
+	if (have_ctlevent)
+		done = process_ctlfd(manager);
+#endif
+
 	return (done);
 }
 #elif defined(USE_EPOLL)
@@ -2979,6 +2989,9 @@ static isc_boolean_t
 process_fds(isc_socketmgr_t *manager, struct epoll_event *events, int nevents) {
 	int i;
 	isc_boolean_t done = ISC_FALSE;
+#ifdef ISC_PLATFORM_USETHREADS
+	isc_boolean_t have_ctlevent = ISC_FALSE;
+#endif
 
 	if (nevents == manager->nevents) {
 		manager_log(manager, ISC_LOGCATEGORY_GENERAL,
@@ -2991,7 +3004,7 @@ process_fds(isc_socketmgr_t *manager, struct epoll_event *events, int nevents) {
 		REQUIRE(events[i].data.fd < (int)manager->maxsocks);
 #ifdef ISC_PLATFORM_USETHREADS
 		if (events[i].data.fd == manager->pipe_fds[0]) {
-			done = process_ctlfd(manager);
+			have_ctlevent = ISC_TRUE;
 			continue;
 		}
 #endif
@@ -3011,6 +3024,11 @@ process_fds(isc_socketmgr_t *manager, struct epoll_event *events, int nevents) {
 			   (events[i].events & EPOLLOUT) != 0);
 	}
 
+#ifdef ISC_PLATFORM_USETHREADS
+	if (have_ctlevent)
+		done = process_ctlfd(manager);
+#endif
+
 	return (done);
 }
 #elif defined(USE_DEVPOLL)
@@ -3018,6 +3036,9 @@ static isc_boolean_t
 process_fds(isc_socketmgr_t *manager, struct pollfd *events, int nevents) {
 	int i;
 	isc_boolean_t done = ISC_FALSE;
+#ifdef ISC_PLATFORM_USETHREADS
+	isc_boolean_t have_ctlevent = ISC_FALSE;
+#endif
 
 	if (nevents == manager->nevents) {
 		manager_log(manager, ISC_LOGCATEGORY_GENERAL,
@@ -3030,7 +3051,7 @@ process_fds(isc_socketmgr_t *manager, struct pollfd *events, int nevents) {
 		REQUIRE(events[i].fd < (int)manager->maxsocks);
 #ifdef ISC_PLATFORM_USETHREADS
 		if (events[i].fd == manager->pipe_fds[0]) {
-			done = process_ctlfd(manager);
+			have_ctlevent = ISC_TRUE;
 			continue;
 		}
 #endif
@@ -3039,6 +3060,11 @@ process_fds(isc_socketmgr_t *manager, struct pollfd *events, int nevents) {
 			   (events[i].events & POLLOUT) != 0);
 	}
 
+#ifdef ISC_PLATFORM_USETHREADS
+	if (have_ctlevent)
+		done = process_ctlfd(manager);
+#endif
+
 	return (done);
 }
 #elif defined(USE_SELECT)
@@ -3172,13 +3198,13 @@ watcher(void *uap) {
 #if defined(USE_KQUEUE) || defined (USE_EPOLL) || defined (USE_DEVPOLL)
 		done = process_fds(manager, manager->events, cc);
 #elif defined(USE_SELECT)
+		process_fds(manager, maxfd, &readfds, &writefds);
+
 		/*
 		 * Process reads on internal, control fd.
 		 */
 		if (FD_ISSET(ctlfd, &readfds))
 			done = process_ctlfd(manager);
-
-		process_fds(manager, maxfd, &readfds, &writefds);
 #endif
 	}
 
@@ -3684,7 +3710,7 @@ socket_recv(isc_socket_t *sock, isc_socketevent_t *dev, isc_task_t *task,
 		 * Enqueue the request.  If the socket was previously not being
 		 * watched, poke the watcher to start paying attention to it.
 		 */
-		if (ISC_LIST_EMPTY(sock->recv_list))
+		if (ISC_LIST_EMPTY(sock->recv_list) && !sock->pending_recv)
 			select_poke(sock->manager, sock->fd, SELECT_POKE_READ);
 		ISC_LIST_ENQUEUE(sock->recv_list, dev, ev_link);
 
@@ -3881,7 +3907,8 @@ socket_send(isc_socket_t *sock, isc_socketevent_t *dev, isc_task_t *task,
 			 * not being watched, poke the watcher to start
 			 * paying attention to it.
 			 */
-			if (ISC_LIST_EMPTY(sock->send_list))
+			if (ISC_LIST_EMPTY(sock->send_list) &&
+			    !sock->pending_send)
 				select_poke(sock->manager, sock->fd,
 					    SELECT_POKE_WRITE);
 			ISC_LIST_ENQUEUE(sock->send_list, dev, ev_link);