avoid a race in the qpzone getsigningtime() implementation

the previous commit introduced a possible race in getsigningtime()
where the rdataset header could change between being found on the
heap and being bound.

getsigningtime() now looks at the first element of the heap, gathers the
locknum, locks the respective lock, and retrieves the header from the
heap again.  If the locknum has changed, it will rinse and repeat.
Theoretically, this could spin forever, but practically, it almost never
will as the heap changes on the zone are very rare.

we simplify matters further by changing the dns_db_getsigningtime()
API call. instead of passing back a bound rdataset, we pass back the
information the caller actually needed: the resigning time, owner name
and type of the rdataset that was first on the heap.

bin/named/server.c

@@ -15081,29 +15081,26 @@ named_server_zonestatus(named_server_t *server, isc_lex_t *lex,
 	{
 		dns_name_t *name;
 		dns_fixedname_t fixed;
-		dns_rdataset_t next;
+		isc_stdtime_t resign;
+		dns_typepair_t typepair;
 
-		dns_rdataset_init(&next);
 		name = dns_fixedname_initname(&fixed);
 
-		result = dns_db_getsigningtime(db, &next, name);
+		result = dns_db_getsigningtime(db, &resign, name, &typepair);
 		if (result == ISC_R_SUCCESS) {
 			char namebuf[DNS_NAME_FORMATSIZE];
 			char typebuf[DNS_RDATATYPE_FORMATSIZE];
 
+			resign -= dns_zone_getsigresigninginterval(zone);
+
 			dns_name_format(name, namebuf, sizeof(namebuf));
-			dns_rdatatype_format(next.covers, typebuf,
+			dns_rdatatype_format(DNS_TYPEPAIR_COVERS(typepair),
-					     sizeof(typebuf));
+					     typebuf, sizeof(typebuf));
 			snprintf(resignbuf, sizeof(resignbuf), "%s/%s", namebuf,
 				 typebuf);
-			isc_time_set(
+			isc_time_set(&resigntime, resign, 0);
-				&resigntime,
-				next.resign -
-					dns_zone_getsigresigninginterval(zone),
-				0);
 			isc_time_formathttptimestamp(&resigntime, rtbuf,
 						     sizeof(rtbuf));
-			dns_rdataset_disassociate(&next);
 		}
 	}
 

bin/tests/system/dyndb/driver/db.c

@@ -363,14 +363,13 @@ setsigningtime(dns_db_t *db, dns_rdataset_t *rdataset, isc_stdtime_t resign) {
 }
 
 static isc_result_t
-getsigningtime(dns_db_t *db, dns_rdataset_t *rdataset,
+getsigningtime(dns_db_t *db, isc_stdtime_t *resign, dns_name_t *name,
-	       dns_name_t *name DNS__DB_FLARG) {
+	       dns_typepair_t *type) {
 	sampledb_t *sampledb = (sampledb_t *)db;
 
 	REQUIRE(VALID_SAMPLEDB(sampledb));
 
-	return (dns__db_getsigningtime(sampledb->rbtdb, rdataset,
+	return (dns_db_getsigningtime(sampledb->rbtdb, resign, name, type));
-				       name DNS__DB_FLARG_PASS));
 }
 
 static dns_stats_t *

lib/dns/db.c

@@ -957,11 +957,11 @@ dns_db_setsigningtime(dns_db_t *db, dns_rdataset_t *rdataset,
 }
 
 isc_result_t
-dns__db_getsigningtime(dns_db_t *db, dns_rdataset_t *rdataset,
+dns_db_getsigningtime(dns_db_t *db, isc_stdtime_t *resign, dns_name_t *name,
-		       dns_name_t *name DNS__DB_FLARG) {
+		      dns_typepair_t *typepair) {
 	if (db->methods->getsigningtime != NULL) {
-		return ((db->methods->getsigningtime)(db, rdataset,
+		return ((db->methods->getsigningtime)(db, resign, name,
-						      name DNS__DB_FLARG_PASS));
+						      typepair));
 	}
 	return (ISC_R_NOTFOUND);
 }

lib/dns/include/dns/db.h

@@ -147,8 +147,9 @@ typedef struct dns_dbmethods {
 				      dns_dbnode_t **nodep DNS__DB_FLARG);
 	isc_result_t (*setsigningtime)(dns_db_t *db, dns_rdataset_t *rdataset,
 				       isc_stdtime_t resign);
-	isc_result_t (*getsigningtime)(dns_db_t *db, dns_rdataset_t *rdataset,
+	isc_result_t (*getsigningtime)(dns_db_t *db, isc_stdtime_t *resign,
-				       dns_name_t *name DNS__DB_FLARG);
+				       dns_name_t     *name,
+				       dns_typepair_t *typepair);
 	dns_stats_t *(*getrrsetstats)(dns_db_t *db);
 	isc_result_t (*findnodeext)(dns_db_t *db, const dns_name_t *name,
 				    bool		     create,
@@ -1585,19 +1586,19 @@ dns_db_setsigningtime(dns_db_t *db, dns_rdataset_t *rdataset,
  * \li	#ISC_R_NOTIMPLEMENTED - Not supported by this DB implementation.
  */
 
-#define dns_db_getsigningtime(db, rdataset, name) \
-	dns__db_getsigningtime(db, rdataset, name DNS__DB_FILELINE)
 isc_result_t
-dns__db_getsigningtime(dns_db_t *db, dns_rdataset_t *rdataset,
+dns_db_getsigningtime(dns_db_t *db, isc_stdtime_t *resign,
-		       dns_name_t *name DNS__DB_FLARG);
+		      dns_name_t *foundname, dns_typepair_t *typepair);
 /*%<
- * Return the rdataset with the earliest signing time in the zone.
+ * Find the rdataset header with the earliest signing time in a zone
- * Note: the rdataset is version agnostic.
+ * database. Update 'foundname' and 'typepair' with its name and
+ * type, and update 'resign' with the time at which it is to be signed.
  *
  * Requires:
  * \li	'db' is a valid zone database.
- * \li	'rdataset' to be initialized but not associated.
+ * \li	'resign' is not NULL.
- * \li	'name' to be NULL or have a buffer associated with it.
+ * \li	'foundname' is not NULL.
+ * \li	'typepair' is not NULL.
  *
  * Returns:
  * \li	#ISC_R_SUCCESS

lib/dns/qpzone.c

@@ -188,7 +188,6 @@ struct qpzonedb {
 	isc_loop_t *loop;
 	struct rcu_head rcu_head;
 
-	isc_mutex_t heaplock;
 	isc_heap_t *heap; /* Resigning heap */
 
 	dns_qpmulti_t *tree;  /* Main QP trie for data storage */
@@ -468,7 +467,6 @@ free_db_rcu(struct rcu_head *rcu_head) {
 	}
 
 	isc_heap_destroy(&qpdb->heap);
-	isc_mutex_destroy(&qpdb->heaplock);
 
 	if (qpdb->gluecachestats != NULL) {
 		isc_stats_detach(&qpdb->gluecachestats);
@@ -657,7 +655,6 @@ dns__qpzone_create(isc_mem_t *mctx, const dns_name_t *origin, dns_dbtype_t type,
 
 	qpdb->common.update_listeners = cds_lfht_new(16, 16, 0, 0, NULL);
 
-	isc_mutex_init(&qpdb->heaplock);
 	isc_heap_create(mctx, resign_sooner, set_index, 0, &qpdb->heap);
 
 	qpdb->active = qpdb->node_lock_count;
@@ -1268,12 +1265,12 @@ newversion(dns_db_t *db, dns_dbversion_t **versionp) {
 
 static void
 resigninsert(qpzonedb_t *qpdb, dns_slabheader_t *newheader) {
-	INSIST(newheader->heap_index == 0);
+	REQUIRE(newheader->heap_index == 0);
-	INSIST(!ISC_LINK_LINKED(newheader, link));
+	REQUIRE(!ISC_LINK_LINKED(newheader, link));
 
-	LOCK(&qpdb->heaplock);
+	RWLOCK(&qpdb->lock, isc_rwlocktype_write);
 	isc_heap_insert(qpdb->heap, newheader);
-	UNLOCK(&qpdb->heaplock);
+	RWUNLOCK(&qpdb->lock, isc_rwlocktype_write);
 
 	newheader->heap = qpdb->heap;
 }
@@ -1281,15 +1278,17 @@ resigninsert(qpzonedb_t *qpdb, dns_slabheader_t *newheader) {
 static void
 resigndelete(qpzonedb_t *qpdb, qpdb_version_t *version,
 	     dns_slabheader_t *header DNS__DB_FLARG) {
-	if (header != NULL && header->heap_index != 0) {
+	if (header == NULL || header->heap_index == 0) {
-		LOCK(&qpdb->heaplock);
+		return;
-		isc_heap_delete(qpdb->heap, header->heap_index);
-		UNLOCK(&qpdb->heaplock);
-
-		header->heap_index = 0;
-		newref(qpdb, HEADERNODE(header) DNS__DB_FLARG_PASS);
-		ISC_LIST_APPEND(version->resigned_list, header, link);
 	}
+
+	RWLOCK(&qpdb->lock, isc_rwlocktype_write);
+	isc_heap_delete(qpdb->heap, header->heap_index);
+	RWUNLOCK(&qpdb->lock, isc_rwlocktype_write);
+
+	header->heap_index = 0;
+	newref(qpdb, HEADERNODE(header) DNS__DB_FLARG_PASS);
+	ISC_LIST_APPEND(version->resigned_list, header, link);
 }
 
 static void
@@ -2412,7 +2411,7 @@ setsigningtime(dns_db_t *db, dns_rdataset_t *rdataset, isc_stdtime_t resign) {
 	}
 	if (header->heap_index != 0) {
 		INSIST(RESIGN(header));
-		LOCK(&qpdb->heaplock);
+		RWLOCK(&qpdb->lock, isc_rwlocktype_write);
 		if (resign == 0) {
 			isc_heap_delete(qpdb->heap, header->heap_index);
 			header->heap_index = 0;
@@ -2422,7 +2421,7 @@ setsigningtime(dns_db_t *db, dns_rdataset_t *rdataset, isc_stdtime_t resign) {
 		} else if (resign_sooner(&oldheader, header)) {
 			isc_heap_decreased(qpdb->heap, header->heap_index);
 		}
-		UNLOCK(&qpdb->heaplock);
+		RWUNLOCK(&qpdb->lock, isc_rwlocktype_write);
 	} else if (resign != 0) {
 		DNS_SLABHEADER_SETATTR(header, DNS_SLABHEADERATTR_RESIGN);
 		resigninsert(qpdb, header);
@@ -2433,33 +2432,52 @@ setsigningtime(dns_db_t *db, dns_rdataset_t *rdataset, isc_stdtime_t resign) {
 }
 
 static isc_result_t
-getsigningtime(dns_db_t *db, dns_rdataset_t *rdataset,
+getsigningtime(dns_db_t *db, isc_stdtime_t *resign, dns_name_t *foundname,
-	       dns_name_t *foundname DNS__DB_FLARG) {
+	       dns_typepair_t *typepair) {
 	qpzonedb_t *qpdb = (qpzonedb_t *)db;
 	dns_slabheader_t *header = NULL;
 	isc_rwlocktype_t nlocktype = isc_rwlocktype_none;
+	uint16_t locknum;
+	isc_result_t result = ISC_R_NOTFOUND;
 
 	REQUIRE(VALID_QPZONE(qpdb));
+	REQUIRE(resign != NULL);
+	REQUIRE(foundname != NULL);
+	REQUIRE(typepair != NULL);
 
-	LOCK(&qpdb->heaplock);
+	RWLOCK(&qpdb->lock, isc_rwlocktype_read);
 	header = isc_heap_element(qpdb->heap, 1);
-	UNLOCK(&qpdb->heaplock);
-
 	if (header == NULL) {
+		RWUNLOCK(&qpdb->lock, isc_rwlocktype_read);
 		return (ISC_R_NOTFOUND);
 	}
+	locknum = HEADERNODE(header)->locknum;
+	RWUNLOCK(&qpdb->lock, isc_rwlocktype_read);
 
-	NODE_RDLOCK(&qpdb->node_locks[HEADERNODE(header)->locknum].lock,
+again:
-		    &nlocktype);
+	NODE_RDLOCK(&qpdb->node_locks[locknum].lock, &nlocktype);
-	bindrdataset(qpdb, HEADERNODE(header), header, 0,
+
-		     rdataset DNS__DB_FLARG_PASS);
+	RWLOCK(&qpdb->lock, isc_rwlocktype_read);
-	if (foundname != NULL) {
+	header = isc_heap_element(qpdb->heap, 1);
-		dns_name_copy(&HEADERNODE(header)->name, foundname);
+	if (header != NULL && HEADERNODE(header)->locknum != locknum) {
+		RWUNLOCK(&qpdb->lock, isc_rwlocktype_read);
+		NODE_UNLOCK(&qpdb->node_locks[locknum].lock, &nlocktype);
+		locknum = HEADERNODE(header)->locknum;
+		goto again;
 	}
-	NODE_UNLOCK(&qpdb->node_locks[HEADERNODE(header)->locknum].lock,
-		    &nlocktype);
 
-	return (ISC_R_SUCCESS);
+	if (header != NULL) {
+		*resign = RESIGN(header)
+				  ? (header->resign << 1) | header->resign_lsb
+				  : 0;
+		dns_name_copy(&HEADERNODE(header)->name, foundname);
+		*typepair = header->type;
+		result = ISC_R_SUCCESS;
+	}
+	RWUNLOCK(&qpdb->lock, isc_rwlocktype_read);
+	NODE_UNLOCK(&qpdb->node_locks[locknum].lock, &nlocktype);
+
+	return (result);
 }
 
 static isc_result_t
@@ -4008,9 +4026,9 @@ deletedata(dns_db_t *db ISC_ATTR_UNUSED, dns_dbnode_t *node ISC_ATTR_UNUSED,
 	dns_slabheader_t *header = data;
 
 	if (header->heap != NULL && header->heap_index != 0) {
-		LOCK(&qpdb->heaplock);
+		RWLOCK(&qpdb->lock, isc_rwlocktype_write);
 		isc_heap_delete(header->heap, header->heap_index);
-		UNLOCK(&qpdb->heaplock);
+		RWUNLOCK(&qpdb->lock, isc_rwlocktype_write);
 	}
 	header->heap_index = 0;
 

lib/dns/rbt-zonedb.c

@@ -1993,8 +1993,8 @@ setsigningtime(dns_db_t *db, dns_rdataset_t *rdataset, isc_stdtime_t resign) {
 }
 
 static isc_result_t
-getsigningtime(dns_db_t *db, dns_rdataset_t *rdataset,
+getsigningtime(dns_db_t *db, isc_stdtime_t *resign, dns_name_t *foundname,
-	       dns_name_t *foundname DNS__DB_FLARG) {
+	       dns_typepair_t *typepair) {
 	dns_rbtdb_t *rbtdb = (dns_rbtdb_t *)db;
 	dns_slabheader_t *header = NULL, *this = NULL;
 	unsigned int i;
@@ -2004,6 +2004,9 @@ getsigningtime(dns_db_t *db, dns_rdataset_t *rdataset,
 	isc_rwlocktype_t nlocktype = isc_rwlocktype_none;
 
 	REQUIRE(VALID_RBTDB(rbtdb));
+	REQUIRE(resign != NULL);
+	REQUIRE(foundname != NULL);
+	REQUIRE(typepair != NULL);
 
 	TREE_RDLOCK(&rbtdb->tree_lock, &tlocktype);
 
@@ -2055,14 +2058,11 @@ getsigningtime(dns_db_t *db, dns_rdataset_t *rdataset,
 		 * Found something; pass back the answer and unlock
 		 * the bucket.
 		 */
-		dns__rbtdb_bindrdataset(rbtdb, RBTDB_HEADERNODE(header), header,
+		*resign = RESIGN(header)
-					0, isc_rwlocktype_read,
+				  ? (header->resign << 1) | header->resign_lsb
-					rdataset DNS__DB_FLARG_PASS);
+				  : 0;
-
+		dns_rbt_fullnamefromnode(RBTDB_HEADERNODE(header), foundname);
-		if (foundname != NULL) {
+		*typepair = header->type;
-			dns_rbt_fullnamefromnode(RBTDB_HEADERNODE(header),
-						 foundname);
-		}
 
 		NODE_UNLOCK(&rbtdb->node_locks[locknum].lock, &nlocktype);
 

lib/dns/zone.c

@@ -3816,12 +3816,12 @@ cleanup:
 
 static void
 set_resigntime(dns_zone_t *zone) {
-	dns_rdataset_t rdataset;
 	dns_fixedname_t fixed;
-	unsigned int resign;
+	isc_stdtime_t resign;
 	isc_result_t result;
 	uint32_t nanosecs;
 	dns_db_t *db = NULL;
+	dns_typepair_t typepair;
 
 	INSIST(LOCKED_ZONE(zone));
 
@@ -3834,7 +3834,6 @@ set_resigntime(dns_zone_t *zone) {
 		return;
 	}
 
-	dns_rdataset_init(&rdataset);
 	dns_fixedname_init(&fixed);
 
 	ZONEDB_LOCK(&zone->dblock, isc_rwlocktype_read);
@@ -3847,15 +3846,14 @@ set_resigntime(dns_zone_t *zone) {
 		return;
 	}
 
-	result = dns_db_getsigningtime(db, &rdataset,
+	result = dns_db_getsigningtime(db, &resign, dns_fixedname_name(&fixed),
-				       dns_fixedname_name(&fixed));
+				       &typepair);
 	if (result != ISC_R_SUCCESS) {
 		isc_time_settoepoch(&zone->resigntime);
 		goto cleanup;
 	}
 
-	resign = rdataset.resign - dns_zone_getsigresigninginterval(zone);
+	resign -= dns_zone_getsigresigninginterval(zone);
-	dns_rdataset_disassociate(&rdataset);
 	nanosecs = isc_random_uniform(1000000000);
 	isc_time_set(&zone->resigntime, resign, nanosecs);
 
@@ -5179,31 +5177,32 @@ zone_postload(dns_zone_t *zone, dns_db_t *db, isc_time_t loadtime,
 		if (zone->type == dns_zone_primary && is_dynamic &&
 		    dns_db_issecure(db) && !inline_raw(zone))
 		{
+			isc_stdtime_t resign;
 			dns_name_t *name;
 			dns_fixedname_t fixed;
-			dns_rdataset_t next;
+			dns_typepair_t typepair;
 
-			dns_rdataset_init(&next);
 			name = dns_fixedname_initname(&fixed);
 
-			result = dns_db_getsigningtime(db, &next, name);
+			result = dns_db_getsigningtime(db, &resign, name,
+						       &typepair);
 			if (result == ISC_R_SUCCESS) {
 				isc_stdtime_t timenow = isc_stdtime_now();
 				char namebuf[DNS_NAME_FORMATSIZE];
 				char typebuf[DNS_RDATATYPE_FORMATSIZE];
 
 				dns_name_format(name, namebuf, sizeof(namebuf));
-				dns_rdatatype_format(next.covers, typebuf,
+				dns_rdatatype_format(
-						     sizeof(typebuf));
+					DNS_TYPEPAIR_COVERS(typepair), typebuf,
+					sizeof(typebuf));
 				dnssec_log(
 					zone, ISC_LOG_DEBUG(3),
 					"next resign: %s/%s "
 					"in %d seconds",
 					namebuf, typebuf,
-					next.resign - timenow -
+					resign - timenow -
 						dns_zone_getsigresigninginterval(
 							zone));
-				dns_rdataset_disassociate(&next);
 			} else {
 				dnssec_log(zone, ISC_LOG_WARNING,
 					   "signed dynamic zone has no "
@@ -6966,18 +6965,16 @@ zone_resigninc(dns_zone_t *zone) {
 	dns__zonediff_t zonediff;
 	dns_fixedname_t fixed;
 	dns_name_t *name;
-	dns_rdataset_t rdataset;
+	dns_typepair_t typepair;
-	dns_rdatatype_t covers;
 	dst_key_t *zone_keys[DNS_MAXZONEKEYS];
 	isc_result_t result;
 	isc_stdtime_t now, inception, soaexpire, expire, fullexpire, stop;
 	unsigned int i;
 	unsigned int nkeys = 0;
-	unsigned int resign;
+	isc_stdtime_t resign;
 
 	ENTER;
 
-	dns_rdataset_init(&rdataset);
 	dns_diff_init(zone->mctx, &_sig_diff);
 	zonediff_init(&zonediff, &_sig_diff);
 
@@ -7024,7 +7021,7 @@ zone_resigninc(dns_zone_t *zone) {
 	stop = now + 5;
 
 	name = dns_fixedname_initname(&fixed);
-	result = dns_db_getsigningtime(db, &rdataset, name);
+	result = dns_db_getsigningtime(db, &resign, name, &typepair);
 	if (result != ISC_R_SUCCESS && result != ISC_R_NOTFOUND) {
 		dns_zone_log(zone, ISC_LOG_ERROR,
 			     "zone_resigninc:dns_db_getsigningtime -> %s",
@@ -7033,10 +7030,9 @@ zone_resigninc(dns_zone_t *zone) {
 
 	i = 0;
 	while (result == ISC_R_SUCCESS) {
-		resign = rdataset.resign -
+		dns_rdatatype_t covers = DNS_TYPEPAIR_COVERS(typepair);
-			 dns_zone_getsigresigninginterval(zone);
+
-		covers = rdataset.covers;
+		resign -= dns_zone_getsigresigninginterval(zone);
-		dns_rdataset_disassociate(&rdataset);
 
 		/*
 		 * Stop if we hit the SOA as that means we have walked the
@@ -7076,7 +7072,7 @@ zone_resigninc(dns_zone_t *zone) {
 				     isc_result_totext(result));
 			break;
 		}
-		result = dns_db_getsigningtime(db, &rdataset, name);
+		result = dns_db_getsigningtime(db, &resign, name, &typepair);
 		if (nkeys == 0 && result == ISC_R_NOTFOUND) {
 			result = ISC_R_SUCCESS;
 			break;