From 24eaff7adc30c3cde22c5926369c3729ad12ae15 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Fri, 14 Feb 2025 21:42:34 -0800 Subject: [PATCH 1/3] qpzone.c:step() could ignore rollbacks the step() function (used for stepping to the prececessor or successor of a database node) could overlook a node because there was an rdataset marked IGNORE because it had been rolled back, covering an active rdataset under it. --- lib/dns/qpzone.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/lib/dns/qpzone.c b/lib/dns/qpzone.c index 58039bd52f..d4570d689a 100644 --- a/lib/dns/qpzone.c +++ b/lib/dns/qpzone.c @@ -2696,13 +2696,25 @@ step(qpz_search_t *search, dns_qpiter_t *it, direction_t direction, while (result == ISC_R_SUCCESS) { isc_rwlock_t *nlock = &qpdb->buckets[node->locknum].lock; isc_rwlocktype_t nlocktype = isc_rwlocktype_none; + dns_slabheader_t *header_next = NULL; NODE_RDLOCK(nlock, &nlocktype); - for (header = node->data; header != NULL; header = header->next) + for (header = node->data; header != NULL; header = header_next) { - if (header->serial <= search->serial && - !IGNORE(header) && !NONEXISTENT(header)) - { + header_next = header->next; + while (header != NULL) { + if (header->serial <= search->serial && + !IGNORE(header)) + { + if (NONEXISTENT(header)) { + header = NULL; + } + break; + } else { + header = header->down; + } + } + if (header != NULL) { break; } } From 7d98aba3ac9189b88d54ac0a690e625d27950e1a Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Tue, 4 Mar 2025 20:21:04 -0800 Subject: [PATCH 2/3] add a unit test to check database rollback check that a database rollback works and the correct (original) data is found on lookup. --- tests/dns/dbversion_test.c | 149 +++++++++++++++++++++++++++++++++++++ 1 file changed, 149 insertions(+) diff --git a/tests/dns/dbversion_test.c b/tests/dns/dbversion_test.c index 4ec3063f04..8cd5aa9219 100644 --- a/tests/dns/dbversion_test.c +++ b/tests/dns/dbversion_test.c @@ -363,6 +363,154 @@ ISC_RUN_TEST_IMPL(getnsec3parameters) { db1, v2, &hash, &flags, &iterations, salt, &salt_length)); } +/* + * Check that the correct node contents are found after a rollback. + */ +ISC_RUN_TEST_IMPL(rollback) { + isc_result_t res; + dns_rdata_t rdata1 = DNS_RDATA_INIT, rdata2 = DNS_RDATA_INIT; + dns_rdataset_t input1 = DNS_RDATASET_INIT; + dns_rdataset_t input2 = DNS_RDATASET_INIT; + dns_rdataset_t rdataset1 = DNS_RDATASET_INIT; + dns_rdataset_t rdataset2 = DNS_RDATASET_INIT; + dns_rdatalist_t rdatalist1, rdatalist2; + dns_rdata_t out1 = DNS_RDATA_INIT, out2 = DNS_RDATA_INIT; + dns_dbnode_t *node = NULL; + char *txt1 = (char *)"\006text 1"; + char *txt2 = (char *)"\006text 2"; + size_t len1 = strlen(txt1), len2 = strlen(txt2); + char buf[1024]; + isc_buffer_t b; + + UNUSED(state); + + isc_buffer_init(&b, buf, sizeof(buf)); + + /* Set up two rdatasets to insert */ + rdata1.rdclass = dns_rdataclass_in; + rdata1.type = dns_rdatatype_txt; + rdata2 = rdata1; + + rdata1.length = len1; + rdata1.data = (unsigned char *)txt1; + rdata2.length = len2; + rdata2.data = (unsigned char *)txt2; + + dns_rdatalist_init(&rdatalist1); + rdatalist1.rdclass = dns_rdataclass_in; + rdatalist1.type = dns_rdatatype_txt; + rdatalist1.ttl = 3600; + rdatalist2 = rdatalist1; + + ISC_LIST_APPEND(rdatalist1.rdata, &rdata1, link); + ISC_LIST_APPEND(rdatalist2.rdata, &rdata2, link); + + dns_rdatalist_tordataset(&rdatalist1, &input1); + dns_rdatalist_tordataset(&rdatalist2, &input2); + + /* db1: Insert the first version ("text 1"), and commit */ + res = dns_db_findnode(db1, dns_rootname, true, &node); + assert_int_equal(res, ISC_R_SUCCESS); + res = dns_db_addrdataset(db1, node, v1, 0, &input1, 0, NULL); + assert_int_equal(res, ISC_R_SUCCESS); + dns_db_closeversion(db1, &v1, true); /* commit */ + assert_null(v1); + dns_db_detachnode(db1, &node); + assert_null(node); + + /* db2: Insert the first version ("text 1"), and commit */ + res = dns_db_findnode(db2, dns_rootname, true, &node); + assert_int_equal(res, ISC_R_SUCCESS); + res = dns_db_addrdataset(db2, node, v2, 0, &input1, 0, NULL); + assert_int_equal(res, ISC_R_SUCCESS); + dns_db_closeversion(db2, &v2, true); /* commit */ + assert_null(v2); + dns_db_detachnode(db2, &node); + assert_null(node); + + /* Reopen the versions */ + dns_db_newversion(db1, &v1); + assert_non_null(v1); + dns_db_newversion(db2, &v2); + assert_non_null(v2); + + /* db1: Insert the second version ("text 2"), and roll back */ + res = dns_db_findnode(db1, dns_rootname, true, &node); + assert_int_equal(res, ISC_R_SUCCESS); + res = dns_db_addrdataset(db1, node, v1, 0, &input2, 0, NULL); + assert_int_equal(res, ISC_R_SUCCESS); + dns_db_closeversion(db1, &v1, false); /* rollback */ + assert_null(v1); + dns_db_detachnode(db1, &node); + assert_null(node); + + /* db2: Insert the second version ("text 2"), and commit */ + res = dns_db_findnode(db2, dns_rootname, true, &node); + assert_int_equal(res, ISC_R_SUCCESS); + res = dns_db_addrdataset(db2, node, v2, 0, &input2, 0, NULL); + assert_int_equal(res, ISC_R_SUCCESS); + dns_db_closeversion(db2, &v2, true); /* commit */ + assert_null(v2); + dns_db_detachnode(db2, &node); + assert_null(node); + + /* db1: Look it up and check that the first version is found */ + dns_db_currentversion(db1, &v1); + assert_non_null(v1); + res = dns_db_findnode(db1, dns_rootname, true, &node); + assert_int_equal(res, ISC_R_SUCCESS); + res = dns_db_findrdataset(db1, node, v1, dns_rdatatype_txt, 0, 0, + &rdataset1, NULL); + assert_int_equal(res, ISC_R_SUCCESS); + + /* db1: Convert result to text */ + res = dns_rdataset_first(&rdataset1); + assert_int_equal(res, ISC_R_SUCCESS); + dns_rdataset_current(&rdataset1, &out1); + + res = dns_rdata_totext(&out1, NULL, &b); + assert_int_equal(res, ISC_R_SUCCESS); + isc_buffer_putuint8(&b, 0); + + /* db1: We should have "text 1" */ + assert_string_equal(buf, "\"text 1\""); + + dns_rdataset_disassociate(&rdataset1); + + dns_db_closeversion(db1, &v1, true); + assert_null(v1); + dns_db_detachnode(db1, &node); + assert_null(node); + + /* db2: Look it up and check that the second version is found */ + dns_db_currentversion(db2, &v2); + assert_non_null(v2); + res = dns_db_findnode(db2, dns_rootname, true, &node); + assert_int_equal(res, ISC_R_SUCCESS); + res = dns_db_findrdataset(db2, node, v2, dns_rdatatype_txt, 0, 0, + &rdataset2, NULL); + assert_int_equal(res, ISC_R_SUCCESS); + + /* db2: Convert result to text */ + res = dns_rdataset_first(&rdataset2); + assert_int_equal(res, ISC_R_SUCCESS); + dns_rdataset_current(&rdataset2, &out2); + isc_buffer_init(&b, buf, sizeof(buf)); + res = dns_rdata_totext(&out2, NULL, &b); + assert_int_equal(res, ISC_R_SUCCESS); + isc_buffer_putuint8(&b, 0); + + /* db2: We should have "text 2" */ + assert_string_equal(buf, "\"text 2\""); + + dns_rdataset_disassociate(&rdataset2); + + dns_db_closeversion(db2, &v2, true); + assert_null(v2); + dns_db_detachnode(db2, &node); + assert_null(node); +} + ISC_TEST_LIST_START ISC_TEST_ENTRY_CUSTOM(find, setup_test, teardown_test) ISC_TEST_ENTRY_CUSTOM(allrdatasets, setup_test, teardown_test) @@ -373,6 +521,7 @@ ISC_TEST_ENTRY_CUSTOM(addrdataset, setup_test, teardown_test) ISC_TEST_ENTRY_CUSTOM(getnsec3parameters, setup_test, teardown_test) ISC_TEST_ENTRY_CUSTOM(attachversion, setup_test, teardown_test) ISC_TEST_ENTRY_CUSTOM(closeversion, setup_test, teardown_test) +ISC_TEST_ENTRY_CUSTOM(rollback, setup_test, teardown_test) ISC_TEST_LIST_END ISC_TEST_MAIN From ecde0ea2d719153c84fca19eaeeeeb6a01c10c1a Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Wed, 5 Mar 2025 15:46:17 -0800 Subject: [PATCH 3/3] add a unit test with an empty node the db_test unit test now looks up an empty nonterminal node to exercise the behavior of the step() function in qpzone. --- tests/dns/db_test.c | 34 +++++++++++++++++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-) diff --git a/tests/dns/db_test.c b/tests/dns/db_test.c index d9b7a1047a..7fa128b801 100644 --- a/tests/dns/db_test.c +++ b/tests/dns/db_test.c @@ -351,8 +351,40 @@ ISC_LOOP_TEST_IMPL(version) { result = dns_db_find(db, name, ver, dns_rdatatype_a, 0, 0, &node, foundname, &rdataset, NULL); assert_int_equal(result, ISC_R_SUCCESS); - dns_rdataset_disassociate(&rdataset); dns_db_detachnode(db, &node); + + /* Now we create a node with an empty parent */ + result = dns_db_newversion(db, &new); + dns_test_namefromstring("long.ent.name.test.test.", &fname); + result = dns_db_findnode(db, name, true, &node); + assert_int_equal(result, ISC_R_SUCCESS); + result = dns_db_addrdataset(db, node, new, 0, &rdataset, 0, NULL); + assert_int_equal(result, ISC_R_SUCCESS); + dns_rdataset_disassociate(&rdataset); + dns_rdataset_init(&rdataset); + + /* look up the ENT; it should be empty */ + dns_test_namefromstring("ent.name.test.test.", &fname); + dns_db_detachnode(db, &node); + result = dns_db_find(db, name, new, dns_rdatatype_a, 0, 0, &node, + foundname, &rdataset, NULL); + assert_int_equal(result, DNS_R_EMPTYNAME); + + /* ... but then we roll it back... */ + dns_db_closeversion(db, &new, false); + + /* ... and the ENT should be NXDOMAIN now */ + dns_test_namefromstring("ent.name.test.test.", &fname); + result = dns_db_find(db, name, ver, dns_rdatatype_a, 0, 0, &node, + foundname, &rdataset, NULL); + assert_int_equal(result, DNS_R_NXDOMAIN); + + if (dns_rdataset_isassociated(&rdataset)) { + dns_rdataset_disassociate(&rdataset); + } + if (node != NULL) { + dns_db_detachnode(db, &node); + } dns_db_closeversion(db, &ver, false); dns_db_detach(&db);