The dsset returned by dns_keynode_dsset needs to be thread safe.

- clone keynode->dsset rather than return a pointer so that thread use is independent of each other. - hold a reference to the dsset (keynode) so it can't be deleted while in use. - create a new keynode when removing DS records so that dangling pointers to the deleted records will not occur. - use a rwlock when accessing the rdatalist to prevent instabilities when DS records are added.
2025-08-31 06:25:31 +00:00 · 2020-06-10 17:07:52 +10:00
parent c24f4eb1c9
commit e5b2eca1d3
8 changed files with 275 additions and 101 deletions
--- a/lib/dns/keytable.c
+++ b/lib/dns/keytable.c
@@ -48,13 +48,49 @@ struct dns_keytable {

 struct dns_keynode {
 	unsigned int magic;
+	isc_mem_t *mctx;
 	isc_refcount_t refcount;
+	isc_rwlock_t rwlock;
 	dns_rdatalist_t *dslist;
 	dns_rdataset_t dsset;
 	bool managed;
 	bool initial;
 };

+static dns_keynode_t *
+new_keynode(dns_rdata_ds_t *ds, dns_keytable_t *keytable, bool managed,
+	    bool initial);
+
+static void
+keynode_disassociate(dns_rdataset_t *rdataset);
+static isc_result_t
+keynode_first(dns_rdataset_t *rdataset);
+static isc_result_t
+keynode_next(dns_rdataset_t *rdataset);
+static void
+keynode_current(dns_rdataset_t *rdataset, dns_rdata_t *rdata);
+static void
+keynode_clone(dns_rdataset_t *source, dns_rdataset_t *target);
+
+static dns_rdatasetmethods_t methods = {
+	keynode_disassociate,
+	keynode_first,
+	keynode_next,
+	keynode_current,
+	keynode_clone,
+	NULL,
+	NULL,
+	NULL,
+	NULL,
+	NULL,
+	NULL, /* settrust */
+	NULL, /* expire */
+	NULL, /* clearprefetch */
+	NULL,
+	NULL,
+	NULL /* addglue */
+};
+
 static void
 keynode_attach(dns_keynode_t *source, dns_keynode_t **target) {
 	REQUIRE(VALID_KEYNODE(source));
@@ -71,11 +107,8 @@ keynode_detach(isc_mem_t *mctx, dns_keynode_t **keynodep) {
 	if (isc_refcount_decrement(&knode->refcount) == 1) {
 		dns_rdata_t *rdata = NULL;
 		isc_refcount_destroy(&knode->refcount);
+		isc_rwlock_destroy(&knode->rwlock);
 		if (knode->dslist != NULL) {
-			if (dns_rdataset_isassociated(&knode->dsset)) {
-				dns_rdataset_disassociate(&knode->dsset);
-			}
-
 			for (rdata = ISC_LIST_HEAD(knode->dslist->rdata);
 			     rdata != NULL;
 			     rdata = ISC_LIST_HEAD(knode->dslist->rdata))
@@ -91,7 +124,8 @@ keynode_detach(isc_mem_t *mctx, dns_keynode_t **keynodep) {
 				    sizeof(*knode->dslist));
 			knode->dslist = NULL;
 		}
-		isc_mem_put(mctx, knode, sizeof(dns_keynode_t));
+		isc_mem_putanddetach(&knode->mctx, knode,
+				     sizeof(dns_keynode_t));
 	}
 }

@@ -171,7 +205,7 @@ dns_keytable_detach(dns_keytable_t **keytablep) {
 	}
 }

-static isc_result_t
+static void
 add_ds(dns_keynode_t *knode, dns_rdata_ds_t *ds, isc_mem_t *mctx) {
 	isc_result_t result;
 	dns_rdata_t *dsrdata = NULL, *rdata = NULL;
@@ -179,13 +213,6 @@ add_ds(dns_keynode_t *knode, dns_rdata_ds_t *ds, isc_mem_t *mctx) {
 	bool exists = false;
 	isc_buffer_t b;

-	if (knode->dslist == NULL) {
-		knode->dslist = isc_mem_get(mctx, sizeof(*knode->dslist));
-		dns_rdatalist_init(knode->dslist);
-		knode->dslist->rdclass = dns_rdataclass_in;
-		knode->dslist->type = dns_rdatatype_ds;
-	}
-
 	dsrdata = isc_mem_get(mctx, sizeof(*dsrdata));
 	dns_rdata_init(dsrdata);

@@ -194,8 +221,28 @@ add_ds(dns_keynode_t *knode, dns_rdata_ds_t *ds, isc_mem_t *mctx) {

 	result = dns_rdata_fromstruct(dsrdata, dns_rdataclass_in,
 				      dns_rdatatype_ds, ds, &b);
-	if (result != ISC_R_SUCCESS) {
-		return (result);
+	RUNTIME_CHECK(result == ISC_R_SUCCESS);
+
+	RWLOCK(&knode->rwlock, isc_rwlocktype_write);
+
+	if (knode->dslist == NULL) {
+		knode->dslist = isc_mem_get(mctx, sizeof(*knode->dslist));
+		dns_rdatalist_init(knode->dslist);
+		knode->dslist->rdclass = dns_rdataclass_in;
+		knode->dslist->type = dns_rdatatype_ds;
+
+		INSIST(knode->dsset.methods == NULL);
+		knode->dsset.methods = &methods;
+		knode->dsset.rdclass = knode->dslist->rdclass;
+		knode->dsset.type = knode->dslist->type;
+		knode->dsset.covers = knode->dslist->covers;
+		knode->dsset.ttl = knode->dslist->ttl;
+		knode->dsset.private1 = knode;
+		knode->dsset.private2 = NULL;
+		knode->dsset.private3 = NULL;
+		knode->dsset.privateuint4 = 0;
+		knode->dsset.private5 = NULL;
+		knode->dsset.trust = dns_trust_ultimate;
 	}

 	for (rdata = ISC_LIST_HEAD(knode->dslist->rdata); rdata != NULL;
@@ -210,26 +257,16 @@ add_ds(dns_keynode_t *knode, dns_rdata_ds_t *ds, isc_mem_t *mctx) {
 	if (exists) {
 		isc_mem_put(mctx, dsrdata->data, DNS_DS_BUFFERSIZE);
 		isc_mem_put(mctx, dsrdata, sizeof(*dsrdata));
-		return (ISC_R_SUCCESS);
+	} else {
+		ISC_LIST_APPEND(knode->dslist->rdata, dsrdata, link);
 	}

-	ISC_LIST_APPEND(knode->dslist->rdata, dsrdata, link);
-
-	if (dns_rdataset_isassociated(&knode->dsset)) {
-		dns_rdataset_disassociate(&knode->dsset);
-	}
-
-	result = dns_rdatalist_tordataset(knode->dslist, &knode->dsset);
-	if (result != ISC_R_SUCCESS) {
-		return (result);
-	}
-
-	knode->dsset.trust = dns_trust_ultimate;
-	return (ISC_R_SUCCESS);
+	RWUNLOCK(&knode->rwlock, isc_rwlocktype_write);
 }

 static isc_result_t
-delete_ds(dns_keynode_t *knode, dns_rdata_ds_t *ds, isc_mem_t *mctx) {
+delete_ds(dns_keytable_t *keytable, dns_rbtnode_t *node, dns_rdata_ds_t *ds) {
+	dns_keynode_t *knode = node->data;
 	isc_result_t result;
 	dns_rdata_t dsrdata = DNS_RDATA_INIT;
 	dns_rdata_t *rdata = NULL;
@@ -237,7 +274,9 @@ delete_ds(dns_keynode_t *knode, dns_rdata_ds_t *ds, isc_mem_t *mctx) {
 	bool found = false;
 	isc_buffer_t b;

+	RWLOCK(&knode->rwlock, isc_rwlocktype_read);
 	if (knode->dslist == NULL) {
+		RWUNLOCK(&knode->rwlock, isc_rwlocktype_read);
 		return (ISC_R_SUCCESS);
 	}

@@ -246,6 +285,7 @@ delete_ds(dns_keynode_t *knode, dns_rdata_ds_t *ds, isc_mem_t *mctx) {
 	result = dns_rdata_fromstruct(&dsrdata, dns_rdataclass_in,
 				      dns_rdatatype_ds, ds, &b);
 	if (result != ISC_R_SUCCESS) {
+		RWUNLOCK(&knode->rwlock, isc_rwlocktype_write);
 		return (result);
 	}

@@ -253,23 +293,40 @@ delete_ds(dns_keynode_t *knode, dns_rdata_ds_t *ds, isc_mem_t *mctx) {
 	     rdata = ISC_LIST_NEXT(rdata, link))
 	{
 		if (dns_rdata_compare(rdata, &dsrdata) == 0) {
-			ISC_LIST_UNLINK(knode->dslist->rdata, rdata, link);
-			isc_mem_put(mctx, rdata->data, DNS_DS_BUFFERSIZE);
-			isc_mem_put(mctx, rdata, sizeof(*rdata));
 			found = true;
 			break;
 		}
 	}

-	if (found) {
-		return (ISC_R_SUCCESS);
-	} else {
+	if (!found) {
+		RWUNLOCK(&knode->rwlock, isc_rwlocktype_read);
 		/*
 		 * The keyname must have matched or we wouldn't be here,
 		 * so we use DNS_R_PARTIALMATCH instead of ISC_R_NOTFOUND.
 		 */
 		return (DNS_R_PARTIALMATCH);
 	}
+
+	/*
+	 * Replace knode with a new instance without the DS.
+	 */
+	node->data = new_keynode(NULL, keytable, knode->managed,
+				 knode->initial);
+	for (rdata = ISC_LIST_HEAD(knode->dslist->rdata); rdata != NULL;
+	     rdata = ISC_LIST_NEXT(rdata, link))
+	{
+		if (dns_rdata_compare(rdata, &dsrdata) != 0) {
+			dns_rdata_ds_t ds0;
+			result = dns_rdata_tostruct(rdata, &ds0, NULL);
+			RUNTIME_CHECK(result == ISC_R_SUCCESS);
+			add_ds(node->data, &ds0, keytable->mctx);
+		}
+	}
+	RWUNLOCK(&knode->rwlock, isc_rwlocktype_read);
+
+	keynode_detach(keytable->mctx, &knode);
+
+	return (ISC_R_SUCCESS);
 }

 /*%
@@ -277,9 +334,9 @@ delete_ds(dns_keynode_t *knode, dns_rdata_ds_t *ds, isc_mem_t *mctx) {
 * "managed" and "initial" as requested and attach the keynode to
 * to "node" in "keytable".
 */
-static isc_result_t
-new_keynode(dns_rdata_ds_t *ds, dns_rbtnode_t *node, dns_keytable_t *keytable,
-	    bool managed, bool initial) {
+static dns_keynode_t *
+new_keynode(dns_rdata_ds_t *ds, dns_keytable_t *keytable, bool managed,
+	    bool initial) {
 	dns_keynode_t *knode = NULL;
 	isc_result_t result;

@@ -291,23 +348,21 @@ new_keynode(dns_rdata_ds_t *ds, dns_rbtnode_t *node, dns_keytable_t *keytable,

 	dns_rdataset_init(&knode->dsset);
 	isc_refcount_init(&knode->refcount, 1);
+	result = isc_rwlock_init(&knode->rwlock, 0, 0);
+	RUNTIME_CHECK(result == ISC_R_SUCCESS);

 	/*
 	 * If a DS was supplied, initialize an rdatalist.
 	 */
 	if (ds != NULL) {
-		result = add_ds(knode, ds, keytable->mctx);
-		if (result != ISC_R_SUCCESS) {
-			return (result);
-		}
+		add_ds(knode, ds, keytable->mctx);
 	}

+	isc_mem_attach(keytable->mctx, &knode->mctx);
 	knode->managed = managed;
 	knode->initial = initial;

-	node->data = knode;
-
-	return (ISC_R_SUCCESS);
+	return (knode);
 }

 /*%
@@ -335,30 +390,21 @@ insert(dns_keytable_t *keytable, bool managed, bool initial,
 		 * trust anchor (or a null key node if "ds" is NULL)
 		 * and attach it to the created node.
 		 */
-		result = new_keynode(ds, node, keytable, managed, initial);
+		node->data = new_keynode(ds, keytable, managed, initial);
 	} else if (result == ISC_R_EXISTS) {
 		/*
 		 * A node already exists for "keyname" in "keytable".
 		 */
-		if (ds == NULL) {
-			/*
-			 * We were told to add a null key at "keyname", which
-			 * means there is nothing left to do as there is either
-			 * a null key at this node already or there is a
-			 * non-null key node which would not be affected.
-			 * Reset result to reflect the fact that the node for
-			 * "keyname" is already marked as secure.
-			 */
-			result = ISC_R_SUCCESS;
-		} else {
+		if (ds != NULL) {
 			dns_keynode_t *knode = node->data;
 			if (knode == NULL) {
-				result = new_keynode(ds, node, keytable,
-						     managed, initial);
+				node->data = new_keynode(ds, keytable, managed,
+							 initial);
 			} else {
-				result = add_ds(knode, ds, keytable->mctx);
+				add_ds(knode, ds, keytable->mctx);
 			}
 		}
+		result = ISC_R_SUCCESS;
 	}

 	RWUNLOCK(&keytable->rwlock, isc_rwlocktype_write);
@@ -442,10 +488,13 @@ dns_keytable_deletekey(dns_keytable_t *keytable, const dns_name_t *keyname,

 	knode = node->data;

+	RWLOCK(&knode->rwlock, isc_rwlocktype_read);
 	if (knode->dslist == NULL) {
+		RWUNLOCK(&knode->rwlock, isc_rwlocktype_read);
 		result = DNS_R_PARTIALMATCH;
 		goto finish;
 	}
+	RWUNLOCK(&knode->rwlock, isc_rwlocktype_read);

 	result = dns_ds_fromkeyrdata(keyname, &rdata, DNS_DSDIGEST_SHA256,
 				     digest, &ds);
@@ -453,7 +502,7 @@ dns_keytable_deletekey(dns_keytable_t *keytable, const dns_name_t *keyname,
 		goto finish;
 	}

-	result = delete_ds(knode, &ds, keytable->mctx);
+	result = delete_ds(keytable, node, &ds);

 finish:
 	RWUNLOCK(&keytable->rwlock, isc_rwlocktype_write);
@@ -606,20 +655,23 @@ keynode_dslist_totext(dns_name_t *name, dns_keynode_t *keynode,
 	isc_result_t result;
 	char namebuf[DNS_NAME_FORMATSIZE];
 	char obuf[DNS_NAME_FORMATSIZE + 200];
-	dns_rdataset_t *dsset = NULL;
+	dns_rdataset_t dsset;

 	dns_name_format(name, namebuf, sizeof(namebuf));

-	dsset = dns_keynode_dsset(keynode);
+	dns_rdataset_init(&dsset);
+	if (!dns_keynode_dsset(keynode, &dsset)) {
+		return (ISC_R_SUCCESS);
+	}

-	for (result = dns_rdataset_first(dsset); result == ISC_R_SUCCESS;
-	     result = dns_rdataset_next(dsset))
+	for (result = dns_rdataset_first(&dsset); result == ISC_R_SUCCESS;
+	     result = dns_rdataset_next(&dsset))
 	{
 		char algbuf[DNS_SECALG_FORMATSIZE];
 		dns_rdata_t rdata = DNS_RDATA_INIT;
 		dns_rdata_ds_t ds;

-		dns_rdataset_current(dsset, &rdata);
+		dns_rdataset_current(&dsset, &rdata);
 		result = dns_rdata_tostruct(&rdata, &ds, NULL);
 		RUNTIME_CHECK(result == ISC_R_SUCCESS);

@@ -632,9 +684,11 @@ keynode_dslist_totext(dns_name_t *name, dns_keynode_t *keynode,

 		result = putstr(text, obuf);
 		if (result != ISC_R_SUCCESS) {
+			dns_rdataset_disassociate(&dsset);
 			return (result);
 		}
 	}
+	dns_rdataset_disassociate(&dsset);

 	return (ISC_R_SUCCESS);
 }
@@ -746,15 +800,19 @@ cleanup:
 	return (result);
 }

-dns_rdataset_t *
-dns_keynode_dsset(dns_keynode_t *keynode) {
+bool
+dns_keynode_dsset(dns_keynode_t *keynode, dns_rdataset_t *rdataset) {
 	REQUIRE(VALID_KEYNODE(keynode));
+	REQUIRE(rdataset == NULL || DNS_RDATASET_VALID(rdataset));

 	if (keynode->dslist != NULL) {
-		return (&keynode->dsset);
+		if (rdataset != NULL) {
+			keynode_clone(&keynode->dsset, rdataset);
+		}
+		return (true);
 	}

-	return (NULL);
+	return (false);
 }

 bool
@@ -777,3 +835,93 @@ dns_keynode_trust(dns_keynode_t *keynode) {

 	keynode->initial = false;
 }
+
+static void
+keynode_disassociate(dns_rdataset_t *rdataset) {
+	dns_keynode_t *keynode;
+
+	REQUIRE(rdataset != NULL);
+	REQUIRE(rdataset->methods == &methods);
+
+	rdataset->methods = NULL;
+	keynode = rdataset->private1;
+	rdataset->private1 = NULL;
+
+	keynode_detach(keynode->mctx, &keynode);
+}
+
+static isc_result_t
+keynode_first(dns_rdataset_t *rdataset) {
+	dns_keynode_t *keynode;
+
+	REQUIRE(rdataset != NULL);
+	REQUIRE(rdataset->methods == &methods);
+
+	keynode = rdataset->private1;
+	RWLOCK(&keynode->rwlock, isc_rwlocktype_read);
+	rdataset->private2 = ISC_LIST_HEAD(keynode->dslist->rdata);
+	RWUNLOCK(&keynode->rwlock, isc_rwlocktype_read);
+
+	if (rdataset->private2 == NULL) {
+		return (ISC_R_NOMORE);
+	}
+
+	return (ISC_R_SUCCESS);
+}
+
+static isc_result_t
+keynode_next(dns_rdataset_t *rdataset) {
+	dns_keynode_t *keynode;
+	dns_rdata_t *rdata;
+
+	REQUIRE(rdataset != NULL);
+	REQUIRE(rdataset->methods == &methods);
+
+	rdata = rdataset->private2;
+	if (rdata == NULL) {
+		return (ISC_R_NOMORE);
+	}
+
+	keynode = rdataset->private1;
+	RWLOCK(&keynode->rwlock, isc_rwlocktype_read);
+	rdataset->private2 = ISC_LIST_NEXT(rdata, link);
+	RWUNLOCK(&keynode->rwlock, isc_rwlocktype_read);
+
+	if (rdataset->private2 == NULL) {
+		return (ISC_R_NOMORE);
+	}
+
+	return (ISC_R_SUCCESS);
+}
+
+static void
+keynode_current(dns_rdataset_t *rdataset, dns_rdata_t *rdata) {
+	dns_rdata_t *list_rdata;
+
+	REQUIRE(rdataset != NULL);
+	REQUIRE(rdataset->methods == &methods);
+
+	list_rdata = rdataset->private2;
+	INSIST(list_rdata != NULL);
+
+	dns_rdata_clone(list_rdata, rdata);
+}
+
+static void
+keynode_clone(dns_rdataset_t *source, dns_rdataset_t *target) {
+	dns_keynode_t *keynode;
+
+	REQUIRE(source != NULL);
+	REQUIRE(target != NULL);
+	REQUIRE(source->methods == &methods);
+
+	keynode = source->private1;
+	isc_refcount_increment(&keynode->refcount);
+
+	*target = *source;
+
+	/*
+	 * Reset iterator state.
+	 */
+	target->private2 = NULL;
+}