From db06cd726c469887d302905e1d88c1db0d8a323b Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Thu, 3 Mar 2016 21:15:21 -0800 Subject: [PATCH] [v9_10] recursively clean empty interior nodes when deleting database records 4324. [bug] When deleting records from a zone database, interior nodes could be left empty but not deleted, damaging search performance afterward. [RT #40997] (cherry picked from commit 44c86318ed432af96848269250930297eea2bba3) --- CHANGES | 4 ++ doc/arm/notes.xml | 7 +++ lib/dns/rbt.c | 118 ++++++++++++++++++++++++++++++---------------- lib/dns/rbtdb.c | 30 ++++++------ 4 files changed, 103 insertions(+), 56 deletions(-) diff --git a/CHANGES b/CHANGES index 3abfe276e0..5789f02fb3 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 6e8ff401e1..736b8e33c2 100644 --- a/doc/arm/notes.xml +++ b/doc/arm/notes.xml @@ -137,6 +137,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 5812af664c..85b6b29593 100644 --- a/lib/dns/rbt.c +++ b/lib/dns/rbt.c @@ -2114,53 +2114,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. - */ - deletefromlevel(node, parent == NULL ? &rbt->root : &DOWN(parent)); + /* + * 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. + */ + 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); + dns_rbtnode_refdestroy(node); - freenode(rbt, &node); + freenode(rbt, &node); - /* - * 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 e6281d2a28..b1c725dd58 100644 --- a/lib/dns/rbtdb.c +++ b/lib/dns/rbtdb.c @@ -512,7 +512,7 @@ struct acachectl { #define PREFETCH(header) \ (((header)->attributes & RDATASET_ATTR_PREFETCH) != 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). @@ -1844,6 +1844,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: /* @@ -2180,21 +2193,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 {