From: Mark Andrews Date: Tue, 22 Mar 2016 23:38:05 +0000 (+1100) Subject: 4338. [bug] Reimplement change 4324 as it wasn't properly doing X-Git-Tag: v9.11.0a1~17 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=558278974eb4a1021943e6bbbc6c7d80dc3707fd;p=thirdparty%2Fbind9.git 4338. [bug] Reimplement change 4324 as it wasn't properly doing all the required book keeping. [RT #41941] --- diff --git a/CHANGES b/CHANGES index 9ae68b39ca1..322ac635032 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,6 @@ +4338. [bug] Reimplement change 4324 as it wasn't properly doing + all the required book keeping. [RT #41941] + 4337. [bug] The previous change exposed a latent flaw in key refresh queries for managed-keys when a cached DNSKEY had TTL 0. [RT #41986] diff --git a/lib/dns/rbt.c b/lib/dns/rbt.c index 85b6b295934..1e2257bb43a 100644 --- a/lib/dns/rbt.c +++ b/lib/dns/rbt.c @@ -2114,91 +2114,29 @@ dns_rbt_deletenode(dns_rbt_t *rbt, dns_rbtnode_t *node, isc_boolean_t recurse) } } - 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); + /* + * 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, so now it 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); - - 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. 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; + dns_rbtnode_refdestroy(node); - /* - * 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; - } + freenode(rbt, &node); /* * This function never fails. diff --git a/lib/dns/rbtdb.c b/lib/dns/rbtdb.c index ee1e4b5ec56..d831162001f 100644 --- a/lib/dns/rbtdb.c +++ b/lib/dns/rbtdb.c @@ -1977,6 +1977,24 @@ delete_node(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node) { } } +/* + * Caller must be holding the node lock. + */ +static inline void +new_reference(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node) { + unsigned int lockrefs, noderefs; + isc_refcount_t *lockref; + + INSIST(!ISC_LINK_LINKED(node, deadlink)); + dns_rbtnode_refincrement0(node, &noderefs); + if (noderefs == 1) { /* this is the first reference to the node */ + lockref = &rbtdb->node_locks[node->locknum].references; + isc_refcount_increment0(lockref, &lockrefs); + INSIST(lockrefs != 0); + } + INSIST(noderefs != 0); +} + /*% * Clean up dead nodes. These are nodes which have no references, and * have no data. They are dead but we could not or chose not to delete @@ -2001,31 +2019,35 @@ cleanup_dead_nodes(dns_rbtdb_t *rbtdb, int bucketnum) { INSIST(dns_rbtnode_refcurrent(node) == 0 && node->data == NULL); - delete_node(rbtdb, node); + if (node->parent != NULL && + node->parent->down == node && node->left == NULL && + node->right == NULL && rbtdb->task != NULL) + { + isc_event_t *ev; + dns_db_t *db; + ev = isc_event_allocate(rbtdb->common.mctx, NULL, + DNS_EVENT_RBTPRUNE, + prune_tree, node, + sizeof(isc_event_t)); + if (ev != NULL) { + new_reference(rbtdb, node); + db = NULL; + attach((dns_db_t *)rbtdb, &db); + ev->ev_sender = db; + isc_task_send(rbtdb->task, &ev); + } else { + ISC_LIST_APPEND(rbtdb->deadnodes[bucketnum], + node, deadlink); + } + } else { + delete_node(rbtdb, node); + } node = ISC_LIST_HEAD(rbtdb->deadnodes[bucketnum]); count--; } } -/* - * Caller must be holding the node lock. - */ -static inline void -new_reference(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node) { - unsigned int lockrefs, noderefs; - isc_refcount_t *lockref; - - INSIST(!ISC_LINK_LINKED(node, deadlink)); - dns_rbtnode_refincrement0(node, &noderefs); - if (noderefs == 1) { /* this is the first reference to the node */ - lockref = &rbtdb->node_locks[node->locknum].references; - isc_refcount_increment0(lockref, &lockrefs); - INSIST(lockrefs != 0); - } - INSIST(noderefs != 0); -} - /* * This function is assumed to be called when a node is newly referenced * and can be in the deadnode list. In that case the node must be retrieved