1126. [bug] The server could access a freed event if shut

down while a client start event was pending
                        delivery. [RT #2061]

CHANGES

@@ -1,3 +1,7 @@
+1126.	[bug]		The server could access a freed event if shut
+			down while a client start event was pending
+			delivery. [RT #2061]
+
 1125.	[bug]		rndc: -k missing from usage. [RT #2057]
 
 1124.	[doc]		dig: +[no]dnssec, +[no]besteffort and +[no]fail

bin/named/client.c

@@ -15,7 +15,7 @@
  * WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
  */
 
-/* $Id: client.c,v 1.194 2001/11/14 19:11:06 gson Exp $ */
+/* $Id: client.c,v 1.195 2001/11/14 21:56:58 gson Exp $ */
 
 #include <config.h>
 
@@ -205,93 +205,6 @@ static void
 client_deactivate(ns_client_t *client) {
 	REQUIRE(NS_CLIENT_VALID(client));
 
-	if (client->interface)
-		ns_interface_detach(&client->interface);
-
-	INSIST(client->naccepts == 0);
-	INSIST(client->recursionquota == NULL);
-	if (client->tcplistener != NULL)
-		isc_socket_detach(&client->tcplistener);
-
-	if (client->udpsocket != NULL)
-		isc_socket_detach(&client->udpsocket);
-
-	if (client->dispatch != NULL)
-		dns_dispatch_detach(&client->dispatch);
-
-	client->attributes = 0;
-	client->mortal = ISC_FALSE;
-
-	LOCK(&client->manager->lock);
-	ISC_LIST_UNLINK(*client->list, client, link);
-	ISC_LIST_APPEND(client->manager->inactive, client, link);
-	client->list = &client->manager->inactive;
-	UNLOCK(&client->manager->lock);
-}
-
-/*
- * Clean up a client object and free its memory.
- * Requires:
- *   The client is in the inactive state.
- */
-
-static void
-client_free(ns_client_t *client) {
-	isc_boolean_t need_clientmgr_destroy = ISC_FALSE;
-	ns_clientmgr_t *manager = NULL;
-
-	REQUIRE(NS_CLIENT_VALID(client));
-
-	/*
-	 * When "shuttingdown" is true, either the task has received
-	 * its shutdown event or no shutdown event has ever been
-	 * set up.  Thus, we have no outstanding shutdown
-	 * event at this point.
-	 */
-	REQUIRE(client->state == NS_CLIENTSTATE_INACTIVE);
-
-	INSIST(client->recursionquota == NULL);
-
-	ns_query_free(client);
-	isc_mem_put(client->mctx, client->recvbuf, RECV_BUFFER_SIZE);
-	isc_event_free((isc_event_t **)&client->sendevent);
-	isc_event_free((isc_event_t **)&client->recvevent);
-	isc_timer_detach(&client->timer);
-
-	if (client->tcpbuf != NULL)
-		isc_mem_put(client->mctx, client->tcpbuf, TCP_BUFFER_SIZE);
-	if (client->opt != NULL) {
-		INSIST(dns_rdataset_isassociated(client->opt));
-		dns_rdataset_disassociate(client->opt);
-		dns_message_puttemprdataset(client->message, &client->opt);
-	}
-	dns_message_destroy(&client->message);
-	if (client->manager != NULL) {
-		manager = client->manager;
-		LOCK(&manager->lock);
-		ISC_LIST_UNLINK(*client->list, client, link);
-		client->list = NULL;
-		if (manager->exiting &&
-		    ISC_LIST_EMPTY(manager->active) &&
-		    ISC_LIST_EMPTY(manager->inactive) &&
-		    ISC_LIST_EMPTY(manager->recursing))
-			need_clientmgr_destroy = ISC_TRUE;
-		UNLOCK(&manager->lock);
-	}
-	/*
-	 * Detaching the task must be done after unlinking from
-	 * the manager's lists because the manager accesses
-	 * client->task.
-	 */
-	if (client->task != NULL)
-		isc_task_detach(&client->task);
-
-	CTRACE("free");
-	client->magic = 0;
-	isc_mem_put(client->mctx, client, sizeof(*client));
-
-	if (need_clientmgr_destroy)
-		clientmgr_destroy(manager);
 }
 
 void
@@ -320,6 +233,9 @@ ns_client_settimeout(ns_client_t *client, unsigned int seconds) {
  */
 static isc_boolean_t
 exit_check(ns_client_t *client) {
+	ns_clientmgr_t *locked_manager = NULL;
+	ns_clientmgr_t *destroy_manager = NULL;
+
 	REQUIRE(NS_CLIENT_VALID(client));
 
 	if (client->state <= client->newstate)
@@ -459,12 +375,47 @@ exit_check(ns_client_t *client) {
 		}
 		/* Recv cancel is complete. */
 
-		client_deactivate(client);
+		if (client->nctls > 0) {
+			/* Still waiting for control event to be delivered */
+			return (ISC_TRUE);
+		}
+
+		/* Deactivate the client. */
+		if (client->interface)
+			ns_interface_detach(&client->interface);
+
+		INSIST(client->naccepts == 0);
+		INSIST(client->recursionquota == NULL);
+		if (client->tcplistener != NULL)
+			isc_socket_detach(&client->tcplistener);
+
+		if (client->udpsocket != NULL)
+			isc_socket_detach(&client->udpsocket);
+
+		if (client->dispatch != NULL)
+			dns_dispatch_detach(&client->dispatch);
+
+		client->attributes = 0;
+		client->mortal = ISC_FALSE;
+
+		LOCK(&client->manager->lock);
+		/*
+		 * Put the client on the inactive list.  If we are aiming for
+		 * the "freed" state, it will be removed from the inactive
+		 * list shortly, and we need to keep the manager locked until
+		 * that has been done, lest the manager decide to reactivate
+		 * the dying client inbetween.
+		 */
+		locked_manager = client->manager;
+		ISC_LIST_UNLINK(*client->list, client, link);
+		ISC_LIST_APPEND(client->manager->inactive, client, link);
+		client->list = &client->manager->inactive;
 		client->state = NS_CLIENTSTATE_INACTIVE;
 		INSIST(client->recursionquota == NULL);
+
 		if (client->state == client->newstate) {
 			client->newstate = NS_CLIENTSTATE_MAX;
-			return (ISC_TRUE); /* We're done. */
+			goto unlock;
 		}
 	}
 
@@ -472,11 +423,73 @@ exit_check(ns_client_t *client) {
 		INSIST(client->newstate == NS_CLIENTSTATE_FREED);
 		/*
 		 * We are trying to free the client.
+		 *
+		 * When "shuttingdown" is true, either the task has received
+		 * its shutdown event or no shutdown event has ever been
+		 * set up.  Thus, we have no outstanding shutdown
+		 * event at this point.
 		 */
-		client_free(client);
+		REQUIRE(client->state == NS_CLIENTSTATE_INACTIVE);
-		return (ISC_TRUE);
+
+		INSIST(client->recursionquota == NULL);
+
+		ns_query_free(client);
+		isc_mem_put(client->mctx, client->recvbuf, RECV_BUFFER_SIZE);
+		isc_event_free((isc_event_t **)&client->sendevent);
+		isc_event_free((isc_event_t **)&client->recvevent);
+		isc_timer_detach(&client->timer);
+
+		if (client->tcpbuf != NULL)
+			isc_mem_put(client->mctx, client->tcpbuf, TCP_BUFFER_SIZE);
+		if (client->opt != NULL) {
+			INSIST(dns_rdataset_isassociated(client->opt));
+			dns_rdataset_disassociate(client->opt);
+			dns_message_puttemprdataset(client->message, &client->opt);
+		}
+		dns_message_destroy(&client->message);
+		if (client->manager != NULL) {
+			ns_clientmgr_t *manager = client->manager;
+			if (locked_manager == NULL) {
+				LOCK(&manager->lock);
+				locked_manager = manager;
+			}
+			ISC_LIST_UNLINK(*client->list, client, link);
+			client->list = NULL;
+			if (manager->exiting &&
+			    ISC_LIST_EMPTY(manager->active) &&
+			    ISC_LIST_EMPTY(manager->inactive) &&
+			    ISC_LIST_EMPTY(manager->recursing))
+				destroy_manager = manager;
+			UNLOCK(&manager->lock);
+		}
+		/*
+		 * Detaching the task must be done after unlinking from
+		 * the manager's lists because the manager accesses
+		 * client->task.
+		 */
+		if (client->task != NULL)
+			isc_task_detach(&client->task);
+
+		CTRACE("free");
+		client->magic = 0;
+		isc_mem_put(client->mctx, client, sizeof(*client));
+
+		goto unlock;
 	}
 
+ unlock:
+	if (locked_manager != NULL) {
+		UNLOCK(&locked_manager->lock);
+		locked_manager = NULL;
+	}
+
+	/*
+	 * Only now is it safe to destroy the client manager (if needed),
+	 * because we have accessed its lock for the last time.
+	 */
+	if (destroy_manager != NULL)
+		clientmgr_destroy(destroy_manager);
+
 	return (ISC_TRUE);
 }
 
@@ -492,6 +505,12 @@ client_start(isc_task_t *task, isc_event_t *event) {
 
 	UNUSED(task);
 
+	INSIST(client->nctls == 1);
+	client->nctls--;
+
+	if (exit_check(client))
+		return;
+
 	if (TCP_CLIENT(client)) {
 		client_accept(client);
 	} else {
@@ -1556,6 +1575,7 @@ client_create(ns_clientmgr_t *manager, ns_client_t **clientp)
 	client->nreads = 0;
 	client->nsends = 0;
 	client->nrecvs = 0;
+	client->nctls = 0;
 	client->references = 0;
 	client->attributes = 0;
 	client->view = NULL;
@@ -2034,6 +2054,8 @@ ns_clientmgr_createclients(ns_clientmgr_t *manager, unsigned int n,
 		ISC_LIST_APPEND(manager->active, client, link);
 		client->list = &manager->active;
 
+		INSIST(client->nctls == 0);
+		client->nctls++;
 		ev = &client->ctlevent;
 		isc_task_send(client->task, &ev);
 	}

bin/named/include/named/client.h

@@ -15,7 +15,7 @@
  * WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
  */
 
-/* $Id: client.h,v 1.62 2001/10/24 03:10:17 marka Exp $ */
+/* $Id: client.h,v 1.63 2001/11/14 22:00:22 gson Exp $ */
 
 #ifndef NAMED_CLIENT_H
 #define NAMED_CLIENT_H 1
@@ -91,6 +91,7 @@ struct ns_client {
 	int			nreads;
 	int			nsends;
 	int			nrecvs;
+	int			nctls;
 	int			references;
 	unsigned int		attributes;
 	isc_task_t *		task;