3370. [bug] Address use after free while shutting down. [RT #30241]

diff --git a/CHANGES b/CHANGES
index 7585c87a5b9040a83b2a824bcfea218149290ee7..b0853740a6e89425203728e3724cad1594004043 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -1,4 +1,6 @@
-3379.  [bug]           nsupdate terminated unexpectedly in interactive mode
+3370.  [bug]           Address use after free while shutting down. [RT #30241]
+
+3369.  [bug]           nsupdate terminated unexpectedly in interactive mode
                        if built with readline support. [RT #29550]
 
 3368.  [bug]           <dns/iptable.h>, <dns/private.h> and <dns/zone.h>

diff --git a/bin/named/client.c b/bin/named/client.c
index bb59200970910d0ea375dfc6f5bdca94e72c39eb..e8654cb097e010ab9dff8077a6f2865eca7cd792 100644 (file)
--- a/bin/named/client.c
+++ b/bin/named/client.c
@@ -501,12 +501,14 @@ exit_check(ns_client_t *client) {
                 * the dying client inbetween.
                 */
                client->state = NS_CLIENTSTATE_INACTIVE;
-               if (!ns_g_clienttest)
-                       ISC_QUEUE_PUSH(manager->inactive, client, ilink);
                INSIST(client->recursionquota == NULL);
 
                if (client->state == client->newstate) {
                        client->newstate = NS_CLIENTSTATE_MAX;
+                       if (!ns_g_clienttest && manager != NULL &&
+                           !manager->exiting)
+                               ISC_QUEUE_PUSH(manager->inactive, client,
+                                              ilink);
                        if (client->needshutdown)
                                isc_task_shutdown(client->task);
                        return (ISC_TRUE);
@@ -526,6 +528,7 @@ exit_check(ns_client_t *client) {
                REQUIRE(client->state == NS_CLIENTSTATE_INACTIVE);
 
                INSIST(client->recursionquota == NULL);
+               INSIST(!ISC_QLINK_LINKED(client, ilink));
 
                ns_query_free(client);
                isc_mem_put(client->mctx, client->recvbuf, RECV_BUFFER_SIZE);
@@ -634,6 +637,9 @@ client_shutdown(isc_task_t *task, isc_event_t *event) {
                client->shutdown_arg = NULL;
        }
 
+       if (ISC_QLINK_LINKED(client, ilink))
+               ISC_QUEUE_UNLINK(client->manager->inactive, client, ilink);
+
        client->newstate = NS_CLIENTSTATE_FREED;
        client->needshutdown = ISC_FALSE;
        (void)exit_check(client);
@@ -2120,6 +2126,8 @@ client_create(ns_clientmgr_t *manager, ns_client_t **clientp) {
 #ifdef ALLOW_FILTER_AAAA_ON_V4
        client->filter_aaaa = dns_v4_aaaa_ok;
 #endif
+       client->needshutdown = ns_g_clienttest;
+
        ISC_EVENT_INIT(&client->ctlevent, sizeof(client->ctlevent), 0, NULL,
                       NS_EVENT_CLIENTCONTROL, client_start, client, client,
                       NULL, NULL);
@@ -2147,8 +2155,6 @@ client_create(ns_clientmgr_t *manager, ns_client_t **clientp) {
        if (result != ISC_R_SUCCESS)
                goto cleanup_query;
 
-       client->needshutdown = ns_g_clienttest;
-
        CTRACE("create");
 
        *clientp = client;
@@ -2514,9 +2520,10 @@ ns_clientmgr_create(isc_mem_t *mctx, isc_taskmgr_t *taskmgr,
 
 void
 ns_clientmgr_destroy(ns_clientmgr_t **managerp) {
+       isc_result_t result;
        ns_clientmgr_t *manager;
        ns_client_t *client;
-       isc_boolean_t need_destroy = ISC_FALSE;
+       isc_boolean_t need_destroy = ISC_FALSE, unlock = ISC_FALSE;
 
        REQUIRE(managerp != NULL);
        manager = *managerp;
@@ -2524,11 +2531,16 @@ ns_clientmgr_destroy(ns_clientmgr_t **managerp) {
 
        MTRACE("destroy");
 
-       LOCK(&manager->listlock);
+       /*
+        * Check for success because we may already be task-exclusive
+        * at this point.  Only if we succeed at obtaining an exclusive
+        * lock now will we need to relinquish it later.
+        */
+       result = isc_task_beginexclusive(ns_g_server->task);
+       if (result == ISC_R_SUCCESS)
+               unlock = ISC_TRUE;
 
-       LOCK(&manager->lock);
        manager->exiting = ISC_TRUE;
-       UNLOCK(&manager->lock);
 
        for (client = ISC_LIST_HEAD(manager->clients);
             client != NULL;
@@ -2538,7 +2550,8 @@ ns_clientmgr_destroy(ns_clientmgr_t **managerp) {
        if (ISC_LIST_EMPTY(manager->clients))
                need_destroy = ISC_TRUE;
 
-       UNLOCK(&manager->listlock);
+       if (unlock)
+               isc_task_endexclusive(ns_g_server->task);
 
        if (need_destroy)
                clientmgr_destroy(manager);
@@ -2555,6 +2568,9 @@ get_client(ns_clientmgr_t *manager, ns_interface_t *ifp,
        ns_client_t *client;
        MTRACE("get client");
 
+       if (manager != NULL && manager->exiting)
+               return (ISC_R_SHUTTINGDOWN);
+
        /*
         * Allocate a client.  First try to get a recycled one;
         * if that fails, make a new one.

diff --git a/lib/isc/include/isc/queue.h b/lib/isc/include/isc/queue.h
index 1bc9d5b4d17a88501572fb1fa6fef8f8e5d78d94..22e2a01e3c4783a0a8786ef218cf2630c9d834ab 100644 (file)
--- a/lib/isc/include/isc/queue.h
+++ b/lib/isc/include/isc/queue.h
@@ -35,7 +35,7 @@
 #define ISC_QLINK_INSIST(x) (void)0
 #endif
 
-#define ISC_QLINK(type) struct { void *prev, *next; }
+#define ISC_QLINK(type) struct { type *prev, *next; }
 
 #define ISC_QLINK_INIT(elt, link) \
        do { \
@@ -142,4 +142,22 @@
                        (ret)->link.next = (ret)->link.prev = (void *)(-1); \
        } while(0)
 
+#define ISC_QUEUE_UNLINK(queue, elt, link) \
+       do { \
+               ISC_QLINK_INSIST(ISC_QLINK_LINKED(elt, link)); \
+               LOCK(&(queue).headlock); \
+               LOCK(&(queue).taillock); \
+               if ((elt)->link.prev == NULL) \
+                       (queue).head = (elt)->link.next; \
+               else \
+                       (elt)->link.prev->link.next = (elt)->link.next; \
+               if ((elt)->link.next == NULL) \
+                       (queue).tail = (elt)->link.prev; \
+               else \
+                       (elt)->link.next->link.prev = (elt)->link.prev; \
+               UNLOCK(&(queue).taillock); \
+               UNLOCK(&(queue).headlock); \
+               (elt)->link.next = (elt)->link.prev = (void *)(-1); \
+       } while(0)
+
 #endif /* ISC_QUEUE_H */

diff --git a/lib/isc/tests/queue_test.c b/lib/isc/tests/queue_test.c
index 74620fef42aa3e9a2efd92ff65e2f26d812b3632..c4a80c4c54b287b7cbc6c255b556cbb9490eeeae 100644 (file)
--- a/lib/isc/tests/queue_test.c
+++ b/lib/isc/tests/queue_test.c
@@ -29,10 +29,11 @@
 
 #include "isctest.h"
 
-typedef struct item {
+typedef struct item item_t;
+struct item {
        int                     value;
        ISC_QLINK(item_t)       qlink;
-} item_t;
+};
 
 typedef ISC_QUEUE(item_t) item_queue_t;
 
@@ -107,6 +108,9 @@ ATF_TC_BODY(queue_valid, tc) {
        ISC_QUEUE_PUSH(queue, &five, qlink);
        ATF_CHECK(ISC_QLINK_LINKED(&five, qlink));
 
+       /* Test unlink by removing one item from the middle */
+       ISC_QUEUE_UNLINK(queue, &three, qlink);
+
        ISC_QUEUE_POP(queue, qlink, p);
        ATF_REQUIRE(p != NULL);
        ATF_CHECK_EQ(p->value, 1);
@@ -115,10 +119,6 @@ ATF_TC_BODY(queue_valid, tc) {
        ATF_REQUIRE(p != NULL);
        ATF_CHECK_EQ(p->value, 2);
 
-       ISC_QUEUE_POP(queue, qlink, p);
-       ATF_REQUIRE(p != NULL);
-       ATF_CHECK_EQ(p->value, 3);
-
        ISC_QUEUE_POP(queue, qlink, p);
        ATF_REQUIRE(p != NULL);
        ATF_CHECK_EQ(p->value, 4);