diff --git a/CHANGES b/CHANGES index 58c2a60b15..0bd113d400 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,6 @@ +6346. [bug] Cleaned up several minor bugs in the RBTDB dbiterator + implementation. [GL !8741] + 6345. [bug] Added missing dns_rdataset_disassociate calls in validator.c:findnsec3proofs. [GL #4571] diff --git a/lib/dns/db.c b/lib/dns/db.c index e7f5d64446..0624b0d264 100644 --- a/lib/dns/db.c +++ b/lib/dns/db.c @@ -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)); diff --git a/lib/dns/include/dns/db.h b/lib/dns/include/dns/db.h index c82c694c5a..b6362d99ee 100644 --- a/lib/dns/include/dns/db.h +++ b/lib/dns/include/dns/db.h @@ -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. diff --git a/lib/dns/rbtdb.c b/lib/dns/rbtdb.c index 58a0f46eeb..9da26c64ce 100644 --- a/lib/dns/rbtdb.c +++ b/lib/dns/rbtdb.c @@ -130,6 +130,10 @@ #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); - if (!rbtdbiter->nonsec3 && result == ISC_R_NOTFOUND) { + break; + case full: + rbtdbiter->current = &rbtdbiter->chain; + result = dns_rbtnodechain_first(rbtdbiter->current, rbtdb->tree, + name, origin); + 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); diff --git a/tests/dns/dbiterator_test.c b/tests/dns/dbiterator_test.c index 2c65a1d10e..43c0c79615 100644 --- a/tests/dns/dbiterator_test.c +++ b/tests/dns/dbiterator_test.c @@ -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); } /*