avoid a race in the qpzone getsigningtime() implementation

the previous commit introduced a possible race in getsigningtime()
where the rdataset header could change between being found on the
heap and being bound.

getsigningtime() now looks at the first element of the heap, gathers the
locknum, locks the respective lock, and retrieves the header from the
heap again.  If the locknum has changed, it will rinse and repeat.
Theoretically, this could spin forever, but practically, it almost never
will as the heap changes on the zone are very rare.

we simplify matters further by changing the dns_db_getsigningtime()
API call. instead of passing back a bound rdataset, we pass back the
information the caller actually needed: the resigning time, owner name
and type of the rdataset that was first on the heap.

diff --git a/bin/named/server.c b/bin/named/server.c
index a543b839a905fd28c50ec14c23867521605e0b05..62818950fbff33aa8b27db628213b36dbdb23379 100644 (file)
--- a/bin/named/server.c
+++ b/bin/named/server.c
@@ -15081,29 +15081,26 @@ named_server_zonestatus(named_server_t *server, isc_lex_t *lex,
        {
                dns_name_t *name;
                dns_fixedname_t fixed;
-               dns_rdataset_t next;
+               isc_stdtime_t resign;
+               dns_typepair_t typepair;
 
-               dns_rdataset_init(&next);
                name = dns_fixedname_initname(&fixed);
 
-               result = dns_db_getsigningtime(db, &next, name);
+               result = dns_db_getsigningtime(db, &resign, name, &typepair);
                if (result == ISC_R_SUCCESS) {
                        char namebuf[DNS_NAME_FORMATSIZE];
                        char typebuf[DNS_RDATATYPE_FORMATSIZE];
 
+                       resign -= dns_zone_getsigresigninginterval(zone);
+
                        dns_name_format(name, namebuf, sizeof(namebuf));
-                       dns_rdatatype_format(next.covers, typebuf,
-                                            sizeof(typebuf));
+                       dns_rdatatype_format(DNS_TYPEPAIR_COVERS(typepair),
+                                            typebuf, sizeof(typebuf));
                        snprintf(resignbuf, sizeof(resignbuf), "%s/%s", namebuf,
                                 typebuf);
-                       isc_time_set(
-                               &resigntime,
-                               next.resign -
-                                       dns_zone_getsigresigninginterval(zone),
-                               0);
+                       isc_time_set(&resigntime, resign, 0);
                        isc_time_formathttptimestamp(&resigntime, rtbuf,
                                                     sizeof(rtbuf));
-                       dns_rdataset_disassociate(&next);
                }
        }
 

diff --git a/bin/tests/system/dyndb/driver/db.c b/bin/tests/system/dyndb/driver/db.c
index 521327d1254423302f89cc95ef3694eb5c9bef87..de26821b2c5c45e40b1684732ab6924a66dab1e3 100644 (file)
--- a/bin/tests/system/dyndb/driver/db.c
+++ b/bin/tests/system/dyndb/driver/db.c
@@ -363,14 +363,13 @@ setsigningtime(dns_db_t *db, dns_rdataset_t *rdataset, isc_stdtime_t resign) {
 }
 
 static isc_result_t
-getsigningtime(dns_db_t *db, dns_rdataset_t *rdataset,
-              dns_name_t *name DNS__DB_FLARG) {
+getsigningtime(dns_db_t *db, isc_stdtime_t *resign, dns_name_t *name,
+              dns_typepair_t *type) {
        sampledb_t *sampledb = (sampledb_t *)db;
 
        REQUIRE(VALID_SAMPLEDB(sampledb));
 
-       return (dns__db_getsigningtime(sampledb->rbtdb, rdataset,
-                                      name DNS__DB_FLARG_PASS));
+       return (dns_db_getsigningtime(sampledb->rbtdb, resign, name, type));
 }
 
 static dns_stats_t *

diff --git a/lib/dns/db.c b/lib/dns/db.c
index a5612e49fd6a39c824228e4040149ce6a4009df8..013065c26d3e3ff93feb9ccfc020432e9bb78c4a 100644 (file)
--- a/lib/dns/db.c
+++ b/lib/dns/db.c
@@ -957,11 +957,11 @@ dns_db_setsigningtime(dns_db_t *db, dns_rdataset_t *rdataset,
 }
 
 isc_result_t
-dns__db_getsigningtime(dns_db_t *db, dns_rdataset_t *rdataset,
-                      dns_name_t *name DNS__DB_FLARG) {
+dns_db_getsigningtime(dns_db_t *db, isc_stdtime_t *resign, dns_name_t *name,
+                     dns_typepair_t *typepair) {
        if (db->methods->getsigningtime != NULL) {
-               return ((db->methods->getsigningtime)(db, rdataset,
-                                                     name DNS__DB_FLARG_PASS));
+               return ((db->methods->getsigningtime)(db, resign, name,
+                                                     typepair));
        }
        return (ISC_R_NOTFOUND);
 }

diff --git a/lib/dns/include/dns/db.h b/lib/dns/include/dns/db.h
index 7643ec3debdbaadf5d7cdfa2fc3ef42b70d43191..c8089b66c67914561c36e49cb0fa7ca11a9eb4bc 100644 (file)
--- a/lib/dns/include/dns/db.h
+++ b/lib/dns/include/dns/db.h
@@ -147,8 +147,9 @@ typedef struct dns_dbmethods {
                                      dns_dbnode_t **nodep DNS__DB_FLARG);
        isc_result_t (*setsigningtime)(dns_db_t *db, dns_rdataset_t *rdataset,
                                       isc_stdtime_t resign);
-       isc_result_t (*getsigningtime)(dns_db_t *db, dns_rdataset_t *rdataset,
-                                      dns_name_t *name DNS__DB_FLARG);
+       isc_result_t (*getsigningtime)(dns_db_t *db, isc_stdtime_t *resign,
+                                      dns_name_t     *name,
+                                      dns_typepair_t *typepair);
        dns_stats_t *(*getrrsetstats)(dns_db_t *db);
        isc_result_t (*findnodeext)(dns_db_t *db, const dns_name_t *name,
                                    bool                     create,
@@ -1585,19 +1586,19 @@ dns_db_setsigningtime(dns_db_t *db, dns_rdataset_t *rdataset,
  * \li #ISC_R_NOTIMPLEMENTED - Not supported by this DB implementation.
  */
 
-#define dns_db_getsigningtime(db, rdataset, name) \
-       dns__db_getsigningtime(db, rdataset, name DNS__DB_FILELINE)
 isc_result_t
-dns__db_getsigningtime(dns_db_t *db, dns_rdataset_t *rdataset,
-                      dns_name_t *name DNS__DB_FLARG);
+dns_db_getsigningtime(dns_db_t *db, isc_stdtime_t *resign,
+                     dns_name_t *foundname, dns_typepair_t *typepair);
 /*%<
- * Return the rdataset with the earliest signing time in the zone.
- * Note: the rdataset is version agnostic.
+ * Find the rdataset header with the earliest signing time in a zone
+ * database. Update 'foundname' and 'typepair' with its name and
+ * type, and update 'resign' with the time at which it is to be signed.
  *
  * Requires:
  * \li 'db' is a valid zone database.
- * \li 'rdataset' to be initialized but not associated.
- * \li 'name' to be NULL or have a buffer associated with it.
+ * \li 'resign' is not NULL.
+ * \li 'foundname' is not NULL.
+ * \li 'typepair' is not NULL.
  *
  * Returns:
  * \li #ISC_R_SUCCESS

diff --git a/lib/dns/qpzone.c b/lib/dns/qpzone.c
index e1c63e9d342e3af664da7fb7e2b00df73eba2490..c8b9a7f9ee37d5782cf0ca8edff779ac44afebb1 100644 (file)
--- a/lib/dns/qpzone.c
+++ b/lib/dns/qpzone.c
@@ -188,7 +188,6 @@ struct qpzonedb {
        isc_loop_t *loop;
        struct rcu_head rcu_head;
 
-       isc_mutex_t heaplock;
        isc_heap_t *heap; /* Resigning heap */
 
        dns_qpmulti_t *tree;  /* Main QP trie for data storage */
@@ -468,7 +467,6 @@ free_db_rcu(struct rcu_head *rcu_head) {
        }
 
        isc_heap_destroy(&qpdb->heap);
-       isc_mutex_destroy(&qpdb->heaplock);
 
        if (qpdb->gluecachestats != NULL) {
                isc_stats_detach(&qpdb->gluecachestats);
@@ -657,7 +655,6 @@ dns__qpzone_create(isc_mem_t *mctx, const dns_name_t *origin, dns_dbtype_t type,
 
        qpdb->common.update_listeners = cds_lfht_new(16, 16, 0, 0, NULL);
 
-       isc_mutex_init(&qpdb->heaplock);
        isc_heap_create(mctx, resign_sooner, set_index, 0, &qpdb->heap);
 
        qpdb->active = qpdb->node_lock_count;
@@ -1268,12 +1265,12 @@ newversion(dns_db_t *db, dns_dbversion_t **versionp) {
 
 static void
 resigninsert(qpzonedb_t *qpdb, dns_slabheader_t *newheader) {
-       INSIST(newheader->heap_index == 0);
-       INSIST(!ISC_LINK_LINKED(newheader, link));
+       REQUIRE(newheader->heap_index == 0);
+       REQUIRE(!ISC_LINK_LINKED(newheader, link));
 
-       LOCK(&qpdb->heaplock);
+       RWLOCK(&qpdb->lock, isc_rwlocktype_write);
        isc_heap_insert(qpdb->heap, newheader);
-       UNLOCK(&qpdb->heaplock);
+       RWUNLOCK(&qpdb->lock, isc_rwlocktype_write);
 
        newheader->heap = qpdb->heap;
 }
@@ -1281,15 +1278,17 @@ resigninsert(qpzonedb_t *qpdb, dns_slabheader_t *newheader) {
 static void
 resigndelete(qpzonedb_t *qpdb, qpdb_version_t *version,
             dns_slabheader_t *header DNS__DB_FLARG) {
-       if (header != NULL && header->heap_index != 0) {
-               LOCK(&qpdb->heaplock);
-               isc_heap_delete(qpdb->heap, header->heap_index);
-               UNLOCK(&qpdb->heaplock);
-
-               header->heap_index = 0;
-               newref(qpdb, HEADERNODE(header) DNS__DB_FLARG_PASS);
-               ISC_LIST_APPEND(version->resigned_list, header, link);
+       if (header == NULL || header->heap_index == 0) {
+               return;
        }
+
+       RWLOCK(&qpdb->lock, isc_rwlocktype_write);
+       isc_heap_delete(qpdb->heap, header->heap_index);
+       RWUNLOCK(&qpdb->lock, isc_rwlocktype_write);
+
+       header->heap_index = 0;
+       newref(qpdb, HEADERNODE(header) DNS__DB_FLARG_PASS);
+       ISC_LIST_APPEND(version->resigned_list, header, link);
 }
 
 static void
@@ -2412,7 +2411,7 @@ setsigningtime(dns_db_t *db, dns_rdataset_t *rdataset, isc_stdtime_t resign) {
        }
        if (header->heap_index != 0) {
                INSIST(RESIGN(header));
-               LOCK(&qpdb->heaplock);
+               RWLOCK(&qpdb->lock, isc_rwlocktype_write);
                if (resign == 0) {
                        isc_heap_delete(qpdb->heap, header->heap_index);
                        header->heap_index = 0;
@@ -2422,7 +2421,7 @@ setsigningtime(dns_db_t *db, dns_rdataset_t *rdataset, isc_stdtime_t resign) {
                } else if (resign_sooner(&oldheader, header)) {
                        isc_heap_decreased(qpdb->heap, header->heap_index);
                }
-               UNLOCK(&qpdb->heaplock);
+               RWUNLOCK(&qpdb->lock, isc_rwlocktype_write);
        } else if (resign != 0) {
                DNS_SLABHEADER_SETATTR(header, DNS_SLABHEADERATTR_RESIGN);
                resigninsert(qpdb, header);
@@ -2433,33 +2432,52 @@ setsigningtime(dns_db_t *db, dns_rdataset_t *rdataset, isc_stdtime_t resign) {
 }
 
 static isc_result_t
-getsigningtime(dns_db_t *db, dns_rdataset_t *rdataset,
-              dns_name_t *foundname DNS__DB_FLARG) {
+getsigningtime(dns_db_t *db, isc_stdtime_t *resign, dns_name_t *foundname,
+              dns_typepair_t *typepair) {
        qpzonedb_t *qpdb = (qpzonedb_t *)db;
        dns_slabheader_t *header = NULL;
        isc_rwlocktype_t nlocktype = isc_rwlocktype_none;
+       uint16_t locknum;
+       isc_result_t result = ISC_R_NOTFOUND;
 
        REQUIRE(VALID_QPZONE(qpdb));
+       REQUIRE(resign != NULL);
+       REQUIRE(foundname != NULL);
+       REQUIRE(typepair != NULL);
 
-       LOCK(&qpdb->heaplock);
+       RWLOCK(&qpdb->lock, isc_rwlocktype_read);
        header = isc_heap_element(qpdb->heap, 1);
-       UNLOCK(&qpdb->heaplock);
-
        if (header == NULL) {
+               RWUNLOCK(&qpdb->lock, isc_rwlocktype_read);
                return (ISC_R_NOTFOUND);
        }
+       locknum = HEADERNODE(header)->locknum;
+       RWUNLOCK(&qpdb->lock, isc_rwlocktype_read);
 
-       NODE_RDLOCK(&qpdb->node_locks[HEADERNODE(header)->locknum].lock,
-                   &nlocktype);
-       bindrdataset(qpdb, HEADERNODE(header), header, 0,
-                    rdataset DNS__DB_FLARG_PASS);
-       if (foundname != NULL) {
+again:
+       NODE_RDLOCK(&qpdb->node_locks[locknum].lock, &nlocktype);
+
+       RWLOCK(&qpdb->lock, isc_rwlocktype_read);
+       header = isc_heap_element(qpdb->heap, 1);
+       if (header != NULL && HEADERNODE(header)->locknum != locknum) {
+               RWUNLOCK(&qpdb->lock, isc_rwlocktype_read);
+               NODE_UNLOCK(&qpdb->node_locks[locknum].lock, &nlocktype);
+               locknum = HEADERNODE(header)->locknum;
+               goto again;
+       }
+
+       if (header != NULL) {
+               *resign = RESIGN(header)
+                                 ? (header->resign << 1) | header->resign_lsb
+                                 : 0;
                dns_name_copy(&HEADERNODE(header)->name, foundname);
+               *typepair = header->type;
+               result = ISC_R_SUCCESS;
        }
-       NODE_UNLOCK(&qpdb->node_locks[HEADERNODE(header)->locknum].lock,
-                   &nlocktype);
+       RWUNLOCK(&qpdb->lock, isc_rwlocktype_read);
+       NODE_UNLOCK(&qpdb->node_locks[locknum].lock, &nlocktype);
 
-       return (ISC_R_SUCCESS);
+       return (result);
 }
 
 static isc_result_t
@@ -4008,9 +4026,9 @@ deletedata(dns_db_t *db ISC_ATTR_UNUSED, dns_dbnode_t *node ISC_ATTR_UNUSED,
        dns_slabheader_t *header = data;
 
        if (header->heap != NULL && header->heap_index != 0) {
-               LOCK(&qpdb->heaplock);
+               RWLOCK(&qpdb->lock, isc_rwlocktype_write);
                isc_heap_delete(header->heap, header->heap_index);
-               UNLOCK(&qpdb->heaplock);
+               RWUNLOCK(&qpdb->lock, isc_rwlocktype_write);
        }
        header->heap_index = 0;
 

diff --git a/lib/dns/rbt-zonedb.c b/lib/dns/rbt-zonedb.c
index f6d0afc517cbe24fc00e9f315b13f41e9ee7f97f..43599fa381db934fcd5e686db384ee1dfc8a0d34 100644 (file)
--- a/lib/dns/rbt-zonedb.c
+++ b/lib/dns/rbt-zonedb.c
@@ -1993,8 +1993,8 @@ setsigningtime(dns_db_t *db, dns_rdataset_t *rdataset, isc_stdtime_t resign) {
 }
 
 static isc_result_t
-getsigningtime(dns_db_t *db, dns_rdataset_t *rdataset,
-              dns_name_t *foundname DNS__DB_FLARG) {
+getsigningtime(dns_db_t *db, isc_stdtime_t *resign, dns_name_t *foundname,
+              dns_typepair_t *typepair) {
        dns_rbtdb_t *rbtdb = (dns_rbtdb_t *)db;
        dns_slabheader_t *header = NULL, *this = NULL;
        unsigned int i;
@@ -2004,6 +2004,9 @@ getsigningtime(dns_db_t *db, dns_rdataset_t *rdataset,
        isc_rwlocktype_t nlocktype = isc_rwlocktype_none;
 
        REQUIRE(VALID_RBTDB(rbtdb));
+       REQUIRE(resign != NULL);
+       REQUIRE(foundname != NULL);
+       REQUIRE(typepair != NULL);
 
        TREE_RDLOCK(&rbtdb->tree_lock, &tlocktype);
 
@@ -2055,14 +2058,11 @@ getsigningtime(dns_db_t *db, dns_rdataset_t *rdataset,
                 * Found something; pass back the answer and unlock
                 * the bucket.
                 */
-               dns__rbtdb_bindrdataset(rbtdb, RBTDB_HEADERNODE(header), header,
-                                       0, isc_rwlocktype_read,
-                                       rdataset DNS__DB_FLARG_PASS);
-
-               if (foundname != NULL) {
-                       dns_rbt_fullnamefromnode(RBTDB_HEADERNODE(header),
-                                                foundname);
-               }
+               *resign = RESIGN(header)
+                                 ? (header->resign << 1) | header->resign_lsb
+                                 : 0;
+               dns_rbt_fullnamefromnode(RBTDB_HEADERNODE(header), foundname);
+               *typepair = header->type;
 
                NODE_UNLOCK(&rbtdb->node_locks[locknum].lock, &nlocktype);
 

diff --git a/lib/dns/zone.c b/lib/dns/zone.c
index 4d6284c6b30f070f1bc979c2eb9f47aaa4d1dae9..34e771b5cbf3254eaab878ae9d228d80ee347c2f 100644 (file)
--- a/lib/dns/zone.c
+++ b/lib/dns/zone.c
@@ -3816,12 +3816,12 @@ cleanup:
 
 static void
 set_resigntime(dns_zone_t *zone) {
-       dns_rdataset_t rdataset;
        dns_fixedname_t fixed;
-       unsigned int resign;
+       isc_stdtime_t resign;
        isc_result_t result;
        uint32_t nanosecs;
        dns_db_t *db = NULL;
+       dns_typepair_t typepair;
 
        INSIST(LOCKED_ZONE(zone));
 
@@ -3834,7 +3834,6 @@ set_resigntime(dns_zone_t *zone) {
                return;
        }
 
-       dns_rdataset_init(&rdataset);
        dns_fixedname_init(&fixed);
 
        ZONEDB_LOCK(&zone->dblock, isc_rwlocktype_read);
@@ -3847,15 +3846,14 @@ set_resigntime(dns_zone_t *zone) {
                return;
        }
 
-       result = dns_db_getsigningtime(db, &rdataset,
-                                      dns_fixedname_name(&fixed));
+       result = dns_db_getsigningtime(db, &resign, dns_fixedname_name(&fixed),
+                                      &typepair);
        if (result != ISC_R_SUCCESS) {
                isc_time_settoepoch(&zone->resigntime);
                goto cleanup;
        }
 
-       resign = rdataset.resign - dns_zone_getsigresigninginterval(zone);
-       dns_rdataset_disassociate(&rdataset);
+       resign -= dns_zone_getsigresigninginterval(zone);
        nanosecs = isc_random_uniform(1000000000);
        isc_time_set(&zone->resigntime, resign, nanosecs);
 
@@ -5179,31 +5177,32 @@ zone_postload(dns_zone_t *zone, dns_db_t *db, isc_time_t loadtime,
                if (zone->type == dns_zone_primary && is_dynamic &&
                    dns_db_issecure(db) && !inline_raw(zone))
                {
+                       isc_stdtime_t resign;
                        dns_name_t *name;
                        dns_fixedname_t fixed;
-                       dns_rdataset_t next;
+                       dns_typepair_t typepair;
 
-                       dns_rdataset_init(&next);
                        name = dns_fixedname_initname(&fixed);
 
-                       result = dns_db_getsigningtime(db, &next, name);
+                       result = dns_db_getsigningtime(db, &resign, name,
+                                                      &typepair);
                        if (result == ISC_R_SUCCESS) {
                                isc_stdtime_t timenow = isc_stdtime_now();
                                char namebuf[DNS_NAME_FORMATSIZE];
                                char typebuf[DNS_RDATATYPE_FORMATSIZE];
 
                                dns_name_format(name, namebuf, sizeof(namebuf));
-                               dns_rdatatype_format(next.covers, typebuf,
-                                                    sizeof(typebuf));
+                               dns_rdatatype_format(
+                                       DNS_TYPEPAIR_COVERS(typepair), typebuf,
+                                       sizeof(typebuf));
                                dnssec_log(
                                        zone, ISC_LOG_DEBUG(3),
                                        "next resign: %s/%s "
                                        "in %d seconds",
                                        namebuf, typebuf,
-                                       next.resign - timenow -
+                                       resign - timenow -
                                                dns_zone_getsigresigninginterval(
                                                        zone));
-                               dns_rdataset_disassociate(&next);
                        } else {
                                dnssec_log(zone, ISC_LOG_WARNING,
                                           "signed dynamic zone has no "
@@ -6966,18 +6965,16 @@ zone_resigninc(dns_zone_t *zone) {
        dns__zonediff_t zonediff;
        dns_fixedname_t fixed;
        dns_name_t *name;
-       dns_rdataset_t rdataset;
-       dns_rdatatype_t covers;
+       dns_typepair_t typepair;
        dst_key_t *zone_keys[DNS_MAXZONEKEYS];
        isc_result_t result;
        isc_stdtime_t now, inception, soaexpire, expire, fullexpire, stop;
        unsigned int i;
        unsigned int nkeys = 0;
-       unsigned int resign;
+       isc_stdtime_t resign;
 
        ENTER;
 
-       dns_rdataset_init(&rdataset);
        dns_diff_init(zone->mctx, &_sig_diff);
        zonediff_init(&zonediff, &_sig_diff);
 
@@ -7024,7 +7021,7 @@ zone_resigninc(dns_zone_t *zone) {
        stop = now + 5;
 
        name = dns_fixedname_initname(&fixed);
-       result = dns_db_getsigningtime(db, &rdataset, name);
+       result = dns_db_getsigningtime(db, &resign, name, &typepair);
        if (result != ISC_R_SUCCESS && result != ISC_R_NOTFOUND) {
                dns_zone_log(zone, ISC_LOG_ERROR,
                             "zone_resigninc:dns_db_getsigningtime -> %s",
@@ -7033,10 +7030,9 @@ zone_resigninc(dns_zone_t *zone) {
 
        i = 0;
        while (result == ISC_R_SUCCESS) {
-               resign = rdataset.resign -
-                        dns_zone_getsigresigninginterval(zone);
-               covers = rdataset.covers;
-               dns_rdataset_disassociate(&rdataset);
+               dns_rdatatype_t covers = DNS_TYPEPAIR_COVERS(typepair);
+
+               resign -= dns_zone_getsigresigninginterval(zone);
 
                /*
                 * Stop if we hit the SOA as that means we have walked the
@@ -7076,7 +7072,7 @@ zone_resigninc(dns_zone_t *zone) {
                                     isc_result_totext(result));
                        break;
                }
-               result = dns_db_getsigningtime(db, &rdataset, name);
+               result = dns_db_getsigningtime(db, &resign, name, &typepair);
                if (nkeys == 0 && result == ISC_R_NOTFOUND) {
                        result = ISC_R_SUCCESS;
                        break;