Testing node->down requires the tree lock to be held.

In decrement_reference only test node->down if the tree lock
is held.  As node->down is not always tested in
decrement_reference we need to test that it is non NULL in
cleanup_dead_nodes prior to removing the node from the rbt
tree.  Additionally it is not always possible to aquire the
node lock and reactivate a node when adding parent nodes.
Reactivate such nodes in cleanup_dead_nodes if required.

diff --git a/lib/dns/rbtdb.c b/lib/dns/rbtdb.c
index 7ce3052286737d8409384e77babad6664fec67d8..e0914f0b95319ab2e54398bdab60248d0ed7c161 100644 (file)
--- a/lib/dns/rbtdb.c
+++ b/lib/dns/rbtdb.c
@@ -1844,6 +1844,10 @@ cleanup_dead_nodes(dns_rbtdb_t *rbtdb, int bucketnum) {
                /*
                 * Since we're holding a tree write lock, it should be
                 * impossible for this node to be referenced by others.
+                *
+                * decrement_reference may not have tested node->down, as
+                * the tree_lock was not held, before adding the node to
+                * deadnodes so we test it here.
                 */
                INSIST(isc_refcount_current(&node->references) == 0 &&
                       node->data == NULL);
@@ -1864,8 +1868,19 @@ cleanup_dead_nodes(dns_rbtdb_t *rbtdb, int bucketnum) {
                        attach((dns_db_t *)rbtdb, &db);
                        ev->ev_sender = db;
                        isc_task_send(rbtdb->task, &ev);
-               } else {
+               } else if (node->down == NULL && node->data == NULL) {
+                       /*
+                        * Not a interior node and not needing to be
+                        * reactivated.
+                        */
                        delete_node(rbtdb, node);
+               } else if (node->data == NULL) {
+                       /*
+                        * A interior node without data. Leave linked to
+                        * to be cleaned up when node->down becomes NULL.
+                        */
+                       ISC_LIST_APPEND(rbtdb->deadnodes[bucketnum],
+                                       node, deadlink);
                }
                node = ISC_LIST_HEAD(rbtdb->deadnodes[bucketnum]);
                count--;
@@ -1945,6 +1960,7 @@ decrement_reference(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node,
 {
        isc_result_t result;
        bool write_locked;
+       bool locked = tlock != isc_rwlocktype_none;
        rbtdb_nodelock_t *nodelock;
        int bucket = node->locknum;
        bool no_reference = true;
@@ -1952,12 +1968,12 @@ decrement_reference(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node,
 
        nodelock = &rbtdb->node_locks[bucket];
 
-#define KEEP_NODE(n, r) \
-       ((n)->data != NULL || (n)->down != NULL || \
+#define KEEP_NODE(n, r, l) \
+       ((n)->data != NULL || ((l) && (n)->down != NULL) || \
         (n) == (r)->origin_node || (n) == (r)->nsec3_origin_node)
 
        /* Handle easy and typical case first. */
-       if (!node->dirty && KEEP_NODE(node, rbtdb)) {
+       if (!node->dirty && KEEP_NODE(node, rbtdb, locked)) {
                if (isc_refcount_decrement(&node->references) == 1) {
                        refs = isc_refcount_decrement(&nodelock->references);
                        INSIST(refs > 0);
@@ -2024,8 +2040,9 @@ decrement_reference(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node,
        refs = isc_refcount_decrement(&nodelock->references);
        INSIST(refs > 0);
 
-       if (KEEP_NODE(node, rbtdb))
+       if (KEEP_NODE(node, rbtdb, locked || write_locked)) {
                goto restore_locks;
+       }
 
 #undef KEEP_NODE