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.

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);
 /*%<

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);

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));
 
+	zmgr_tlsctx_attach(zone->zmgr, &zmgr_tlsctx_cache);
+
+	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);
 
-	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);
-
-	isc_tlsctx_cache_detach(&zmgr_tlsctx_cache);
+	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;