From 19f6a63184295dfed790c69851bbbdb37a0f38a0 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] --- OPTIONS | 38 ++++++------- OPTIONS.md | 2 + lib/dns/rbtdb.c | 140 +++++++++++++++++++++++++++--------------------- lib/isc/heap.c | 18 +++++++ 4 files changed, 116 insertions(+), 82 deletions(-) diff --git a/OPTIONS b/OPTIONS index 21afbec59d..138fad82e9 100644 --- a/OPTIONS +++ b/OPTIONS @@ -4,25 +4,21 @@ defined in configure. Some of these settings are: -Setting Description - Overwrite memory with tag values when allocating --DISC_MEM_DEFAULTFILL=1 or freeing it; this impairs performance but - makes debugging of memory problems easier. - Don't track memory allocations by file and line --DISC_MEM_TRACKLINES=0 number; this improves performance but makes - debugging more difficult. --DISC_FACILITY=LOG_LOCAL0 Change the default syslog facility for named --DNS_CLIENT_DROPPORT=0 Disable dropping queries from particular - well-known ports: --DCHECK_SIBLING=0 Don't check sibling glue in named-checkzone --DCHECK_LOCAL=0 Don't check out-of-zone addresses in - named-checkzone --DNS_RUN_PID_DIR=0 Create default PID files in ${localstatedir}/run - rather than ${localstatedir}/run/named/ - Increase the maximum number of configurable --DNS_RPZ_MAX_ZONES=64 response policy zones from 32 to 64; this is the - highest possible setting - Disable the use of inline functions to implement --DISC_BUFFER_USEINLINE=0 the isc_buffer API: this reduces performance but - may be useful when debugging +|Setting |Description | |-----------------------------------| +----------------------------------------| |-DISC_MEM_DEFAULTFILL=1| +Overwrite memory with tag values when allocating or freeing it; this +impairs performance but makes debugging of memory problems easier.| | +-DISC_MEM_TRACKLINES=0|Don't track memory allocations by file and line +number; this improves performance but makes debugging more difficult.| | +-DISC_FACILITY=LOG_LOCAL0|Change the default syslog facility for named| | +-DNS_CLIENT_DROPPORT=0|Disable dropping queries from particular well-known +ports:| |-DCHECK_SIBLING=0|Don't check sibling glue in named-checkzone| | +-DCHECK_LOCAL=0|Don't check out-of-zone addresses in named-checkzone| | +-DNS_RUN_PID_DIR=0|Create default PID files in ${localstatedir}/run rather +than ${localstatedir}/run/named/| |-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_BUFFER_USEINLINE=0|Disable the use +of inline functions to implement the isc_buffer API: this reduces +performance but may be useful when debugging | |-DISC_HEAP_CHECK|Test heap +consistency after every heap operation; used when debugging | diff --git a/OPTIONS.md b/OPTIONS.md index 0dda7dea85..aeeea219f7 100644 --- 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/`| |`-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_BUFFER_USEINLINE=0`|Disable the use of inline functions to implement the `isc_buffer` API: this reduces performance but may be useful when debugging | +|`-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 e1014cc4e9..73f6604a46 100644 --- a/lib/dns/rbtdb.c +++ b/lib/dns/rbtdb.c @@ -6100,6 +6100,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, @@ -6438,41 +6454,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)) @@ -6482,13 +6465,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)) { @@ -6517,6 +6536,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; @@ -6530,6 +6554,14 @@ add32(dns_rbtdb_t *rbtdb, dns_rbtnode_t *rbtnode, rbtdb_version_t *rbtversion, mark_header_ancient(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 { /* @@ -6599,16 +6631,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); } /* @@ -7077,12 +7105,7 @@ subtractrdataset(dns_db_t *db, dns_dbnode_t *node, dns_dbversion_t *version, * to additional info. We need to clear these fields * to avoid having duplicated references. */ - 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; @@ -7117,12 +7140,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 @@ -8120,14 +8138,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 8cb3bbe9c1..1a5db9df0e 100644 --- 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);