]> git.ipfire.org Git - thirdparty/bind9.git/commitdiff
fix several bugs in the RBTDB dbiterator implementation
authorEvan Hunt <each@isc.org>
Wed, 14 Feb 2024 20:58:01 +0000 (12:58 -0800)
committerEvan Hunt <each@isc.org>
Thu, 15 Feb 2024 18:15:50 +0000 (10:15 -0800)
- the DNS_DB_NSEC3ONLY and DNS_DB_NONSEC3 flags are mutually
  exclusive; it never made sense to set both at the same time.
  to enforce this, it is now a fatal error to do so.  the
  dbiterator implementation has been cleaned up to remove
  code that treated the two as independent: if nonsec3 is
  true, we can be certain nsec3only is false, and vice versa.
- previously, iterating a database backwards omitted
  NSEC3 records even if DNS_DB_NONSEC3 had not been set. this
  has been corrected.
- when an iterator reaches the origin node of the NSEC3 tree, we
  need to skip over it and go to the next node in the sequence.
  the NSEC3 origin node is there for housekeeping purposes and
  never contains data.
- the dbiterator_test unit test has been expanded, several
  incorrect expectations have been fixed. (for example, the
  expected number of iterations has been reduced by one; we were
  previously counting the NSEC3 origin node and we should not
  have been doing so.)

lib/dns/db.c
lib/dns/include/dns/db.h
lib/dns/rbtdb.c
tests/dns/dbiterator_test.c

index e7f5d64446fb7142586a48a267749e5716713be9..0624b0d264e03d66990ee402f34fcd61011fb983 100644 (file)
@@ -608,6 +608,8 @@ dns_db_createiterator(dns_db_t *db, unsigned int flags,
 
        REQUIRE(DNS_DB_VALID(db));
        REQUIRE(iteratorp != NULL && *iteratorp == NULL);
+       REQUIRE((flags & (DNS_DB_NSEC3ONLY | DNS_DB_NONSEC3)) !=
+               (DNS_DB_NSEC3ONLY | DNS_DB_NONSEC3));
 
        if (db->methods->createiterator != NULL) {
                return (db->methods->createiterator(db, flags, iteratorp));
index c82c694c5a4097267cb3fe0b5c1b0831d08b8a89..b6362d99eedb31661877e14b40954013849f84bb 100644 (file)
@@ -1089,6 +1089,8 @@ dns_db_createiterator(dns_db_t *db, unsigned int options,
  *
  * \li iteratorp != NULL && *iteratorp == NULL
  *
+ * \li 'flags' contains at most one of #DNS_DB_NSEC3ONLY and #DNS_DB_NONSEC3.
+ *
  * Ensures:
  *
  * \li On success, *iteratorp will be a valid database iterator.
index 58a0f46eebe4b4d3a55da10a17675140a80cbcd3..9da26c64ce5f63627ccf81be32a67d73892574d1 100644 (file)
 
 #define KEEPSTALE(rbtdb) ((rbtdb)->common.serve_stale_ttl > 0)
 
+#define RBTDBITER_NSEC3_ORIGIN_NODE(rbtdb, iterator)       \
+       ((iterator)->current == &(iterator)->nsec3chain && \
+        (iterator)->node == (rbtdb)->nsec3_origin_node)
+
 /*%
  * Number of buckets for cache DB entries (locks, LRU lists, TTL heaps).
  * There is a tradeoff issue about configuring this value: if this is too
@@ -228,8 +232,7 @@ typedef struct rbtdb_dbiterator {
        dns_rbtnodechain_t nsec3chain;
        dns_rbtnodechain_t *current;
        dns_rbtnode_t *node;
-       bool nsec3only;
-       bool nonsec3;
+       enum { full, nonsec3, nsec3only } nsec3mode;
 } rbtdb_dbiterator_t;
 
 static void
@@ -2340,6 +2343,8 @@ dns__rbtdb_createiterator(dns_db_t *db, unsigned int options,
        rbtdb_dbiterator_t *rbtdbiter = NULL;
 
        REQUIRE(VALID_RBTDB(rbtdb));
+       REQUIRE((options & (DNS_DB_NSEC3ONLY | DNS_DB_NONSEC3)) !=
+               (DNS_DB_NSEC3ONLY | DNS_DB_NONSEC3));
 
        rbtdbiter = isc_mem_get(rbtdb->common.mctx, sizeof(*rbtdbiter));
 
@@ -2355,11 +2360,16 @@ dns__rbtdb_createiterator(dns_db_t *db, unsigned int options,
        dns_fixedname_init(&rbtdbiter->name);
        dns_fixedname_init(&rbtdbiter->origin);
        rbtdbiter->node = NULL;
-       rbtdbiter->nsec3only = ((options & DNS_DB_NSEC3ONLY) != 0);
-       rbtdbiter->nonsec3 = ((options & DNS_DB_NONSEC3) != 0);
+       if ((options & DNS_DB_NSEC3ONLY) != 0) {
+               rbtdbiter->nsec3mode = nsec3only;
+       } else if ((options & DNS_DB_NONSEC3) != 0) {
+               rbtdbiter->nsec3mode = nonsec3;
+       } else {
+               rbtdbiter->nsec3mode = full;
+       }
        dns_rbtnodechain_init(&rbtdbiter->chain);
        dns_rbtnodechain_init(&rbtdbiter->nsec3chain);
-       if (rbtdbiter->nsec3only) {
+       if (rbtdbiter->nsec3mode == nsec3only) {
                rbtdbiter->current = &rbtdbiter->nsec3chain;
        } else {
                rbtdbiter->current = &rbtdbiter->chain;
@@ -4387,23 +4397,48 @@ dbiterator_first(dns_dbiterator_t *iterator DNS__DB_FLARG) {
        dns_rbtnodechain_reset(&rbtdbiter->chain);
        dns_rbtnodechain_reset(&rbtdbiter->nsec3chain);
 
-       if (rbtdbiter->nsec3only) {
+       switch (rbtdbiter->nsec3mode) {
+       case nsec3only:
                rbtdbiter->current = &rbtdbiter->nsec3chain;
                result = dns_rbtnodechain_first(rbtdbiter->current,
                                                rbtdb->nsec3, name, origin);
-       } else {
+               break;
+       case nonsec3:
+               rbtdbiter->current = &rbtdbiter->chain;
+               result = dns_rbtnodechain_first(rbtdbiter->current, rbtdb->tree,
+                                               name, origin);
+               break;
+       case full:
                rbtdbiter->current = &rbtdbiter->chain;
                result = dns_rbtnodechain_first(rbtdbiter->current, rbtdb->tree,
                                                name, origin);
-               if (!rbtdbiter->nonsec3 && result == ISC_R_NOTFOUND) {
+               if (result == ISC_R_NOTFOUND) {
                        rbtdbiter->current = &rbtdbiter->nsec3chain;
                        result = dns_rbtnodechain_first(
                                rbtdbiter->current, rbtdb->nsec3, name, origin);
                }
+               break;
+       default:
+               UNREACHABLE();
        }
+
        if (result == ISC_R_SUCCESS || result == DNS_R_NEWORIGIN) {
                result = dns_rbtnodechain_current(rbtdbiter->current, NULL,
                                                  NULL, &rbtdbiter->node);
+
+               /* If we're in the NSEC3 tree, skip the origin */
+               if (RBTDBITER_NSEC3_ORIGIN_NODE(rbtdb, rbtdbiter)) {
+                       rbtdbiter->node = NULL;
+                       result = dns_rbtnodechain_next(rbtdbiter->current, name,
+                                                      origin);
+                       if (result == ISC_R_SUCCESS ||
+                           result == DNS_R_NEWORIGIN)
+                       {
+                               result = dns_rbtnodechain_current(
+                                       rbtdbiter->current, NULL, NULL,
+                                       &rbtdbiter->node);
+                       }
+               }
                if (result == ISC_R_SUCCESS) {
                        rbtdbiter->new_origin = true;
                        reference_iter_node(rbtdbiter DNS__DB_FLARG_PASS);
@@ -4448,20 +4483,61 @@ dbiterator_last(dns_dbiterator_t *iterator DNS__DB_FLARG) {
        dns_rbtnodechain_reset(&rbtdbiter->chain);
        dns_rbtnodechain_reset(&rbtdbiter->nsec3chain);
 
-       result = ISC_R_NOTFOUND;
-       if (rbtdbiter->nsec3only && !rbtdbiter->nonsec3) {
+       switch (rbtdbiter->nsec3mode) {
+       case nsec3only:
                rbtdbiter->current = &rbtdbiter->nsec3chain;
                result = dns_rbtnodechain_last(rbtdbiter->current, rbtdb->nsec3,
                                               name, origin);
-       }
-       if (!rbtdbiter->nsec3only && result == ISC_R_NOTFOUND) {
+               break;
+       case nonsec3:
                rbtdbiter->current = &rbtdbiter->chain;
                result = dns_rbtnodechain_last(rbtdbiter->current, rbtdb->tree,
                                               name, origin);
+               break;
+       case full:
+               rbtdbiter->current = &rbtdbiter->nsec3chain;
+               result = dns_rbtnodechain_last(rbtdbiter->current, rbtdb->nsec3,
+                                              name, origin);
+               if (result == ISC_R_NOTFOUND) {
+                       rbtdbiter->current = &rbtdbiter->chain;
+                       result = dns_rbtnodechain_last(
+                               rbtdbiter->current, rbtdb->tree, name, origin);
+               }
+               break;
+       default:
+               UNREACHABLE();
        }
+
        if (result == ISC_R_SUCCESS || result == DNS_R_NEWORIGIN) {
                result = dns_rbtnodechain_current(rbtdbiter->current, NULL,
                                                  NULL, &rbtdbiter->node);
+               if (RBTDBITER_NSEC3_ORIGIN_NODE(rbtdb, rbtdbiter)) {
+                       /*
+                        * NSEC3 tree only has an origin node.
+                        */
+                       rbtdbiter->node = NULL;
+                       switch (rbtdbiter->nsec3mode) {
+                       case nsec3only:
+                               result = ISC_R_NOMORE;
+                               break;
+                       case nonsec3:
+                       case full:
+                               rbtdbiter->current = &rbtdbiter->chain;
+                               result = dns_rbtnodechain_last(
+                                       rbtdbiter->current, rbtdb->tree, name,
+                                       origin);
+                               if (result == ISC_R_SUCCESS ||
+                                   result == DNS_R_NEWORIGIN)
+                               {
+                                       result = dns_rbtnodechain_current(
+                                               rbtdbiter->current, NULL, NULL,
+                                               &rbtdbiter->node);
+                               }
+                               break;
+                       default:
+                               UNREACHABLE();
+                       }
+               }
                if (result == ISC_R_SUCCESS) {
                        rbtdbiter->new_origin = true;
                        reference_iter_node(rbtdbiter DNS__DB_FLARG_PASS);
@@ -4503,17 +4579,20 @@ dbiterator_seek(dns_dbiterator_t *iterator,
        dns_rbtnodechain_reset(&rbtdbiter->chain);
        dns_rbtnodechain_reset(&rbtdbiter->nsec3chain);
 
-       if (rbtdbiter->nsec3only) {
+       switch (rbtdbiter->nsec3mode) {
+       case nsec3only:
                rbtdbiter->current = &rbtdbiter->nsec3chain;
                result = dns_rbt_findnode(rbtdb->nsec3, name, NULL,
                                          &rbtdbiter->node, rbtdbiter->current,
                                          DNS_RBTFIND_EMPTYDATA, NULL, NULL);
-       } else if (rbtdbiter->nonsec3) {
+               break;
+       case nonsec3:
                rbtdbiter->current = &rbtdbiter->chain;
                result = dns_rbt_findnode(rbtdb->tree, name, NULL,
                                          &rbtdbiter->node, rbtdbiter->current,
                                          DNS_RBTFIND_EMPTYDATA, NULL, NULL);
-       } else {
+               break;
+       case full:
                /*
                 * Stay on main chain if not found on either chain.
                 */
@@ -4533,6 +4612,9 @@ dbiterator_seek(dns_dbiterator_t *iterator,
                                result = tresult;
                        }
                }
+               break;
+       default:
+               UNREACHABLE();
        }
 
        if (result == ISC_R_SUCCESS || result == DNS_R_PARTIALMATCH) {
@@ -4572,11 +4654,29 @@ dbiterator_prev(dns_dbiterator_t *iterator DNS__DB_FLARG) {
                resume_iteration(rbtdbiter);
        }
 
+       dereference_iter_node(rbtdbiter DNS__DB_FLARG_PASS);
+
        name = dns_fixedname_name(&rbtdbiter->name);
        origin = dns_fixedname_name(&rbtdbiter->origin);
        result = dns_rbtnodechain_prev(rbtdbiter->current, name, origin);
-       if (result == ISC_R_NOMORE && !rbtdbiter->nsec3only &&
-           !rbtdbiter->nonsec3 && &rbtdbiter->nsec3chain == rbtdbiter->current)
+       if (rbtdbiter->current == &rbtdbiter->nsec3chain &&
+           (result == ISC_R_SUCCESS || result == DNS_R_NEWORIGIN))
+       {
+               /*
+                * If we're in the NSEC3 tree, it's empty or we've
+                * reached the origin, then we're done with it.
+                */
+               result = dns_rbtnodechain_current(rbtdbiter->current, NULL,
+                                                 NULL, &rbtdbiter->node);
+               if (result == ISC_R_NOTFOUND ||
+                   RBTDBITER_NSEC3_ORIGIN_NODE(rbtdb, rbtdbiter))
+               {
+                       rbtdbiter->node = NULL;
+                       result = ISC_R_NOMORE;
+               }
+       }
+       if (result == ISC_R_NOMORE && rbtdbiter->nsec3mode != nsec3only &&
+           &rbtdbiter->nsec3chain == rbtdbiter->current)
        {
                rbtdbiter->current = &rbtdbiter->chain;
                dns_rbtnodechain_reset(rbtdbiter->current);
@@ -4587,8 +4687,6 @@ dbiterator_prev(dns_dbiterator_t *iterator DNS__DB_FLARG) {
                }
        }
 
-       dereference_iter_node(rbtdbiter DNS__DB_FLARG_PASS);
-
        if (result == DNS_R_NEWORIGIN || result == ISC_R_SUCCESS) {
                rbtdbiter->new_origin = (result == DNS_R_NEWORIGIN);
                result = dns_rbtnodechain_current(rbtdbiter->current, NULL,
@@ -4624,8 +4722,8 @@ dbiterator_next(dns_dbiterator_t *iterator DNS__DB_FLARG) {
        name = dns_fixedname_name(&rbtdbiter->name);
        origin = dns_fixedname_name(&rbtdbiter->origin);
        result = dns_rbtnodechain_next(rbtdbiter->current, name, origin);
-       if (result == ISC_R_NOMORE && !rbtdbiter->nsec3only &&
-           !rbtdbiter->nonsec3 && &rbtdbiter->chain == rbtdbiter->current)
+       if (result == ISC_R_NOMORE && rbtdbiter->nsec3mode != nonsec3 &&
+           &rbtdbiter->chain == rbtdbiter->current)
        {
                rbtdbiter->current = &rbtdbiter->nsec3chain;
                dns_rbtnodechain_reset(rbtdbiter->current);
@@ -4639,9 +4737,25 @@ dbiterator_next(dns_dbiterator_t *iterator DNS__DB_FLARG) {
        dereference_iter_node(rbtdbiter DNS__DB_FLARG_PASS);
 
        if (result == DNS_R_NEWORIGIN || result == ISC_R_SUCCESS) {
+               /*
+                * If we've just started the NSEC3 tree,
+                * skip over the origin.
+                */
                rbtdbiter->new_origin = (result == DNS_R_NEWORIGIN);
                result = dns_rbtnodechain_current(rbtdbiter->current, NULL,
                                                  NULL, &rbtdbiter->node);
+               if (RBTDBITER_NSEC3_ORIGIN_NODE(rbtdb, rbtdbiter)) {
+                       rbtdbiter->node = NULL;
+                       result = dns_rbtnodechain_next(rbtdbiter->current, name,
+                                                      origin);
+                       if (result == ISC_R_SUCCESS ||
+                           result == DNS_R_NEWORIGIN)
+                       {
+                               result = dns_rbtnodechain_current(
+                                       rbtdbiter->current, NULL, NULL,
+                                       &rbtdbiter->node);
+                       }
+               }
        }
        if (result == ISC_R_SUCCESS) {
                reference_iter_node(rbtdbiter DNS__DB_FLARG_PASS);
index 2c65a1d10ed47271b59da0f37ce165c3da9cf4ae..43c0c796157c20e9d4fa390b5286e70b2c3e9ea9 100644 (file)
@@ -74,7 +74,7 @@ ISC_RUN_TEST_IMPL(create_nsec3) {
 
 /* walk: walk a database */
 static void
-test_walk(const char *filename, int nodes) {
+test_walk(const char *filename, int flags, int nodes) {
        isc_result_t result;
        dns_db_t *db = NULL;
        dns_dbiterator_t *iter = NULL;
@@ -88,7 +88,7 @@ test_walk(const char *filename, int nodes) {
        result = dns_test_loaddb(&db, dns_dbtype_zone, TEST_ORIGIN, filename);
        assert_int_equal(result, ISC_R_SUCCESS);
 
-       result = dns_db_createiterator(db, 0, &iter);
+       result = dns_db_createiterator(db, flags, &iter);
        assert_int_equal(result, ISC_R_SUCCESS);
 
        for (result = dns_dbiterator_first(iter); result == ISC_R_SUCCESS;
@@ -112,18 +112,26 @@ test_walk(const char *filename, int nodes) {
 ISC_RUN_TEST_IMPL(walk) {
        UNUSED(state);
 
-       test_walk(TESTS_DIR "/testdata/dbiterator/zone1.data", 13);
+       test_walk(TESTS_DIR "/testdata/dbiterator/zone1.data", 0, 12);
+       test_walk(TESTS_DIR "/testdata/dbiterator/zone1.data", DNS_DB_NONSEC3,
+                 12);
+       test_walk(TESTS_DIR "/testdata/dbiterator/zone1.data", DNS_DB_NSEC3ONLY,
+                 0);
 }
 
 ISC_RUN_TEST_IMPL(walk_nsec3) {
        UNUSED(state);
 
-       test_walk(TESTS_DIR "/testdata/dbiterator/zone2.data", 33);
+       test_walk(TESTS_DIR "/testdata/dbiterator/zone2.data", 0, 32);
+       test_walk(TESTS_DIR "/testdata/dbiterator/zone2.data", DNS_DB_NONSEC3,
+                 12);
+       test_walk(TESTS_DIR "/testdata/dbiterator/zone2.data", DNS_DB_NSEC3ONLY,
+                 20);
 }
 
 /* reverse: walk database backwards */
 static void
-test_reverse(const char *filename) {
+test_reverse(const char *filename, int flags, int nodes) {
        isc_result_t result;
        dns_db_t *db = NULL;
        dns_dbiterator_t *iter = NULL;
@@ -137,7 +145,7 @@ test_reverse(const char *filename) {
        result = dns_test_loaddb(&db, dns_dbtype_zone, TEST_ORIGIN, filename);
        assert_int_equal(result, ISC_R_SUCCESS);
 
-       result = dns_db_createiterator(db, 0, &iter);
+       result = dns_db_createiterator(db, flags, &iter);
        assert_int_equal(result, ISC_R_SUCCESS);
 
        for (result = dns_dbiterator_last(iter); result == ISC_R_SUCCESS;
@@ -152,7 +160,7 @@ test_reverse(const char *filename) {
                i++;
        }
 
-       assert_int_equal(i, 12);
+       assert_int_equal(i, nodes);
 
        dns_dbiterator_destroy(&iter);
        dns_db_detach(&db);
@@ -161,18 +169,26 @@ test_reverse(const char *filename) {
 ISC_RUN_TEST_IMPL(reverse) {
        UNUSED(state);
 
-       test_reverse(TESTS_DIR "/testdata/dbiterator/zone1.data");
+       test_reverse(TESTS_DIR "/testdata/dbiterator/zone1.data", 0, 12);
+       test_reverse(TESTS_DIR "/testdata/dbiterator/zone1.data",
+                    DNS_DB_NONSEC3, 12);
+       test_reverse(TESTS_DIR "/testdata/dbiterator/zone1.data",
+                    DNS_DB_NSEC3ONLY, 0);
 }
 
 ISC_RUN_TEST_IMPL(reverse_nsec3) {
        UNUSED(state);
 
-       test_reverse(TESTS_DIR "/testdata/dbiterator/zone2.data");
+       test_reverse(TESTS_DIR "/testdata/dbiterator/zone2.data", 0, 32);
+       test_reverse(TESTS_DIR "/testdata/dbiterator/zone2.data",
+                    DNS_DB_NONSEC3, 12);
+       test_reverse(TESTS_DIR "/testdata/dbiterator/zone2.data",
+                    DNS_DB_NSEC3ONLY, 20);
 }
 
 /* seek: walk database starting at a particular node */
 static void
-test_seek_node(const char *filename, int nodes) {
+test_seek_node(const char *filename, int flags, int nodes) {
        isc_result_t result;
        dns_db_t *db = NULL;
        dns_dbiterator_t *iter = NULL;
@@ -187,14 +203,19 @@ test_seek_node(const char *filename, int nodes) {
        result = dns_test_loaddb(&db, dns_dbtype_zone, TEST_ORIGIN, filename);
        assert_int_equal(result, ISC_R_SUCCESS);
 
-       result = dns_db_createiterator(db, 0, &iter);
+       result = dns_db_createiterator(db, flags, &iter);
        assert_int_equal(result, ISC_R_SUCCESS);
 
        result = make_name("c." TEST_ORIGIN, seekname);
        assert_int_equal(result, ISC_R_SUCCESS);
 
        result = dns_dbiterator_seek(iter, seekname);
-       assert_int_equal(result, ISC_R_SUCCESS);
+       if (flags == DNS_DB_NSEC3ONLY) {
+               /* "c" isn't in the NSEC3 tree but the origin node is */
+               assert_int_equal(result, DNS_R_PARTIALMATCH);
+       } else {
+               assert_int_equal(result, ISC_R_SUCCESS);
+       }
 
        while (result == ISC_R_SUCCESS) {
                result = dns_dbiterator_current(iter, &node, name);
@@ -209,6 +230,31 @@ test_seek_node(const char *filename, int nodes) {
 
        assert_int_equal(i, nodes);
 
+       /* now reset the iterator and walk backwards */
+       i = 0;
+       result = dns_dbiterator_seek(iter, seekname);
+       if (flags == DNS_DB_NSEC3ONLY) {
+               /* "c" isn't in the NSEC3 tree but the origin node is */
+               assert_int_equal(result, DNS_R_PARTIALMATCH);
+               nodes = 0;
+       } else {
+               assert_int_equal(result, ISC_R_SUCCESS);
+               nodes = 4;
+       }
+
+       while (result == ISC_R_SUCCESS) {
+               result = dns_dbiterator_current(iter, &node, name);
+               if (result == DNS_R_NEWORIGIN) {
+                       result = ISC_R_SUCCESS;
+               }
+               assert_int_equal(result, ISC_R_SUCCESS);
+               dns_db_detachnode(db, &node);
+               result = dns_dbiterator_prev(iter);
+               i++;
+       }
+
+       assert_int_equal(i, nodes);
+
        dns_dbiterator_destroy(&iter);
        dns_db_detach(&db);
 }
@@ -216,13 +262,21 @@ test_seek_node(const char *filename, int nodes) {
 ISC_RUN_TEST_IMPL(seek_node) {
        UNUSED(state);
 
-       test_seek_node(TESTS_DIR "/testdata/dbiterator/zone1.data", 10);
+       test_seek_node(TESTS_DIR "/testdata/dbiterator/zone1.data", 0, 9);
+       test_seek_node(TESTS_DIR "/testdata/dbiterator/zone1.data",
+                      DNS_DB_NONSEC3, 9);
+       test_seek_node(TESTS_DIR "/testdata/dbiterator/zone1.data",
+                      DNS_DB_NSEC3ONLY, 0);
 }
 
 ISC_RUN_TEST_IMPL(seek_node_nsec3) {
        UNUSED(state);
 
-       test_seek_node(TESTS_DIR "/testdata/dbiterator/zone2.data", 30);
+       test_seek_node(TESTS_DIR "/testdata/dbiterator/zone2.data", 0, 29);
+       test_seek_node(TESTS_DIR "/testdata/dbiterator/zone2.data",
+                      DNS_DB_NONSEC3, 9);
+       test_seek_node(TESTS_DIR "/testdata/dbiterator/zone2.data",
+                      DNS_DB_NSEC3ONLY, 0);
 }
 
 /*