From 063500972f4fde34d9842e9077fd6979532fef76 Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Mon, 27 Nov 2017 15:15:41 +1100 Subject: [PATCH] More: 4819. [bug] Fully backout the transaction when adding a RRset to the resigning / removal heaps fails. [RT #46473] (cherry picked from commit 19f6a63184295dfed790c69851bbbdb37a0f38a0) --- OPTIONS.md | 2 + lib/dns/rbtdb.c | 140 +++++++++++++++++++++++++++--------------------- lib/isc/heap.c | 18 +++++++ 3 files changed, 99 insertions(+), 61 deletions(-) diff --git a/OPTIONS.md b/OPTIONS.md index d69fdfab11..32f648ec56 100644 --- a/OPTIONS.md +++ b/OPTIONS.md @@ -30,3 +30,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 5d9d54f112..b94a4b44a9 100644 --- a/lib/dns/rbtdb.c +++ b/lib/dns/rbtdb.c @@ -6016,6 +6016,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, @@ -6346,41 +6362,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)) @@ -6390,13 +6373,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)) { @@ -6425,6 +6444,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; @@ -6438,6 +6462,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 { /* @@ -6507,16 +6539,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); } /* @@ -6986,12 +7014,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; @@ -7028,12 +7051,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 @@ -8077,14 +8095,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 3dd23cf56b..a9176fc079 100644 --- a/lib/isc/heap.c +++ b/lib/isc/heap.c @@ -72,6 +72,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, @@ -158,6 +170,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 @@ -183,6 +196,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 @@ -191,6 +205,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)) @@ -210,9 +225,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; @@ -248,6 +265,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);