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

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

diff --git a/CHANGES b/CHANGES
index c0f04ea9b8e2a3ab92979aceb5cb27def48aa42d..412eb5ab5cedaefc88ebfb27ab8d1483faf87f5c 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -1,3 +1,6 @@
+1126.  [bug]           The server could access a freed event if shut
+                       down while a client start event was pending
+                       delivery. [RT #2061]
 
        --- 9.2.0rc10 released ---
 

diff --git a/bin/named/client.c b/bin/named/client.c
index 352042b342d9ec018deb513479e62d7752e347a4..73906288e32d3e0189995a5f26cdd47ec8df1a57 100644 (file)
--- a/bin/named/client.c
+++ b/bin/named/client.c
@@ -15,7 +15,7 @@
  * WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
  */
 
-/* $Id: client.c,v 1.176.2.6 2001/11/15 01:24:15 marka Exp $ */
+/* $Id: client.c,v 1.176.2.7 2001/11/15 02:51:44 marka Exp $ */
 
 #include <config.h>
 
@@ -173,104 +173,6 @@ static void client_start(isc_task_t *task, isc_event_t *event);
 static void client_request(isc_task_t *task, isc_event_t *event);
 static void ns_client_dumpmessage(ns_client_t *client, const char *reason);
 
-/*
- * Enter the inactive state.
- *
- * Requires:
- *     No requests are outstanding.
- */
-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->manager->active, 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))
-                       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
 ns_client_settimeout(ns_client_t *client, unsigned int seconds) {
        isc_result_t result;
@@ -297,6 +199,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)
@@ -436,12 +341,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;
                }
        }
 
@@ -449,11 +389,71 @@ 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);
-               return (ISC_TRUE);
+               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) {
+                       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))
+                               destroy_manager = manager;
+               }
+               /*
+                * 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);
 }
 
@@ -469,6 +469,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 {
@@ -1527,6 +1533,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;
@@ -1997,6 +2004,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);
        }

diff --git a/bin/named/include/named/client.h b/bin/named/include/named/client.h
index b76f8ce4db21934d7a86cb5c9be661646b40db9c..9ae040c9de5dad0a097024c38c8f240d24966564 100644 (file)
--- a/bin/named/include/named/client.h
+++ b/bin/named/include/named/client.h
@@ -15,7 +15,7 @@
  * WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
  */
 
-/* $Id: client.h,v 1.60.2.1 2001/11/15 01:24:18 marka Exp $ */
+/* $Id: client.h,v 1.60.2.2 2001/11/15 02:51:46 marka 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;