Attach to the dns_dispatchmgr in the dns_view object

The dns_dispatchmgr object was only set in the dns_view object making it
prone to use-after-free in the dns_xfrin unit when shutting down named.

Remove dns_view_setdispatchmgr() and optionally pass the dispatchmgr
directly to dns_view_create() when it is attached and not just assigned,
so the dns_dispatchmgr doesn't cease to exist too early.

The dns_view_getdnsdispatchmgr() is now protected by the RCU lock, the
dispatchmgr reference is incremented, so the caller needs to detach from
it, and the function can return NULL in case the dns_view has been
already shut down.

diff --git a/bin/delv/delv.c b/bin/delv/delv.c
index e947abedec6334f209d691a2683b0fc9c142ebf7..d8f71d7e248f402aefd2de1a6b54e77811dd8df2 100644 (file)
--- a/bin/delv/delv.c
+++ b/bin/delv/delv.c
@@ -2148,7 +2148,8 @@ run_server(void *arg) {
        CHECK(ns_interfacemgr_create(mctx, sctx, loopmgr, netmgr, dispatchmgr,
                                     NULL, false, &interfacemgr));
 
-       CHECK(dns_view_create(mctx, dns_rdataclass_in, "_default", &view));
+       CHECK(dns_view_create(mctx, dispatchmgr, dns_rdataclass_in, "_default",
+                             &view));
        CHECK(dns_cache_create(loopmgr, dns_rdataclass_in, "", &cache));
        dns_view_setcache(view, cache, false);
        dns_cache_detach(&cache);
@@ -2164,7 +2165,6 @@ run_server(void *arg) {
        dns_view_initsecroots(view);
        CHECK(setup_dnsseckeys(NULL, view));
 
-       dns_view_setdispatchmgr(view, dispatchmgr);
        CHECK(dns_view_createresolver(view, loopmgr, 1, netmgr, 0,
                                      tlsctx_client_cache, dispatch, NULL));
 

diff --git a/bin/named/server.c b/bin/named/server.c
index c8b1bdbf816d773c27543490a97112bd3b209a98..6575d3cbcf8303c736764dbc9a221b3d8b7fa6ce 100644 (file)
--- a/bin/named/server.c
+++ b/bin/named/server.c
@@ -6423,13 +6423,12 @@ create_view(const cfg_obj_t *vconfig, dns_viewlist_t *viewlist,
        }
        INSIST(view == NULL);
 
-       result = dns_view_create(named_g_mctx, viewclass, viewname, &view);
+       result = dns_view_create(named_g_mctx, named_g_dispatchmgr, viewclass,
+                                viewname, &view);
        if (result != ISC_R_SUCCESS) {
                return (result);
        }
 
-       dns_view_setdispatchmgr(view, named_g_dispatchmgr);
-
        isc_nonce_buf(view->secret, sizeof(view->secret));
 
        ISC_LIST_APPEND(*viewlist, view, link);

diff --git a/bin/tests/system/pipelined/pipequeries.c b/bin/tests/system/pipelined/pipequeries.c
index b242118fc2a3c975f54f11eaa79e425d68d10049..bfe0c1b84aa598bbd16a956c106d23717a4019db 100644 (file)
--- a/bin/tests/system/pipelined/pipequeries.c
+++ b/bin/tests/system/pipelined/pipequeries.c
@@ -282,7 +282,7 @@ main(int argc, char *argv[]) {
        RUNCHECK(dns_requestmgr_create(mctx, loopmgr, dispatchmgr, dispatchv4,
                                       NULL, &requestmgr));
 
-       RUNCHECK(dns_view_create(mctx, 0, "_test", &view));
+       RUNCHECK(dns_view_create(mctx, NULL, 0, "_test", &view));
 
        isc_loopmgr_setup(loopmgr, sendqueries, NULL);
        isc_loopmgr_teardown(loopmgr, teardown_view, view);

diff --git a/bin/tools/mdig.c b/bin/tools/mdig.c
index 24ac710a562b18ae2bb03e46f2d380842cb83772..f88edf49f6f825c6f86af356481649faaf4fde44 100644 (file)
--- a/bin/tools/mdig.c
+++ b/bin/tools/mdig.c
@@ -2167,7 +2167,7 @@ main(int argc, char *argv[]) {
                mctx, loopmgr, dispatchmgr, have_ipv4 ? dispatchvx : NULL,
                have_ipv6 ? dispatchvx : NULL, &requestmgr));
 
-       RUNCHECK(dns_view_create(mctx, 0, "_mdig", &view));
+       RUNCHECK(dns_view_create(mctx, NULL, 0, "_mdig", &view));
 
        query = ISC_LIST_HEAD(queries);
        isc_loopmgr_setup(loopmgr, sendqueries, query);

diff --git a/fuzz/dns_message_checksig.c b/fuzz/dns_message_checksig.c
index f3385ab4a818c3e27fa3b94e221214429f3694cf..a253fa7027650b7cf841f89a2b495a906e09ab33 100644 (file)
--- a/fuzz/dns_message_checksig.c
+++ b/fuzz/dns_message_checksig.c
@@ -183,7 +183,7 @@ LLVMFuzzerInitialize(int *argc ISC_ATTR_UNUSED, char ***argv ISC_ATTR_UNUSED) {
 
        isc_loopmgr_create(mctx, 1, &loopmgr);
 
-       result = dns_view_create(mctx, dns_rdataclass_in, "view", &view);
+       result = dns_view_create(mctx, NULL, dns_rdataclass_in, "view", &view);
        if (result != ISC_R_SUCCESS) {
                fprintf(stderr, "dns_view_create failed: %s\n",
                        isc_result_totext(result));

diff --git a/lib/dns/client.c b/lib/dns/client.c
index 2eee2d34809d207dc6b5808d07102a758ec592da..205c9764bc1e5154b0fb1bb434cb4c853383d426 100644 (file)
--- a/lib/dns/client.c
+++ b/lib/dns/client.c
@@ -206,13 +206,12 @@ createview(isc_mem_t *mctx, dns_rdataclass_t rdclass, isc_loopmgr_t *loopmgr,
        isc_result_t result;
        dns_view_t *view = NULL;
 
-       result = dns_view_create(mctx, rdclass, DNS_CLIENTVIEW_NAME, &view);
+       result = dns_view_create(mctx, dispatchmgr, rdclass,
+                                DNS_CLIENTVIEW_NAME, &view);
        if (result != ISC_R_SUCCESS) {
                return (result);
        }
 
-       dns_view_setdispatchmgr(view, dispatchmgr);
-
        /* Initialize view security roots */
        dns_view_initsecroots(view);
 

diff --git a/lib/dns/include/dns/view.h b/lib/dns/include/dns/view.h
index f22cb876a296b78b17937ad87c28331453cda99a..aed9761679f31749db178c035ea4c5e788857912 100644 (file)
--- a/lib/dns/include/dns/view.h
+++ b/lib/dns/include/dns/view.h
@@ -259,8 +259,8 @@ struct dns_view {
 #endif /* HAVE_LMDB */
 
 isc_result_t
-dns_view_create(isc_mem_t *mctx, dns_rdataclass_t rdclass, const char *name,
-               dns_view_t **viewp);
+dns_view_create(isc_mem_t *mctx, dns_dispatchmgr_t *dispmgr,
+               dns_rdataclass_t rdclass, const char *name, dns_view_t **viewp);
 /*%<
  * Create a view.
  *
@@ -378,9 +378,6 @@ dns_view_createresolver(dns_view_t *view, isc_loopmgr_t *loopmgr,
  *
  *\li  'view' does not have a resolver already.
  *
- *\li  A dispatch manager has been associated with the view by calling
- *     dns_view_setdispatchmgr().
- *
  *\li  The requirements of dns_resolver_create() apply to 'ndisp',
  *     'netmgr', 'options', 'tlsctx_cache', 'dispatchv4', and 'dispatchv6'.
  *
@@ -1254,12 +1251,10 @@ dns_view_getudpsize(dns_view_t *view);
  * Get the current EDNS UDP buffer size.
  */
 
-void
-dns_view_setdispatchmgr(dns_view_t *view, dns_dispatchmgr_t *dispatchmgr);
 dns_dispatchmgr_t *
 dns_view_getdispatchmgr(dns_view_t *view);
 /*%<
- * Set/get the dispatch manager for the view; this wil be used
+ * Get the attached dispatch manager for the view; this will be used
  * by the resolver and request managers to send and receive DNS
  * messages.
  */

diff --git a/lib/dns/view.c b/lib/dns/view.c
index 8f08e108d7efee914c36effd54ca2783614c73bc..982c7518bca1c87880be890ba6842cc3423d2a35 100644 (file)
--- a/lib/dns/view.c
+++ b/lib/dns/view.c
@@ -79,7 +79,8 @@
 #define DEFAULT_EDNS_BUFSIZE 1232
 
 isc_result_t
-dns_view_create(isc_mem_t *mctx, dns_rdataclass_t rdclass, const char *name,
+dns_view_create(isc_mem_t *mctx, dns_dispatchmgr_t *dispatchmgr,
+               dns_rdataclass_t rdclass, const char *name,
                dns_view_t **viewp) {
        dns_view_t *view = NULL;
        isc_result_t result;
@@ -129,6 +130,10 @@ dns_view_create(isc_mem_t *mctx, dns_rdataclass_t rdclass, const char *name,
 
        isc_mem_attach(mctx, &view->mctx);
 
+       if (dispatchmgr != NULL) {
+               dns_dispatchmgr_attach(dispatchmgr, &view->dispatchmgr);
+       }
+
        isc_mutex_init(&view->lock);
 
        isc_rwlock_init(&view->sfd_lock);
@@ -442,6 +447,7 @@ dns_view_detach(dns_view_t **viewp) {
                dns_resolver_t *resolver = NULL;
                dns_adb_t *adb = NULL;
                dns_requestmgr_t *requestmgr = NULL;
+               dns_dispatchmgr_t *dispatchmgr = NULL;
 
                isc_refcount_destroy(&view->references);
 
@@ -477,6 +483,7 @@ dns_view_detach(dns_view_t **viewp) {
                        }
                }
                adb = rcu_xchg_pointer(&view->adb, NULL);
+               dispatchmgr = rcu_xchg_pointer(&view->dispatchmgr, NULL);
                rcu_read_unlock();
 
                if (view->requestmgr != NULL) {
@@ -510,14 +517,15 @@ dns_view_detach(dns_view_t **viewp) {
                if (resolver != NULL) {
                        dns_resolver_detach(&resolver);
                }
-               if (adb != NULL || zonetable != NULL) {
-                       synchronize_rcu();
-                       if (adb != NULL) {
-                               dns_adb_detach(&adb);
-                       }
-                       if (zonetable != NULL) {
-                               dns_zt_detach(&zonetable);
-                       }
+               synchronize_rcu();
+               if (dispatchmgr != NULL) {
+                       dns_dispatchmgr_detach(&dispatchmgr);
+               }
+               if (adb != NULL) {
+                       dns_adb_detach(&adb);
+               }
+               if (zonetable != NULL) {
+                       dns_zt_detach(&zonetable);
                }
                if (requestmgr != NULL) {
                        dns_requestmgr_detach(&requestmgr);
@@ -2411,16 +2419,18 @@ dns_view_getudpsize(dns_view_t *view) {
        return (view->udpsize);
 }
 
-void
-dns_view_setdispatchmgr(dns_view_t *view, dns_dispatchmgr_t *dispatchmgr) {
-       REQUIRE(DNS_VIEW_VALID(view));
-       view->dispatchmgr = dispatchmgr;
-}
-
 dns_dispatchmgr_t *
 dns_view_getdispatchmgr(dns_view_t *view) {
        REQUIRE(DNS_VIEW_VALID(view));
-       return (view->dispatchmgr);
+
+       rcu_read_lock();
+       dns_dispatchmgr_t *dispatchmgr = rcu_dereference(view->dispatchmgr);
+       if (dispatchmgr != NULL) {
+               dns_dispatchmgr_ref(dispatchmgr);
+       }
+       rcu_read_unlock();
+
+       return (dispatchmgr);
 }
 
 isc_result_t

diff --git a/lib/dns/xfrin.c b/lib/dns/xfrin.c
index b144c46663b4cd677865d910a3a100fbceb60568..8e04d23e7762d372b47640ad8c06406024267843 100644 (file)
--- a/lib/dns/xfrin.c
+++ b/lib/dns/xfrin.c
@@ -928,9 +928,15 @@ xfrin_start(dns_xfrin_t *xfr) {
        if (xfr->transport == NULL ||
            dns_transport_get_type(xfr->transport) == DNS_TRANSPORT_TCP)
        {
-               result = dns_dispatch_gettcp(dns_view_getdispatchmgr(xfr->view),
-                                            &xfr->primaryaddr,
-                                            &xfr->sourceaddr, &xfr->disp);
+               dns_dispatchmgr_t *dispmgr = dns_view_getdispatchmgr(xfr->view);
+               if (dispmgr == NULL) {
+                       result = ISC_R_SHUTTINGDOWN;
+               } else {
+                       result = dns_dispatch_gettcp(dispmgr, &xfr->primaryaddr,
+                                                    &xfr->sourceaddr,
+                                                    &xfr->disp);
+                       dns_dispatchmgr_detach(&dispmgr);
+               }
        }
        if (result == ISC_R_SUCCESS) {
                char peer[ISC_SOCKADDR_FORMATSIZE];
@@ -938,9 +944,16 @@ xfrin_start(dns_xfrin_t *xfr) {
                xfrin_log(xfr, ISC_LOG_DEBUG(1),
                          "attached to TCP connection to %s", peer);
        } else {
-               CHECK(dns_dispatch_createtcp(dns_view_getdispatchmgr(xfr->view),
-                                            &xfr->sourceaddr,
-                                            &xfr->primaryaddr, &xfr->disp));
+               dns_dispatchmgr_t *dispmgr = dns_view_getdispatchmgr(xfr->view);
+               if (dispmgr == NULL) {
+                       result = ISC_R_SHUTTINGDOWN;
+               } else {
+                       result = dns_dispatch_createtcp(
+                               dispmgr, &xfr->sourceaddr, &xfr->primaryaddr,
+                               &xfr->disp);
+                       dns_dispatchmgr_detach(&dispmgr);
+               }
+               CHECK(result);
        }
 
        /* Set the maximum timer */

diff --git a/tests/dns/dnstap_test.c b/tests/dns/dnstap_test.c
index ead4a83ef73217491230f9d9446b2e207daa7d4a..e25a5dfeff6528c1b460c978646e5cc614410f64 100644 (file)
--- a/tests/dns/dnstap_test.c
+++ b/tests/dns/dnstap_test.c
@@ -146,7 +146,7 @@ ISC_RUN_TEST_IMPL(dns_dt_send) {
        isc_time_t p, f;
        struct fstrm_iothr_options *fopt;
 
-       result = dns_test_makeview("test", false, &view);
+       result = dns_test_makeview("test", false, false, &view);
        assert_int_equal(result, ISC_R_SUCCESS);
 
        fopt = fstrm_iothr_options_init();

diff --git a/tests/dns/keytable_test.c b/tests/dns/keytable_test.c
index f3657c6649b0a171c85d72b3923414ffa966b2a3..2442876340e82d84ac2ab955a256f5055fa9e4bf 100644 (file)
--- a/tests/dns/keytable_test.c
+++ b/tests/dns/keytable_test.c
@@ -170,7 +170,7 @@ create_tables(void) {
        dns_name_t *keyname = dns_fixedname_name(&fn);
        isc_stdtime_t now = isc_stdtime_now();
 
-       assert_int_equal(dns_test_makeview("view", false, &view),
+       assert_int_equal(dns_test_makeview("view", false, false, &view),
                         ISC_R_SUCCESS);
 
        dns_keytable_create(view, &keytable);
@@ -602,7 +602,7 @@ ISC_LOOP_TEST_IMPL(nta) {
        dns_view_t *myview = NULL;
        isc_stdtime_t now = isc_stdtime_now();
 
-       result = dns_test_makeview("view", false, &myview);
+       result = dns_test_makeview("view", false, false, &myview);
        assert_int_equal(result, ISC_R_SUCCESS);
 
        dns_view_initsecroots(myview);

diff --git a/tests/dns/resolver_test.c b/tests/dns/resolver_test.c
index e9d133e3fccc2b4d3aafb6fa8458854a35bb7629..09106aadd6b4ccd91a08d025f68866bec7941403 100644 (file)
--- a/tests/dns/resolver_test.c
+++ b/tests/dns/resolver_test.c
@@ -35,7 +35,6 @@
 
 #include <tests/dns.h>
 
-static dns_dispatchmgr_t *dispatchmgr = NULL;
 static dns_dispatch_t *dispatch = NULL;
 static dns_view_t *view = NULL;
 static isc_tlsctx_cache_t *tlsctx_cache = NULL;
@@ -44,21 +43,22 @@ static int
 setup_test(void **state) {
        isc_result_t result;
        isc_sockaddr_t local;
+       dns_dispatchmgr_t *dispatchmgr = NULL;
 
        setup_managers(state);
 
-       result = dns_dispatchmgr_create(mctx, netmgr, &dispatchmgr);
+       result = dns_test_makeview("view", true, false, &view);
        assert_int_equal(result, ISC_R_SUCCESS);
 
-       result = dns_test_makeview("view", false, &view);
-       assert_int_equal(result, ISC_R_SUCCESS);
-
-       dns_view_setdispatchmgr(view, dispatchmgr);
+       dispatchmgr = dns_view_getdispatchmgr(view);
+       assert_non_null(dispatchmgr);
 
        isc_sockaddr_any(&local);
        result = dns_dispatch_createudp(dispatchmgr, &local, &dispatch);
        assert_int_equal(result, ISC_R_SUCCESS);
 
+       dns_dispatchmgr_detach(&dispatchmgr);
+
        return (0);
 }
 
@@ -66,7 +66,6 @@ static int
 teardown_test(void **state) {
        dns_dispatch_detach(&dispatch);
        dns_view_detach(&view);
-       dns_dispatchmgr_detach(&dispatchmgr);
        teardown_managers(state);
 
        return (0);

diff --git a/tests/include/tests/dns.h b/tests/include/tests/dns.h
index 4a3896e4e06e37a0d4e48c2db4cd3741a852b799..22a626f35e22fd645f2f00d223700ac9d3bda1a7 100644 (file)
--- a/tests/include/tests/dns.h
+++ b/tests/include/tests/dns.h
@@ -50,7 +50,8 @@ typedef struct {
        }
 
 isc_result_t
-dns_test_makeview(const char *name, bool with_cache, dns_view_t **viewp);
+dns_test_makeview(const char *name, bool with_dispatchmgr, bool with_cache,
+                 dns_view_t **viewp);
 
 /*%
  * Create a zone with origin 'name', return a pointer to the zone object in

diff --git a/tests/libtest/dns.c b/tests/libtest/dns.c
index 4c8880fea7976b76499d49c71033e50776299a08..de0545be8b1805813cf154c2a0077f2f8b4142db 100644 (file)
--- a/tests/libtest/dns.c
+++ b/tests/libtest/dns.c
@@ -45,6 +45,7 @@
 
 #include <dns/callbacks.h>
 #include <dns/db.h>
+#include <dns/dispatch.h>
 #include <dns/fixedname.h>
 #include <dns/log.h>
 #include <dns/name.h>
@@ -59,12 +60,27 @@ dns_zonemgr_t *zonemgr = NULL;
  * Create a view.
  */
 isc_result_t
-dns_test_makeview(const char *name, bool with_cache, dns_view_t **viewp) {
+dns_test_makeview(const char *name, bool with_dispatchmgr, bool with_cache,
+                 dns_view_t **viewp) {
        isc_result_t result;
        dns_view_t *view = NULL;
        dns_cache_t *cache = NULL;
+       dns_dispatchmgr_t *dispatchmgr = NULL;
+
+       if (with_dispatchmgr) {
+               result = dns_dispatchmgr_create(mctx, netmgr, &dispatchmgr);
+               if (result != ISC_R_SUCCESS) {
+                       return (result);
+               }
+       }
+
+       result = dns_view_create(mctx, dispatchmgr, dns_rdataclass_in, name,
+                                &view);
+
+       if (dispatchmgr != NULL) {
+               dns_dispatchmgr_detach(&dispatchmgr);
+       }
 
-       result = dns_view_create(mctx, dns_rdataclass_in, name, &view);
        if (result != ISC_R_SUCCESS) {
                return (result);
        }
@@ -124,7 +140,7 @@ dns_test_makezone(const char *name, dns_zone_t **zonep, dns_view_t *view,
         * If requested, create a view.
         */
        if (createview) {
-               result = dns_test_makeview("view", false, &view);
+               result = dns_test_makeview("view", false, false, &view);
                if (result != ISC_R_SUCCESS) {
                        goto detach_zone;
                }

diff --git a/tests/libtest/ns.c b/tests/libtest/ns.c
index 0952de6f36ceb23335ba5d91fcabc781e03eb531..8f7f1302533af023efd08765e91767e300df1c0e 100644 (file)
--- a/tests/libtest/ns.c
+++ b/tests/libtest/ns.c
@@ -439,7 +439,8 @@ ns_test_qctx_create(const ns_test_qctx_create_params_t *params,
        /*
         * Every client needs to belong to a view.
         */
-       result = dns_test_makeview("view", params->with_cache, &client->view);
+       result = dns_test_makeview("view", false, params->with_cache,
+                                  &client->view);
        if (result != ISC_R_SUCCESS) {
                goto detach_client;
        }

diff --git a/tests/ns/notify_test.c b/tests/ns/notify_test.c
index 863a4a0692b8c9f9e693de25508c2ddc62c0300d..96fb3c26a43897dc0c95e30c463f2b273b9418f7 100644 (file)
--- a/tests/ns/notify_test.c
+++ b/tests/ns/notify_test.c
@@ -70,7 +70,7 @@ ISC_LOOP_TEST_IMPL(notify_start) {
        result = ns_test_getclient(NULL, false, &client);
        assert_int_equal(result, ISC_R_SUCCESS);
 
-       result = dns_test_makeview("view", false, &client->view);
+       result = dns_test_makeview("view", false, false, &client->view);
        assert_int_equal(result, ISC_R_SUCCESS);
 
        result = ns_test_serve_zone("example.com",