Fix a data race between dns_zone_getxfr() and dns_xfrin_create()

There is a data race between the statistics channel, which uses
`dns_zone_getxfr()` to get a reference to `zone->xfr`, and the creation
of `zone->xfr`, because the latter happens outside of a zone lock.

Split the `dns_xfrin_create()` function into two parts to separate the
zone tranfer startring part from the zone transfer object creation part.
This allows us to attach the new object to a local variable first, then
attach it to `zone->xfr` under a lock, and only then start the transfer.

diff --git a/lib/dns/include/dns/xfrin.h b/lib/dns/include/dns/xfrin.h
index a024ce9ca3fe8e7e927383f9a344a31c8af7f255..04ad65072d002a380350b4af29820bffa3deeae8 100644 (file)
--- a/lib/dns/include/dns/xfrin.h
+++ b/lib/dns/include/dns/xfrin.h
@@ -51,26 +51,20 @@ typedef struct dns_xfrin dns_xfrin_t;
 
 ISC_LANG_BEGINDECLS
 
-isc_result_t
+void
 dns_xfrin_create(dns_zone_t *zone, dns_rdatatype_t xfrtype,
                 uint32_t ixfr_maxdiffs, const isc_sockaddr_t *primaryaddr,
                 const isc_sockaddr_t *sourceaddr, dns_tsigkey_t *tsigkey,
                 dns_transport_type_t soa_transport_type,
                 dns_transport_t *transport, isc_tlsctx_cache_t *tlsctx_cache,
-                isc_mem_t *mctx, dns_xfrindone_t done, dns_xfrin_t **xfrp);
+                isc_mem_t *mctx, dns_xfrin_t **xfrp);
 /*%<
- * Attempt to start an incoming zone transfer of 'zone'
- * from 'primaryaddr', creating a dns_xfrin_t object to
- * manage it.  Attach '*xfrp' to the newly created object.
- *
- * Iff ISC_R_SUCCESS is returned, '*done' is called with
- * 'zone' and a result code as arguments when the transfer finishes.
+ * Create an incoming zone transfer object of 'zone' from
+ * 'primaryaddr'.  Attach '*xfrp' to the newly created object.
  *
  * Requires:
  *\li  'xfrp' != NULL and '*xfrp' == NULL.
  *
- *\li  'done' != NULL.
- *
  *\li  'primaryaddr' has a non-zero port number.
  *
  *\li  'zone' is a valid zone and is associated with a view.
@@ -90,6 +84,22 @@ dns_xfrin_create(dns_zone_t *zone, dns_rdatatype_t xfrtype,
  *     caller itself.
  */
 
+isc_result_t
+dns_xfrin_start(dns_xfrin_t *xfr, dns_xfrindone_t done);
+/*%<
+ * Attempt to start an incoming zone transfer of 'xfr->zone'
+ * using the previously created '*xfr' object.
+ *
+ * Iff ISC_R_SUCCESS is returned, '*done' is called with
+ * 'zone' and a result code as arguments when the transfer finishes.
+ *
+ * Requires:
+ *\li  'xfr' is a valid dns_xfrin_t object and is associated with a zone.
+ *
+ *\li  'done' != NULL.
+ *
+ */
+
 isc_time_t
 dns_xfrin_getstarttime(dns_xfrin_t *xfr);
 /*%<

diff --git a/lib/dns/xfrin.c b/lib/dns/xfrin.c
index 7b591b654298b0ecdc9bf95b705f45b194e23d5f..20d6081d48739bc9f664fc248a327b21efe49d5b 100644 (file)
--- a/lib/dns/xfrin.c
+++ b/lib/dns/xfrin.c
@@ -865,24 +865,20 @@ failure:
        return (result);
 }
 
-isc_result_t
+void
 dns_xfrin_create(dns_zone_t *zone, dns_rdatatype_t xfrtype,
                 uint32_t ixfr_maxdiffs, const isc_sockaddr_t *primaryaddr,
                 const isc_sockaddr_t *sourceaddr, dns_tsigkey_t *tsigkey,
                 dns_transport_type_t soa_transport_type,
                 dns_transport_t *transport, isc_tlsctx_cache_t *tlsctx_cache,
-                isc_mem_t *mctx, dns_xfrindone_t done, dns_xfrin_t **xfrp) {
+                isc_mem_t *mctx, dns_xfrin_t **xfrp) {
        dns_name_t *zonename = dns_zone_getorigin(zone);
        dns_xfrin_t *xfr = NULL;
-       isc_result_t result;
        dns_db_t *db = NULL;
        isc_loop_t *loop = NULL;
 
        REQUIRE(xfrp != NULL && *xfrp == NULL);
-       REQUIRE(done != NULL);
        REQUIRE(isc_sockaddr_getport(primaryaddr) != 0);
-       REQUIRE(zone != NULL);
-       REQUIRE(dns_zone_getview(zone) != NULL);
 
        loop = dns_zone_getloop(zone);
 
@@ -898,26 +894,26 @@ dns_xfrin_create(dns_zone_t *zone, dns_rdatatype_t xfrtype,
 
        if (db != NULL) {
                xfr->zone_had_db = true;
+               dns_db_detach(&db);
        }
 
-       xfr->done = done;
-
-       /*
-        * Set *xfrp now, before calling xfrin_start(), otherwise it's
-        * possible the 'done' callback could be run before *xfrp
-        * was attached.
-        */
        *xfrp = xfr;
+}
+
+isc_result_t
+dns_xfrin_start(dns_xfrin_t *xfr, dns_xfrindone_t done) {
+       isc_result_t result;
+
+       REQUIRE(xfr != NULL);
+       REQUIRE(xfr->zone != NULL);
+       REQUIRE(done != NULL);
+
+       xfr->done = done;
 
        result = xfrin_start(xfr);
        if (result != ISC_R_SUCCESS) {
                xfr->done = NULL;
-               xfrin_fail(xfr, result, "zone transfer setup failed");
-               dns_xfrin_detach(xfrp);
-       }
-
-       if (db != NULL) {
-               dns_db_detach(&db);
+               xfrin_fail(xfr, result, "zone transfer start failed");
        }
 
        return (result);

diff --git a/lib/dns/zone.c b/lib/dns/zone.c
index f09da85cf72744bfb2cfebefcc7ed203a412b066..e8a57f7f41b5914805fc06cd0d96c4e7573f002d 100644 (file)
--- a/lib/dns/zone.c
+++ b/lib/dns/zone.c
@@ -314,6 +314,7 @@ struct dns_zone {
        char *keydirectory;
        dns_keyfileio_t *kfio;
        dns_keystorelist_t *keystores;
+       dns_xfrin_t *xfr;
 
        uint32_t maxrefresh;
        uint32_t minrefresh;
@@ -342,7 +343,6 @@ struct dns_zone {
        isc_sockaddr_t xfrsource4;
        isc_sockaddr_t xfrsource6;
        isc_sockaddr_t sourceaddr;
-       dns_xfrin_t *xfr;           /* loop locked */
        dns_tsigkey_t *tsigkey;     /* key used for xfr */
        dns_transport_t *transport; /* transport used for xfr */
        /* Access Control Lists */
@@ -18006,10 +18006,9 @@ again:
        zone_settimer(zone, &now);
 
        /*
-        * If creating the transfer object failed, zone->xfr is NULL.
-        * Otherwise, we are called as the done callback of a zone
-        * transfer object that just entered its shutting-down
-        * state.  Since we are no longer responsible for shutting
+        * We are called as the done callback of a zone
+        * transfer object that just entered its shutting-down state or
+        * failed to start.  Since we are no longer responsible for shutting
         * it down, we can detach our reference.
         */
        if (zone->xfr != NULL) {
@@ -18365,6 +18364,7 @@ got_transfer_quota(void *arg) {
        const char *soa_before = "";
        bool loaded;
        isc_tlsctx_cache_t *zmgr_tlsctx_cache = NULL;
+       dns_xfrin_t *xfr = NULL;
 
        if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_EXITING)) {
                zone_xfrdone(zone, NULL, ISC_R_CANCELED);
@@ -18514,24 +18514,30 @@ got_transfer_quota(void *arg) {
 
        INSIST(isc_sockaddr_pf(&primaryaddr) == isc_sockaddr_pf(&sourceaddr));
 
-       if (zone->xfr != NULL) {
-               dns_xfrin_detach(&zone->xfr);
-       }
-
        zmgr_tlsctx_attach(zone->zmgr, &zmgr_tlsctx_cache);
 
-       result = dns_xfrin_create(
-               zone, xfrtype, ixfr_maxdiffs, &primaryaddr, &sourceaddr,
-               zone->tsigkey, soa_transport_type, zone->transport,
-               zmgr_tlsctx_cache, zone->mctx, zone_xfrdone, &zone->xfr);
+       dns_xfrin_create(zone, xfrtype, ixfr_maxdiffs, &primaryaddr,
+                        &sourceaddr, zone->tsigkey, soa_transport_type,
+                        zone->transport, zmgr_tlsctx_cache, zone->mctx, &xfr);
+       INSIST(xfr != NULL);
 
        isc_tlsctx_cache_detach(&zmgr_tlsctx_cache);
 
+       LOCK_ZONE(zone);
+       if (zone->xfr != NULL) {
+               dns_xfrin_detach(&zone->xfr);
+       }
+       dns_xfrin_attach(xfr, &zone->xfr);
+       UNLOCK_ZONE(zone);
+
+       dns_xfrin_detach(&xfr);
+
        /*
         * Any failure in this function is handled like a failed
         * zone transfer.  This ensures that we get removed from
         * zmgr->xfrin_in_progress.
         */
+       result = dns_xfrin_start(zone->xfr, zone_xfrdone);
        if (result != ISC_R_SUCCESS) {
                zone_xfrdone(zone, NULL, result);
                return;