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 24d10740e8518dafdc54311f308d9e4d3d38db45..ce61fa2aafb2e5de10c2f4e8bd57451413065360 100644 (file)
--- 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 77de2b650c364bd81ca5c1bbdba91b39bc373239..fe766714884363bf7c8396d36acf3dccbbfa1a2c 100644 (file)
--- 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);