fix: usr: Fix a bug in DNS UPDATE processing with inline-signing enabled

In rare cases the :iscman:`named` process could terminate unexpectedly
when processing authorized DNS UPDATE messages in quick procession
which are updating a zone with inline-signing enabled. This has been
fixed.

Closes #5816

Merge branch '5816-inline-signing-concurrent-update-fix' into 'main'

See merge request isc-projects/bind9!11982

bin/tests/system/inline/ns3/named.conf.j2

@@ -57,6 +57,15 @@ zone "bits" {
 	sig-signing-signatures 1;	// force incremental processing
 };
 
+zone "incremental-updates" {
+    type primary;
+    file "incremental-updates.db";
+    inline-signing yes;
+    dnssec-policy inline;
+    sig-signing-signatures 1;	// force incremental processing
+    allow-update { any; };
+};
+
 server 10.53.0.4 { request-ixfr no; };
 
 zone "noixfr" {

bin/tests/system/inline/setup.sh

@@ -31,6 +31,7 @@ cp ns3/primary.db.in ns3/nsec3.db
 cp ns3/primary.db.in ns3/externalkey.db
 cp ns3/primary.db.in ns3/delayedkeys.db
 cp ns3/primary.db.in ns3/removedkeys-primary.db
+cp ns3/primary.db.in ns3/incremental-updates.db
 cp ns3/include.db.in ns3/include.db
 
 mkdir ns3/removedkeys

bin/tests/system/inline/tests_inline_incremental_updates.py

@@ -0,0 +1,65 @@
+# Copyright (C) Internet Systems Consortium, Inc. ("ISC")
+#
+# SPDX-License-Identifier: MPL-2.0
+#
+# This Source Code Form is subject to the terms of the Mozilla Public
+# License, v. 2.0.  If a copy of the MPL was not distributed with this
+# file, you can obtain one at https://mozilla.org/MPL/2.0/.
+#
+# See the COPYRIGHT file distributed with this work for additional
+# information regarding copyright ownership.
+
+import random
+import threading
+
+import dns.update
+import pytest
+
+pytestmark = pytest.mark.extra_artifacts(
+    [
+        "K*",
+        "*.out*",
+        "*/*.out*",
+        "ns*/K*",
+        "ns*/dsset-*",
+        "ns*/*.bk",
+        "ns*/*.db",
+        "ns*/*.jbk",
+        "ns*/*.jnl",
+        "ns*/*.nzd",
+        "ns*/*.signed",
+        "ns*/trusted.conf",
+        "ns3/delayedkeys.conf",
+        "ns3/removedkeys",
+    ]
+)
+
+
+def worker(server, count) -> None:
+    zone = "incremental-updates"
+    for i in range(count):
+        try:
+            sub = random.randrange(1000000)
+            update_msg = dns.update.UpdateMessage(zone)
+            update_msg.add(f"a-{sub}-{i}.{zone}.", 300, "A", "10.0.0.1")
+            server.nsupdate(update_msg)
+        except Exception:  # pylint: disable=broad-exception-caught
+            break
+
+
+def test_inline_incremental_updates(ns3):
+    """
+    Flood the server with updates to check how 'receive secure serial'
+    is coping with quick incremental updates.
+    """
+    threads_n = 10
+    updates_n = 10
+    threads = [
+        threading.Thread(target=worker, args=(ns3, updates_n), daemon=True)
+        for _ in range(threads_n)
+    ]
+
+    for thread in threads:
+        thread.start()
+    for thread in threads:
+        thread.join()

lib/dns/include/dns/update.h

@@ -63,4 +63,7 @@ isc_result_t
 dns_update_signaturesinc(dns_update_log_t *log, dns_zone_t *zone, dns_db_t *db,
 			 dns_dbversion_t *oldver, dns_dbversion_t *newver,
 			 dns_diff_t *diff, uint32_t sigvalidityinterval,
-			 dns_update_state_t **state);
+			 dns_update_state_t **statep);
+
+void
+dns_update_state_clear(dns_update_state_t **statep, bool destroy);

lib/dns/update.c

@@ -1310,6 +1310,7 @@ dns_update_signatures(dns_update_log_t *log, dns_zone_t *zone, dns_db_t *db,
 
 struct dns_update_state {
 	unsigned int magic;
+	isc_mem_t *mctx;
 	dns_diff_t diffnames;
 	dns_diff_t affected;
 	dns_diff_t sig_diff;
@@ -1366,10 +1367,9 @@ dns_update_signaturesinc(dns_update_log_t *log, dns_zone_t *zone, dns_db_t *db,
 			 dns_diff_t *diff, uint32_t sigvalidityinterval,
 			 dns_update_state_t **statep) {
 	isc_result_t result = ISC_R_SUCCESS;
-	dns_update_state_t mystate, *state = NULL;
+	dns_update_state_t mystate = { 0 }, *state = NULL;
 	dns_difftuple_t *tuple = NULL;
 	bool flag, build_nsec;
-	unsigned int i;
 	dns_rdata_soa_t soa;
 	dns_rdata_t rdata = DNS_RDATA_INIT;
 	dns_rdataset_t rdataset;
@@ -1385,7 +1385,10 @@ dns_update_signaturesinc(dns_update_log_t *log, dns_zone_t *zone, dns_db_t *db,
 			state = &mystate;
 		} else {
 			state = isc_mem_get(diff->mctx, sizeof(*state));
+			state->mctx = NULL;
+			isc_mem_attach(diff->mctx, &state->mctx);
 		}
+		state->magic = STATE_MAGIC;
 
 		dns_diff_init(diff->mctx, &state->diffnames);
 		dns_diff_init(diff->mctx, &state->affected);
@@ -1444,7 +1447,6 @@ dns_update_signaturesinc(dns_update_log_t *log, dns_zone_t *zone, dns_db_t *db,
 		 */
 		CHECK(dns_diff_sort(diff, temp_order));
 		state->state = sign_updates;
-		state->magic = STATE_MAGIC;
 		SET_IF_NOT_NULL(statep, state);
 	} else {
 		REQUIRE(DNS_STATE_VALID(*statep));
@@ -2005,6 +2007,18 @@ cleanup:
 		dns_db_detachnode(&node);
 	}
 
+	dns_update_state_clear(&state, state != &mystate);
+	SET_IF_NOT_NULL(statep, NULL);
+
+	return result;
+}
+
+void
+dns_update_state_clear(dns_update_state_t **statep, bool destroy) {
+	REQUIRE(DNS_STATE_VALID(*statep));
+
+	dns_update_state_t *state = *statep;
+
 	dns_diff_clear(&state->sig_diff);
 	dns_diff_clear(&state->nsec_diff);
 	dns_diff_clear(&state->nsec_mindiff);
@@ -2013,17 +2027,15 @@ cleanup:
 	dns_diff_clear(&state->diffnames);
 	dns_diff_clear(&state->work);
 
-	for (i = 0; i < state->nkeys; i++) {
+	for (size_t i = 0; i < state->nkeys; i++) {
 		dst_key_free(&state->zone_keys[i]);
 	}
 
-	if (state != &mystate) {
+	if (destroy) {
 		*statep = NULL;
 		state->magic = 0;
-		isc_mem_put(diff->mctx, state, sizeof(*state));
+		isc_mem_putanddetach(&state->mctx, state, sizeof(*state));
 	}
-
-	return result;
 }
 
 static isc_stdtime_t

lib/dns/zone.c

@@ -593,6 +593,10 @@ dns__zone_free(dns_zone_t *zone) {
 		isc_mem_put(zone->mctx, include, sizeof *include);
 	}
 
+	if (zone->rss_state != NULL) {
+		dns_update_state_clear(&zone->rss_state, true);
+	}
+
 	if (zone->masterfile != NULL) {
 		isc_mem_free(zone->mctx, zone->masterfile);
 	}
@@ -13799,7 +13803,6 @@ struct rss {
 	dns_zone_t *zone;
 	dns_db_t *db;
 	uint32_t serial;
-	ISC_LINK(struct rss) link;
 };
 
 static void
@@ -14392,18 +14395,51 @@ receive_secure_serial(void *arg) {
 
 	LOCK_ZONE(zone);
 
-	/*
-	 * The receive_secure_serial() is loop-serialized for the zone.  Make
-	 * sure there's no processing currently running.
-	 */
+	if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_EXITING) ||
+	    !dns__zone_inline_secure(zone))
+	{
+		/*
+		 * If this is a callback for a new secure serial that was
+		 * never processed and the zone is shutting down, then just
+		 * free 'rss_next' and return.
+		 */
+		if (rss == zone->rss_next) {
+			isc_mem_put(zone->mctx, rss, sizeof(*rss));
+			zone->rss_next = NULL;
+			UNLOCK_ZONE(zone);
+			dns_zone_idetach(&zone);
+			return;
+		}
 
-	INSIST(zone->rss == NULL || zone->rss == rss);
-
-	if (zone->rss != NULL) {
-		INSIST(zone->rss == rss);
+		/* Otherwise, this is an ongoing processing, do the cleanup. */
 		UNLOCK_ZONE(zone);
+		CLEANUP(ISC_R_SHUTTINGDOWN);
+	}
+
+	/*
+	 * The receive_secure_serial() is loop-serialized for the zone, but it
+	 * is possible for new serial to arrive before the old processing is
+	 * completed, because the old call could have been rescheduled if
+	 * dns_update_signaturesinc() below returned DNS_R_CONTINUE.
+	 */
+	if (zone->rss != NULL) {
+		/* There is an existing processing */
+		UNLOCK_ZONE(zone);
+		if (zone->rss != rss) {
+			/*
+			 * A new 'rss' is sandwiched between a previous call
+			 * with the old 'zone->rss' and a future scheduled
+			 * call with the old 'zone->rss'. Reschedule the new
+			 * 'rss' so the old one can be completed first.
+			 */
+			isc_async_run(zone->loop, receive_secure_serial, rss);
+			return;
+		}
 	} else {
-		zone->rss = rss;
+		/* New arrival */
+		INSIST(rss == zone->rss_next);
+		zone->rss = MOVE_OWNERSHIP(zone->rss_next);
+
 		dns_diff_init(zone->mctx, &zone->rss_diff);
 
 		/*
@@ -14519,6 +14555,7 @@ receive_secure_serial(void *arg) {
 		&log, zone, zone->rss_db, zone->rss_oldver, zone->rss_newver,
 		&zone->rss_diff, zone->sigvalidityinterval, &zone->rss_state);
 	if (result == DNS_R_CONTINUE) {
+		/* Signing quantum reached, reschedule the next chunk. */
 		if (rjournal != NULL) {
 			dns_journal_destroy(&rjournal);
 		}
@@ -14580,7 +14617,7 @@ cleanup:
 	if (zone->rss_raw != NULL) {
 		dns_zone_detach(&zone->rss_raw);
 	}
-	if (result != ISC_R_SUCCESS) {
+	if (result != ISC_R_SUCCESS && result != ISC_R_SHUTTINGDOWN) {
 		LOCK_ZONE(zone);
 		dns__zone_set_resigntime(zone);
 		timenow = isc_time_now();
@@ -14623,15 +14660,24 @@ static isc_result_t
 zone_send_secureserial(dns_zone_t *zone, uint32_t serial) {
 	struct rss *rss = NULL;
 
-	rss = isc_mem_get(zone->secure->mctx, sizeof(*rss));
-	*rss = (struct rss){
-		.serial = serial,
-		.link = ISC_LINK_INITIALIZER,
-	};
-
 	INSIST(LOCKED_ZONE(zone->secure));
-	zone_iattach(zone->secure, &rss->zone);
-	isc_async_run(zone->secure->loop, receive_secure_serial, rss);
+
+	if (zone->secure->rss_next != NULL) {
+		/*
+		 * New version came earlier than the previous one had a
+		 * chance to be processed, just update the serial.
+		 */
+		zone->secure->rss_next->serial = serial;
+	} else {
+		rss = isc_mem_get(zone->secure->mctx, sizeof(*rss));
+		*rss = (struct rss){
+			.serial = serial,
+		};
+		zone_iattach(zone->secure, &rss->zone);
+
+		zone->secure->rss_next = rss;
+		isc_async_run(zone->secure->loop, receive_secure_serial, rss);
+	}
 
 	DNS_ZONE_CLRFLAG(zone, DNS_ZONEFLG_SENDSECURE);
 	return ISC_R_SUCCESS;
@@ -15057,7 +15103,7 @@ zone_send_securedb(dns_zone_t *zone, dns_db_t *db) {
 	struct rss *rss = NULL;
 
 	rss = isc_mem_get(zone->secure->mctx, sizeof(*rss));
-	*rss = (struct rss){ .link = ISC_LINK_INITIALIZER };
+	*rss = (struct rss){ 0 };
 
 	INSIST(LOCKED_ZONE(zone->secure));
 	zone_iattach(zone->secure, &rss->zone);

lib/dns/zone_p.h

@@ -582,6 +582,7 @@ struct dns_zone {
 	dns_db_t *rss_db;
 	dns_zone_t *rss_raw;
 	struct rss *rss;
+	struct rss *rss_next;
 	dns_update_state_t *rss_state;
 
 	isc_stats_t *gluecachestats;