Fix strict weak ordering violation in resign_sooner()

resign_sooner_values() only checked whether rhs was SOA-typed when
resign times were equal, but did not check lhs. When both entries were
SOA-typed with equal resign times, the comparison returned true in both
directions, violating irreflexivity and corrupting heap invariants.

Add lhs_typepair parameter and require lhs to be non-SOA for the
tie-breaking logic to apply.

diff --git a/lib/dns/qpzone.c b/lib/dns/qpzone.c
index d7604affbab717ff3dad6832c014c76492ca7429..47c274889a6d7f16ca5bb8ad2ba21ce52e952708 100644 (file)
--- a/lib/dns/qpzone.c
+++ b/lib/dns/qpzone.c
@@ -637,13 +637,16 @@ qpdb_destroy(dns_db_t *arg) {
 /*%
  * Compare resign times with SOA priority logic.
  * Returns true if lhs should be resigned sooner than rhs.
+ * When resign times are equal, non-SOA RRsets sort before the SOA RRset.
  */
 static bool
-resign_sooner_values(int64_t lhs_resign, int64_t rhs_resign,
-                    dns_typepair_t rhs_typepair) {
+resign_sooner_values(int64_t lhs_resign, dns_typepair_t lhs_typepair,
+                    int64_t rhs_resign, dns_typepair_t rhs_typepair) {
+       bool lhs_is_soa = lhs_typepair == DNS_SIGTYPEPAIR(dns_rdatatype_soa);
+       bool rhs_is_soa = rhs_typepair == DNS_SIGTYPEPAIR(dns_rdatatype_soa);
+
        return lhs_resign < rhs_resign ||
-              (lhs_resign == rhs_resign &&
-               rhs_typepair == DNS_SIGTYPEPAIR(dns_rdatatype_soa));
+              (lhs_resign == rhs_resign && lhs_is_soa < rhs_is_soa);
 }
 
 /*%
@@ -655,9 +658,9 @@ resign_sooner(void *v1, void *v2) {
        qpz_resign_t *elem1 = v1;
        qpz_resign_t *elem2 = v2;
 
-       return resign_sooner_values(elem1->header->resign,
-                                   elem2->header->resign,
-                                   elem2->header->typepair);
+       return resign_sooner_values(
+               elem1->header->resign, elem1->header->typepair,
+               elem2->header->resign, elem2->header->typepair);
 }
 
 /*%
@@ -1993,6 +1996,7 @@ add(qpzonedb_t *qpdb, qpznode_t *node, const dns_name_t *nodename,
                                if (loading && RESIGN(newheader) &&
                                    RESIGN(header) &&
                                    resign_sooner_values(header->resign,
+                                                        header->typepair,
                                                         newheader->resign,
                                                         newheader->typepair))
                                {
@@ -2482,13 +2486,14 @@ setsigningtime(dns_db_t *db, dns_dbnode_t *dbnode, dns_rdataset_t *rdataset,
 
                        header->resign = new_resign;
 
-                       if (resign_sooner_values(new_resign, old_resign,
-                                                header->typepair))
+                       if (resign_sooner_values(new_resign, header->typepair,
+                                                old_resign, header->typepair))
                        {
                                isc_heap_increased(qpdb->heap->heap,
                                                   found_elem->heap_index);
-                       } else if (resign_sooner_values(old_resign, new_resign,
-                                                       header->typepair))
+                       } else if (resign_sooner_values(
+                                          old_resign, header->typepair,
+                                          new_resign, header->typepair))
                        {
                                isc_heap_decreased(qpdb->heap->heap,
                                                   found_elem->heap_index);

diff --git a/tests/dns/qpzone_test.c b/tests/dns/qpzone_test.c
index ebb662bc4259d3d5a3fd9d7d9e3493cf65f4ae10..066f5921a5372ee82910fad79c9eefc652464038 100644 (file)
--- a/tests/dns/qpzone_test.c
+++ b/tests/dns/qpzone_test.c
@@ -387,6 +387,22 @@ ISC_RUN_TEST_IMPL(setownercase) {
        assert_true(dns_name_caseequal(name1, name2));
 }
 
+ISC_RUN_TEST_IMPL(resign_sooner_values) {
+       dns_typepair_t soa = DNS_SIGTYPEPAIR(dns_rdatatype_soa);
+       dns_typepair_t other = DNS_SIGTYPEPAIR(dns_rdatatype_a);
+
+       UNUSED(state);
+
+       assert_true(resign_sooner_values(10, other, 20, other));
+       assert_false(resign_sooner_values(20, other, 10, other));
+
+       assert_true(resign_sooner_values(10, other, 10, soa));
+       assert_false(resign_sooner_values(10, soa, 10, other));
+
+       assert_false(resign_sooner_values(10, soa, 10, soa));
+       assert_false(resign_sooner_values(10, other, 10, other));
+}
+
 ISC_RUN_TEST_IMPL(diffop_add_sub) {
        isc_result_t result;
        dns_db_t *db = NULL;
@@ -503,6 +519,7 @@ ISC_RUN_TEST_IMPL(diffop_addresign) {
 ISC_TEST_LIST_START
 ISC_TEST_ENTRY(ownercase)
 ISC_TEST_ENTRY(setownercase)
+ISC_TEST_ENTRY(resign_sooner_values)
 ISC_TEST_ENTRY(diffop_add_sub)
 ISC_TEST_ENTRY(diffop_addresign)
 ISC_TEST_LIST_END