Never close a file descriptor that is monitored using

poll()/select() from another thread.
It introduces subtle bugs.

git-svn-id: file:///var/lib/svn/clamav-devel/branches/clamd-proto@4742 77e5149b-7576-45b1-b177-96237e5ba77b

ChangeLog

@@ -1,3 +1,9 @@
+Thu Feb 12 11:37:29 EET 2009 (edwin)
+------------------------------------
+ * clamd/others.c, clamd/server-th.c: Never close a file descriptor
+ that is monitored using poll()/select() from another thread.  It
+ introduces subtle bugs.
+
 Wed Feb 11 17:05:08 EET 2009 (edwin)
 ------------------------------------
  * clamd/others.c, clamd/scanner.c, clamd/server-th.c: fix some races

clamd/others.c

@@ -449,6 +449,9 @@ int fds_poll_recv(struct fd_data *data, int timeout, int check_signals)
 		    continue;
 		}
 		revents = data->poll_data[i].revents;
+		if (revents & (POLLIN|POLLHUP)) {
+		    logg("*POLL: POLLIN|POLLHUP on fd %d\n",data->poll_data[i].fd);
+		}
 		if (revents & POLLIN) {
 		    int ret = read_fd_data(&data->buf[i]);
 		    /* Data available to be read */
@@ -461,7 +464,8 @@ int fds_poll_recv(struct fd_data *data, int timeout, int check_signals)
 		if (revents & (POLLHUP | POLLERR | POLLNVAL)) {
 		    if (revents & (POLLHUP| POLLNVAL)) {
 			/* remote disconnected */
-			logg("^poll_recv_fds: Client disconnected\n");
+			logg("^poll_recv_fds: Client disconnected (FD %d)\n",
+			     data->poll_data[i].fd);
 		    } else {
 			/* error on file descriptor */
 			logg("!poll_recv_fds: Error condition on fd %d\n",

clamd/server-th.c

@@ -876,6 +876,31 @@ int recvloop_th(int *socketds, unsigned nsockets, struct cl_engine *engine, unsi
 			    logg("*RECVTH: mode -> MODE_WAITREPLY\n");
 			    /* no more commands are accepted */
 			    conn.mode = MODE_WAITREPLY;
+			    /* Stop monitoring this FD, it will be closed either
+			     * by us, or by the scanner thread. 
+			     * Never close a file descriptor that is being
+			     * monitored by poll()/select() from another thread,
+			     * because this can lead to subtle bugs such as:
+			     * Other thread closes file descriptor -> POLLHUP is
+			     * set, but the poller thread doesn't wake up yet.
+			     * Another client opens a connection and sends some
+			     * data. If the socket reuses the previous file descriptor,
+			     * then POLLIN is set on the file descriptor too.
+			     * When poll() wakes up it sees POLLIN | POLLHUP
+			     * and thinks that the client has sent some data,
+			     * and closed the connection, so clamd closes the
+			     * connection in turn resulting in a bug.
+			     *
+			     * If we wouldn't have poll()-ed the file descriptor
+			     * we closed in another thread, but rather made sure
+			     * that we don't put a FD that we're about to close
+			     * into poll()'s list of watched fds; then POLLHUP
+			     * would be set, but the file descriptor would stay
+			     * open, until we wake up from poll() and close it.
+			     * Thus a new connection won't be able to reuse the
+			     * same FD, and there is no bug.
+			     * */
+			    buf->fd = -1;
 			}
 		    }
 		    pos += cmdlen+1;