From: Petr Špaček <pspacek@isc.org>
Date: Thu, 13 Oct 2022 11:08:22 +0000 (+0200)
Subject: Fix latent bug in RBT node attributes handling
X-Git-Tag: v9.19.7~65^2~1
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=8a3aa8bda41946f1adf46201aa0be9555f4f2529;p=thirdparty%2Fbind9.git

Fix latent bug in RBT node attributes handling

Originally RBT node stored three lowest bits from dns_name_t attributes.
This had a curious side-effect noticed by Tony Finch:

If you create an rbt node from a DYNAMIC name then the flag will be
propagated through dns_rbt_namefromnode() ... if you subsequently call
dns_name_free() it will try to isc_mem_put() a piece of an rbt node ...
but dns_name_free() REQUIRE()s that the name is dynamic so in the usual
case where rbt nodes are created from non-dynamic names, this kind of
code will fail an assertion.

This is a bug it dates back to june 1999 when NAMEATTR_DYNAMIC was
invented.

Apparently it does not happen often :-)
I'm planning to get rid of DNS_NAMEATTR_ definitions and bit operations,
so removal of this "three-bit-subset" assignment is a first step.

We can keep only the ABSOLUTE flag in RBT node and nothing else because
names attached to rbt nodes are always readonly: The internal node_name()
function always sets the NAMEATTR_READONLY when making a dns_name that
refers to the node's name, so the READONLY flag will be set in the name
returned by dns_rbt_namefromnode().

Co-authored-by: Tony Finch <fanf@isc.org>
---

diff --git a/lib/dns/include/dns/rbt.h b/lib/dns/include/dns/rbt.h
index 24d10740e85..ce61fa2aafb 100644
--- a/lib/dns/include/dns/rbt.h
+++ b/lib/dns/include/dns/rbt.h
@@ -71,9 +71,7 @@ struct dns_rbtnode {
 	/*@{*/
 	/*!
 	 * The following bitfields add up to a total bitwidth of 32.
-	 * The range of values necessary for each item is indicated,
-	 * but in the case of "attributes" the field is wider to accommodate
-	 * possible future expansion.
+	 * The range of values necessary for each item is indicated.
 	 *
 	 * In each case below the "range" indicated is what's _necessary_ for
 	 * the bitfield to hold, not what it actually _can_ hold.
@@ -92,7 +90,7 @@ struct dns_rbtnode {
 	unsigned int is_root	   : 1; /*%< range is 0..1 */
 	unsigned int color	   : 1; /*%< range is 0..1 */
 	unsigned int find_callback : 1; /*%< range is 0..1 */
-	unsigned int attributes	   : 3; /*%< range is 0..2 */
+	bool	     absolute	   : 1; /*%< node with absolute DNS name */
 	unsigned int nsec	   : 2; /*%< range is 0..3 */
 	unsigned int namelen	   : 8; /*%< range is 1..255 */
 	unsigned int offsetlen	   : 8; /*%< range is 1..128 */
diff --git a/lib/dns/rbt.c b/lib/dns/rbt.c
index 77de2b650c3..fe766714884 100644
--- a/lib/dns/rbt.c
+++ b/lib/dns/rbt.c
@@ -130,7 +130,7 @@ node_name(dns_rbtnode_t *node, dns_name_t *name) {
 	name->labels = node->offsetlen;
 	name->ndata = NAME(node);
 	name->offsets = OFFSETS(node);
-	name->attributes = node->attributes;
+	name->attributes = node->absolute ? DNS_NAMEATTR_ABSOLUTE : 0;
 	name->attributes |= DNS_NAMEATTR_READONLY;
 }
 
@@ -644,7 +644,7 @@ dns_rbt_addnode(dns_rbt_t *rbt, const dns_name_t *name, dns_rbtnode_t **nodep) {
 				current->right = NULL;
 
 				current->color = BLACK;
-				current->attributes &= ~DNS_NAMEATTR_ABSOLUTE;
+				current->absolute = false;
 
 				rbt->nodecount++;
 				dns_name_getlabelsequence(name,
@@ -1543,7 +1543,7 @@ create_node(isc_mem_t *mctx, const dns_name_t *name, dns_rbtnode_t **nodep) {
 	 */
 	node->oldnamelen = node->namelen = region.length;
 	OLDOFFSETLEN(node) = node->offsetlen = labels;
-	node->attributes = name->attributes;
+	node->absolute = (name->attributes & DNS_NAMEATTR_ABSOLUTE) != 0;
 
 	memmove(NAME(node), region.base, region.length);
 	memmove(OFFSETS(node), name->offsets, labels);