diff --git a/CHANGES b/CHANGES index 04ec09548e..aa9b6c8e85 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,7 @@ +4324. [bug] When deleting records from a zone database, interior + nodes could be left empty but not deleted, damaging + search performance afterward. [RT #40997] + 4323. [bug] Improve HTTP header processing on statschannel. [RT #41674] diff --git a/doc/arm/notes.xml b/doc/arm/notes.xml index 16d2928cb2..b5b31511fc 100644 --- a/doc/arm/notes.xml +++ b/doc/arm/notes.xml @@ -122,6 +122,13 @@
Bug Fixes + + + When deleting records from a zone database, interior nodes + could be left empty but not deleted, damaging search + performance afterward. [RT #40997] + + The server could crash due to a use-after-free if a diff --git a/lib/dns/rbt.c b/lib/dns/rbt.c index 989e5ff02b..65d5e56fd5 100644 --- a/lib/dns/rbt.c +++ b/lib/dns/rbt.c @@ -1371,54 +1371,91 @@ dns_rbt_deletenode(dns_rbt_t *rbt, dns_rbtnode_t *node, isc_boolean_t recurse) } } - /* - * Note the node that points to the level of the node that is being - * deleted. If the deleted node is the top level, parent will be set - * to NULL. - */ - parent = get_upper_node(node); + for (;;) { + /* + * Note the node that points to the level of the node + * that is being deleted. If the deleted node is the + * top level, parent will be set to NULL. + */ + parent = get_upper_node(node); - /* - * This node now has no down pointer (either because it didn't - * have one to start, or because it was recursively removed). - * So now the node needs to be removed from this level. - */ - dns_rbt_deletefromlevel(node, parent == NULL ? &rbt->root : + /* + * This node now has no down pointer (either because it didn't + * have one to start, or because it was recursively removed). + * So now the node needs to be removed from this level. + */ + dns_rbt_deletefromlevel(node, parent == NULL ? &rbt->root : &DOWN(parent)); - if (DATA(node) != NULL && rbt->data_deleter != NULL) - rbt->data_deleter(DATA(node), rbt->deleter_arg); + if (DATA(node) != NULL && rbt->data_deleter != NULL) + rbt->data_deleter(DATA(node), rbt->deleter_arg); - unhash_node(rbt, node); + unhash_node(rbt, node); #if DNS_RBT_USEMAGIC - node->magic = 0; + node->magic = 0; #endif - dns_rbtnode_refdestroy(node); - isc_mem_put(rbt->mctx, node, NODE_SIZE(node)); - rbt->nodecount--; + dns_rbtnode_refdestroy(node); + isc_mem_put(rbt->mctx, node, NODE_SIZE(node)); + rbt->nodecount--; - /* - * There are now two special cases that can exist that would - * not have existed if the tree had been created using only - * the names that now exist in it. (This is all related to - * join_nodes() as described in this function's introductory comment.) - * Both cases exist when the deleted node's parent (the node - * that pointed to the deleted node's level) is not null but - * it has no data: parent != NULL && DATA(parent) == NULL. - * - * The first case is that the deleted node was the last on its level: - * DOWN(parent) == NULL. This case can only exist if the parent was - * previously deleted -- and so now, apparently, the parent should go - * away. That can't be done though because there might be external - * references to it, such as through a nodechain. - * - * The other case also involves a parent with no data, but with the - * deleted node being the next-to-last node instead of the last: - * LEFT(DOWN(parent)) == NULL && RIGHT(DOWN(parent)) == NULL. - * Presumably now the remaining node on the level should be joined - * with the parent, but it's already been described why that can't be - * done. - */ + /* + * There are now two special cases that can exist that + * would not have existed if the tree had been created + * using only the names that now exist in it. (This is + * all related to join_nodes() as described in this + * function's introductory comment.) Both cases exist + * when the deleted node's parent (the node that pointed + * to the deleted node's level) is not null but it has + * no data: parent != NULL && DATA(parent) == NULL. + * + * The first case is that the deleted node was the last + * on its level: DOWN(parent) == NULL. This case can + * only exist if the parent was previously deleted -- + * and so now, apparently, the parent should go away. + * That can't be done though because there might be + * external references to it, such as through a + * nodechain. XXXMUKS: We don't care about this. + * + * The other case also involves a parent with no data, + * but with the deleted node being the next-to-last node + * instead of the last: LEFT(DOWN(parent)) == NULL && + * RIGHT(DOWN(parent)) == NULL. Presumably now the + * remaining node on the level should be joined with the + * parent, but it's already been described why that + * can't be done. + */ + + /* + * Is this the root?. + */ + if (parent == NULL) + break; + + /* + * If the node deletion did not cause the subtree to disappear + * completely, return early. + */ + if (DOWN(parent) != NULL) + break; + + /* + * If the upper node is not empty, it cannot be deleted. + */ + if (DATA(parent) != NULL) + break; + + /* + * If upper node is the root node (.), don't attempt to + * delete it. The root node must always exist. + */ + if (parent == rbt->root) + break; + + /* + * Ascend up the tree and delete the upper node. + */ + node = parent; + } /* * This function never fails. diff --git a/lib/dns/rbtdb.c b/lib/dns/rbtdb.c index b88956b9ba..056e688002 100644 --- a/lib/dns/rbtdb.c +++ b/lib/dns/rbtdb.c @@ -442,7 +442,7 @@ struct acachectl { #define NEGATIVE(header) \ (((header)->attributes & RDATASET_ATTR_NEGATIVE) != 0) -#define DEFAULT_NODE_LOCK_COUNT 7 /*%< Should be prime. */ +#define DEFAULT_NODE_LOCK_COUNT 523 /*%< Should be prime. */ /*% * Number of buckets for cache DB entries (locks, LRU lists, TTL heaps). @@ -1637,6 +1637,19 @@ delete_node(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node) INSIST(!ISC_LINK_LINKED(node, deadlink)); + if (isc_log_wouldlog(dns_lctx, ISC_LOG_DEBUG(1))) { + char printname[DNS_NAME_FORMATSIZE]; + isc_log_write(dns_lctx, + DNS_LOGCATEGORY_DATABASE, + DNS_LOGMODULE_CACHE, + ISC_LOG_DEBUG(1), + "delete_node(): %p %s (bucket %d)", + node, + dns_rbt_formatnodename(node, + printname, sizeof(printname)), + node->locknum); + } + switch (node->nsec) { case DNS_RBT_NSEC_NORMAL: #ifdef BIND9 @@ -1969,21 +1982,6 @@ decrement_reference(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, deadlink); } } else { - if (isc_log_wouldlog(dns_lctx, ISC_LOG_DEBUG(1))) { - char printname[DNS_NAME_FORMATSIZE]; - - isc_log_write(dns_lctx, - DNS_LOGCATEGORY_DATABASE, - DNS_LOGMODULE_CACHE, - ISC_LOG_DEBUG(1), - "decrement_reference: " - "delete from rbt: %p %s", - node, - dns_rbt_formatnodename(node, - printname, - sizeof(printname))); - } - delete_node(rbtdb, node); } } else {