More:

4819. [bug] Fully backout the transaction when adding a RRset to the resigning / removal heaps fails. [RT #46473] (cherry picked from commit 19f6a63184)
2026-03-18 08:32:17 -04:00 · 2017-11-27 15:15:41 +11:00 · 2017-11-27 15:15:41 +11:00 · 063500972f
commit 063500972f
parent 72148d1505
3 changed files with 99 additions and 61 deletions
--- 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 |
--- 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);
 	}
--- 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);