More:

4819.   [bug]           Fully backout the transaction when adding a RRset
                        to the resigning / removal heaps fails. [RT #46473]

(cherry picked from commit 19f6a63184295dfed790c69851bbbdb37a0f38a0)

diff --git a/OPTIONS.md b/OPTIONS.md
index a2e795306bc0448300e98c7c06e2003145ca9aad..09cf4c5c0299deaa8bfef61b57d4c03a18a4d27a 100644 (file)
--- a/OPTIONS.md
+++ b/OPTIONS.md
@@ -22,3 +22,5 @@ Some of these settings are:
 |`-DNS_RUN_PID_DIR=0`|Create default PID files in `${localstatedir}/run` rather than `${localstatedir}/run/{named,lwresd}/`|
 |`-DDIG_SIGCHASE=1`|Enable DNSSEC signature chasing support in `dig`.  (Note: This feature is deprecated. Use `delv` instead.)|
 |`-DNS_RPZ_MAX_ZONES=64`|Increase the maximum number of configurable response policy zones from 32 to 64; this is the highest possible setting|
+|`-DISC_HEAP_CHECK`|Test heap consistency after every heap operation; used
+when debugging |

diff --git a/lib/dns/rbtdb.c b/lib/dns/rbtdb.c
index d1d8049ad8a137fc9821c5a74213053b25195548..5fca51106bde606585c488d69bb16520ca72ca48 100644 (file)
--- a/lib/dns/rbtdb.c
+++ b/lib/dns/rbtdb.c
@@ -6062,6 +6062,22 @@ resign_delete(dns_rbtdb_t *rbtdb, rbtdb_version_t *version,
        }
 }
 
+static void
+update_recordsandbytes(isc_boolean_t add, rbtdb_version_t *rbtversion,
+                      rdatasetheader_t *header)
+{
+       unsigned char *hdr = (unsigned char *)header;
+       size_t hdrsize = sizeof (*header);
+
+       if (add) {
+               rbtversion->records += dns_rdataslab_count(hdr, hdrsize);
+               rbtversion->bytes += dns_rdataslab_size(hdr, hdrsize);
+       } else {
+               rbtversion->records -= dns_rdataslab_count(hdr, hdrsize);
+               rbtversion->bytes -= dns_rdataslab_size(hdr, hdrsize);
+       }
+}
+
 static isc_result_t
 add32(dns_rbtdb_t *rbtdb, dns_rbtnode_t *rbtnode, rbtdb_version_t *rbtversion,
       rdatasetheader_t *newheader, unsigned int options, isc_boolean_t loading,
@@ -6392,41 +6408,8 @@ add32(dns_rbtdb_t *rbtdb, dns_rbtnode_t *rbtnode, rbtdb_version_t *rbtversion,
                }
                INSIST(rbtversion == NULL ||
                       rbtversion->serial >= topheader->serial);
-               if (topheader_prev != NULL)
-                       topheader_prev->next = newheader;
-               else
-                       rbtnode->data = newheader;
-               newheader->next = topheader->next;
-               if (rbtversion != NULL)
-                       RWLOCK(&rbtversion->rwlock, isc_rwlocktype_write);
-               if (rbtversion != NULL && !header_nx) {
-                       rbtversion->records -=
-                               dns_rdataslab_count((unsigned char *)header,
-                                                   sizeof(*header));
-                       rbtversion->bytes -=
-                               dns_rdataslab_size((unsigned char *)header,
-                                                  sizeof(*header));
-               }
-               if (rbtversion != NULL && !newheader_nx) {
-                       rbtversion->records +=
-                               dns_rdataslab_count((unsigned char *)newheader,
-                                                   sizeof(*newheader));
-                       rbtversion->bytes +=
-                               dns_rdataslab_size((unsigned char *)newheader,
-                                                  sizeof(*newheader));
-               }
-               if (rbtversion != NULL)
-                       RWUNLOCK(&rbtversion->rwlock, isc_rwlocktype_write);
                if (loading) {
-                       /*
-                        * There are no other references to 'header' when
-                        * loading, so we MAY clean up 'header' now.
-                        * Since we don't generate changed records when
-                        * loading, we MUST clean up 'header' now.
-                        */
                        newheader->down = NULL;
-                       free_rdataset(rbtdb, rbtdb->common.mctx, header);
-
                        idx = newheader->node->locknum;
                        if (IS_CACHE(rbtdb)) {
                                if (ZEROTTL(newheader))
@@ -6436,13 +6419,49 @@ add32(dns_rbtdb_t *rbtdb, dns_rbtnode_t *rbtnode, rbtdb_version_t *rbtversion,
                                        ISC_LIST_PREPEND(rbtdb->rdatasets[idx],
                                                         newheader, link);
                                INSIST(rbtdb->heaps != NULL);
-                               (void)isc_heap_insert(rbtdb->heaps[idx],
+                               result = isc_heap_insert(rbtdb->heaps[idx],
+                                                        newheader);
+                               if (result != ISC_R_SUCCESS) {
+                                       free_rdataset(rbtdb,
+                                                     rbtdb->common.mctx,
                                                      newheader);
+                                       return (result);
+                               }
                        } else if (RESIGN(newheader)) {
                                result = resign_insert(rbtdb, idx, newheader);
-                               if (result != ISC_R_SUCCESS)
+                               if (result != ISC_R_SUCCESS) {
+                                       free_rdataset(rbtdb,
+                                                     rbtdb->common.mctx,
+                                                     newheader);
                                        return (result);
+                               }
+                               /*
+                                * Don't call resign_delete as we don't need
+                                * to reverse the delete.  The free_rdataset
+                                * call below will clean up the heap entry.
+                                */
+                       }
+
+                       /*
+                        * There are no other references to 'header' when
+                        * loading, so we MAY clean up 'header' now.
+                        * Since we don't generate changed records when
+                        * loading, we MUST clean up 'header' now.
+                        */
+                       if (topheader_prev != NULL)
+                               topheader_prev->next = newheader;
+                       else
+                               rbtnode->data = newheader;
+                       newheader->next = topheader->next;
+                       if (rbtversion != NULL && !header_nx) {
+                               RWLOCK(&rbtversion->rwlock,
+                                      isc_rwlocktype_write);
+                               update_recordsandbytes(ISC_FALSE, rbtversion,
+                                                      header);
+                               RWUNLOCK(&rbtversion->rwlock,
+                                        isc_rwlocktype_write);
                        }
+                       free_rdataset(rbtdb, rbtdb->common.mctx, header);
                } else {
                        idx = newheader->node->locknum;
                        if (IS_CACHE(rbtdb)) {
@@ -6471,6 +6490,11 @@ add32(dns_rbtdb_t *rbtdb, dns_rbtnode_t *rbtnode, rbtdb_version_t *rbtversion,
                                }
                                resign_delete(rbtdb, rbtversion, header);
                        }
+                       if (topheader_prev != NULL)
+                               topheader_prev->next = newheader;
+                       else
+                               rbtnode->data = newheader;
+                       newheader->next = topheader->next;
                        newheader->down = topheader;
                        topheader->next = newheader;
                        rbtnode->dirty = 1;
@@ -6484,6 +6508,14 @@ add32(dns_rbtdb_t *rbtdb, dns_rbtnode_t *rbtnode, rbtdb_version_t *rbtversion,
                                        mark_stale_header(rbtdb, sigheader);
                                }
                        }
+                       if (rbtversion != NULL && !header_nx) {
+                               RWLOCK(&rbtversion->rwlock,
+                                      isc_rwlocktype_write);
+                               update_recordsandbytes(ISC_FALSE, rbtversion,
+                                                      header);
+                               RWUNLOCK(&rbtversion->rwlock,
+                                        isc_rwlocktype_write);
+                       }
                }
        } else {
                /*
@@ -6553,16 +6585,12 @@ add32(dns_rbtdb_t *rbtdb, dns_rbtnode_t *rbtnode, rbtdb_version_t *rbtversion,
                        newheader->down = NULL;
                        rbtnode->data = newheader;
                }
-               if (rbtversion != NULL && !newheader_nx) {
-                       RWLOCK(&rbtversion->rwlock, isc_rwlocktype_write);
-                       rbtversion->records +=
-                               dns_rdataslab_count((unsigned char *)newheader,
-                                                   sizeof(*newheader));
-                       rbtversion->bytes +=
-                               dns_rdataslab_size((unsigned char *)newheader,
-                                                  sizeof(*newheader));
-                       RWUNLOCK(&rbtversion->rwlock, isc_rwlocktype_write);
-               }
+       }
+
+       if (rbtversion != NULL && !newheader_nx) {
+               RWLOCK(&rbtversion->rwlock, isc_rwlocktype_write);
+               update_recordsandbytes(ISC_TRUE, rbtversion, newheader);
+               RWUNLOCK(&rbtversion->rwlock, isc_rwlocktype_write);
        }
 
        /*
@@ -7037,12 +7065,7 @@ subtractrdataset(dns_db_t *db, dns_dbnode_t *node, dns_dbversion_t *version,
                         */
                        newheader->additional_auth = NULL;
                        newheader->additional_glue = NULL;
-                       rbtversion->records +=
-                               dns_rdataslab_count((unsigned char *)newheader,
-                                                   sizeof(*newheader));
-                       rbtversion->bytes +=
-                               dns_rdataslab_size((unsigned char *)newheader,
-                                                  sizeof(*newheader));
+                       update_recordsandbytes(ISC_TRUE, rbtversion, newheader);
                } else if (result == DNS_R_NXRRSET) {
                        /*
                         * This subtraction would remove all of the rdata;
@@ -7079,12 +7102,7 @@ subtractrdataset(dns_db_t *db, dns_dbnode_t *node, dns_dbversion_t *version,
                 * topheader.
                 */
                INSIST(rbtversion->serial >= topheader->serial);
-               rbtversion->records -=
-                               dns_rdataslab_count((unsigned char *)header,
-                                                   sizeof(*header));
-               rbtversion->bytes -=
-                               dns_rdataslab_size((unsigned char *)header,
-                                                  sizeof(*header));
+               update_recordsandbytes(ISC_FALSE, rbtversion, header);
                if (topheader_prev != NULL)
                        topheader_prev->next = newheader;
                else
@@ -8134,14 +8152,14 @@ setsigningtime(dns_db_t *db, dns_rdataset_t *rdataset, isc_stdtime_t resign) {
                if (resign == 0) {
                        isc_heap_delete(rbtdb->heaps[header->node->locknum],
                                        header->heap_index);
-                       header->heap_index = 0;
-               } else if (resign_sooner(header, &oldheader))
+               } else if (resign_sooner(header, &oldheader)) {
                        isc_heap_increased(rbtdb->heaps[header->node->locknum],
                                           header->heap_index);
-               else if (resign_sooner(&oldheader, header))
+               } else if (resign_sooner(&oldheader, header)) {
                        isc_heap_decreased(rbtdb->heaps[header->node->locknum],
                                           header->heap_index);
-       } else if (resign != 0 && header->heap_index == 0) {
+               }
+       } else if (resign != 0) {
                header->attributes |= RDATASET_ATTR_RESIGN;
                result = resign_insert(rbtdb, header->node->locknum, header);
        }

diff --git a/lib/isc/heap.c b/lib/isc/heap.c
index 8cb3bbe9c16a9add2b7c1769bacbb8028fa9a3ff..1a5db9df0e2c921bf0d75c3d2cb7b48aff187087 100644 (file)
--- a/lib/isc/heap.c
+++ b/lib/isc/heap.c
@@ -63,6 +63,18 @@ struct isc_heap {
        isc_heapindex_t                 index;
 };
 
+#ifdef ISC_HEAP_CHECK
+static void
+heap_check(isc_heap_t *heap) {
+       unsigned int i;
+       for (i = 1; i <= heap->last; i++) {
+               INSIST(HEAPCONDITION(i));
+       }
+}
+#else
+#define heap_check(x) (void)0
+#endif
+
 isc_result_t
 isc_heap_create(isc_mem_t *mctx, isc_heapcompare_t compare,
                isc_heapindex_t idx, unsigned int size_increment,
@@ -149,6 +161,7 @@ float_up(isc_heap_t *heap, unsigned int i, void *elt) {
                (heap->index)(heap->array[i], i);
 
        INSIST(HEAPCONDITION(i));
+       heap_check(heap);
 }
 
 static void
@@ -174,6 +187,7 @@ sink_down(isc_heap_t *heap, unsigned int i, void *elt) {
                (heap->index)(heap->array[i], i);
 
        INSIST(HEAPCONDITION(i));
+       heap_check(heap);
 }
 
 isc_result_t
@@ -182,6 +196,7 @@ isc_heap_insert(isc_heap_t *heap, void *elt) {
 
        REQUIRE(VALID_HEAP(heap));
 
+       heap_check(heap);
        new_last = heap->last + 1;
        RUNTIME_CHECK(new_last > 0); /* overflow check */
        if (new_last >= heap->size && !resize(heap))
@@ -201,9 +216,11 @@ isc_heap_delete(isc_heap_t *heap, unsigned int idx) {
        REQUIRE(VALID_HEAP(heap));
        REQUIRE(idx >= 1 && idx <= heap->last);
 
+       heap_check(heap);
        if (idx == heap->last) {
                heap->array[heap->last] = NULL;
                heap->last--;
+               heap_check(heap);
        } else {
                elt = heap->array[heap->last];
                heap->array[heap->last] = NULL;
@@ -239,6 +256,7 @@ isc_heap_element(isc_heap_t *heap, unsigned int idx) {
        REQUIRE(VALID_HEAP(heap));
        REQUIRE(idx >= 1);
 
+       heap_check(heap);
        if (idx <= heap->last)
                return (heap->array[idx]);
        return (NULL);