diff --git a/CHANGES b/CHANGES index 8f1f0cee58..c192c71af7 100644 --- a/CHANGES +++ b/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 diff --git a/bin/named/client.c b/bin/named/client.c index 3a5858bef6..5d7113a23e 100644 --- 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.194 2001/11/14 19:11:06 gson Exp $ */ +/* $Id: client.c,v 1.195 2001/11/14 21:56:58 gson Exp $ */ #include @@ -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); - 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) && + 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); } diff --git a/bin/named/include/named/client.h b/bin/named/include/named/client.h index a9178db334..5e5d282ebd 100644 --- 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.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;