Merge branch '1897-fix-max-transfer-timeouts' into 'main'

Reimplement the max-transfer-time-out and max-transfer-idle-out

Closes #1897

See merge request isc-projects/bind9!5850

CHANGES

@@ -1,3 +1,6 @@
+5811.	[bug]		Reimplement the maximum and idle timeouts for outgoing
+			zone tranfers. [GL #1897]
+
 5810.	[func]		New option '-J' for dnssec-signzone and dnssec-verify
 			allows loading journal files. [GL #2486]
 

bin/tests/system/run.sh.in

@@ -88,6 +88,7 @@ if ! $do_run; then
         LOG_DRIVER_FLAGS="--verbose yes --color-tests yes" \
         LOG_FLAGS="$log_flags" \
         TEST_LARGE_MAP="${TEST_LARGE_MAP}" \
+        CI_ENABLE_ALL_TESTS="${CI_ENABLE_ALL_TESTS}" \
         make -e check
     exit $?
 fi

bin/tests/system/timeouts/conftest.py

@@ -20,6 +20,9 @@ def pytest_configure(config):
     config.addinivalue_line(
         "markers", "dnspython2: mark tests that need dnspython >= 2.0.0"
     )
+    config.addinivalue_line(
+        "markers", "long: mark tests that take a long time to run"
+    )
 
 
 def pytest_collection_modifyitems(config, items):
@@ -46,6 +49,13 @@ def pytest_collection_modifyitems(config, items):
             if "dnspython2" in item.keywords:
                 item.add_marker(skip_dnspython2)
 
+    skip_long_tests = pytest.mark.skip(
+        reason="need CI_ENABLE_ALL_TESTS environment variable")
+    if not os.environ.get("CI_ENABLE_ALL_TESTS"):
+        for item in items:
+            if "long" in item.keywords:
+                item.add_marker(skip_long_tests)
+
 
 @pytest.fixture
 def port(request):

bin/tests/system/timeouts/ns1/named.conf.in

@@ -30,6 +30,8 @@ options {
 	tcp-initial-timeout 20;
 	tcp-idle-timeout 50;
 	tcp-keepalive-timeout 70;
+	max-transfer-time-out 5; /* minutes */
+	max-transfer-idle-out 1; /* minutes */
 };
 
 zone "." {

bin/tests/system/timeouts/tests-tcp.py

@@ -220,3 +220,76 @@ def test_send_timeout(port):
                 (response, rtime) = dns.query.receive_tcp(sock, timeout())
             except ConnectionError as e:
                 raise EOFError from e
+
+
+@pytest.mark.dnspython
+@pytest.mark.dnspython2
+@pytest.mark.long
+def test_max_transfer_idle_out(port):
+    import dns.query
+    import dns.rdataclass
+    import dns.rdatatype
+
+    with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as sock:
+        sock.connect(("10.53.0.1", port))
+
+        name = dns.name.from_text("example.")
+        msg = create_msg("example.", "AXFR")
+        (sbytes, stime) = dns.query.send_tcp(sock, msg, timeout())
+
+        # Receive the initial DNS message with SOA
+        (response, rtime) = dns.query.receive_tcp(sock, timeout(),
+                                                  one_rr_per_rrset=True)
+        soa = response.get_rrset(dns.message.ANSWER, name,
+                                 dns.rdataclass.IN, dns.rdatatype.SOA)
+        assert soa is not None
+
+        time.sleep(61)  # max-transfer-idle-out is 1 minute
+
+        with pytest.raises(ConnectionResetError):
+            # Process queued TCP messages
+            while True:
+                (response, rtime) = \
+                    dns.query.receive_tcp(sock, timeout(),
+                                          one_rr_per_rrset=True)
+                soa = response.get_rrset(dns.message.ANSWER, name,
+                                         dns.rdataclass.IN, dns.rdatatype.SOA)
+                if soa is not None:
+                    break
+            assert soa is None
+
+
+@pytest.mark.dnspython
+@pytest.mark.dnspython2
+@pytest.mark.long
+def test_max_transfer_time_out(port):
+    import dns.query
+    import dns.rdataclass
+    import dns.rdatatype
+
+    with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as sock:
+        sock.connect(("10.53.0.1", port))
+
+        name = dns.name.from_text("example.")
+        msg = create_msg("example.", "AXFR")
+        (sbytes, stime) = dns.query.send_tcp(sock, msg, timeout())
+
+        # Receive the initial DNS message with SOA
+        (response, rtime) = dns.query.receive_tcp(sock, timeout(),
+                                                  one_rr_per_rrset=True)
+        soa = response.get_rrset(dns.message.ANSWER, name,
+                                 dns.rdataclass.IN, dns.rdatatype.SOA)
+        assert soa is not None
+
+        # The loop should timeout at the 5 minutes (max-transfer-time-out)
+        with pytest.raises(EOFError):
+            while True:
+                time.sleep(1)
+                (response, rtime) = \
+                    dns.query.receive_tcp(sock, timeout(),
+                                          one_rr_per_rrset=True)
+                soa = response.get_rrset(dns.message.ANSWER, name,
+                                         dns.rdataclass.IN, dns.rdatatype.SOA)
+                if soa is not None:
+                    break
+        assert soa is None

doc/notes/notes-current.rst

@@ -81,3 +81,8 @@ Bug Fixes
 
 - Client TCP connections are now closed immediately when data received
   cannot be parsed as a valid DNS request. :gl:`#3149`
+
+- The ``max-transfer-time-out`` and ``max-transfer-idle-out`` options were
+  not implemented when the BIND 9 networking stack was refactored in 9.16.
+  The missing functionality has been re-implemented and outgoing zone
+  transfers now time out properly when not progressing. :gl:`#1897`

lib/isc/Makefile.am

@@ -109,6 +109,7 @@ libisc_la_SOURCES =		\
 	netmgr/netmgr.c		\
 	netmgr/tcp.c		\
 	netmgr/tcpdns.c		\
+	netmgr/timer.c		\
 	netmgr/tlsdns.c		\
 	netmgr/udp.c		\
 	netmgr/uv-compat.c	\

lib/isc/include/isc/netmgr.h

@@ -700,3 +700,27 @@ isc__nm_force_tid(int tid);
 
 void
 isc_nmhandle_setwritetimeout(isc_nmhandle_t *handle, uint64_t write_timeout);
+
+/*
+ * Timer related functions
+ */
+
+typedef struct isc_nm_timer isc_nm_timer_t;
+
+typedef void (*isc_nm_timer_cb)(void *, isc_result_t);
+
+void
+isc_nm_timer_create(isc_nmhandle_t *, isc_nm_timer_cb, void *,
+		    isc_nm_timer_t **);
+
+void
+isc_nm_timer_attach(isc_nm_timer_t *, isc_nm_timer_t **);
+
+void
+isc_nm_timer_detach(isc_nm_timer_t **);
+
+void
+isc_nm_timer_start(isc_nm_timer_t *, uint64_t);
+
+void
+isc_nm_timer_stop(isc_nm_timer_t *);

lib/isc/netmgr/timer.c

@@ -0,0 +1,118 @@
+/*
+ * 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.
+ */
+
+#include <uv.h>
+
+#include <isc/netmgr.h>
+#include <isc/util.h>
+
+#include "netmgr-int.h"
+
+struct isc_nm_timer {
+	isc_refcount_t references;
+	uv_timer_t timer;
+	isc_nmhandle_t *handle;
+	isc_nm_timer_cb cb;
+	void *cbarg;
+};
+
+void
+isc_nm_timer_create(isc_nmhandle_t *handle, isc_nm_timer_cb cb, void *cbarg,
+		    isc_nm_timer_t **timerp) {
+	isc__networker_t *worker = NULL;
+	isc_nmsocket_t *sock = NULL;
+	isc_nm_timer_t *timer = NULL;
+	int r;
+
+	REQUIRE(isc__nm_in_netthread());
+	REQUIRE(VALID_NMHANDLE(handle));
+	REQUIRE(VALID_NMSOCK(handle->sock));
+
+	sock = handle->sock;
+	worker = &sock->mgr->workers[isc_nm_tid()];
+
+	timer = isc_mem_get(sock->mgr->mctx, sizeof(*timer));
+	*timer = (isc_nm_timer_t){ .cb = cb, .cbarg = cbarg };
+	isc_refcount_init(&timer->references, 1);
+	isc_nmhandle_attach(handle, &timer->handle);
+
+	r = uv_timer_init(&worker->loop, &timer->timer);
+	UV_RUNTIME_CHECK(uv_timer_init, r);
+
+	uv_handle_set_data((uv_handle_t *)&timer->timer, timer);
+
+	*timerp = timer;
+}
+
+void
+isc_nm_timer_attach(isc_nm_timer_t *timer, isc_nm_timer_t **timerp) {
+	REQUIRE(timer != NULL);
+	REQUIRE(timerp != NULL && *timerp == NULL);
+
+	isc_refcount_increment(&timer->references);
+	*timerp = timer;
+}
+
+static void
+timer_destroy(uv_handle_t *uvhandle) {
+	isc_nm_timer_t *timer = uv_handle_get_data(uvhandle);
+	isc_nmhandle_t *handle = timer->handle;
+	isc_mem_t *mctx = timer->handle->sock->mgr->mctx;
+
+	isc_mem_put(mctx, timer, sizeof(*timer));
+
+	isc_nmhandle_detach(&handle);
+}
+
+void
+isc_nm_timer_detach(isc_nm_timer_t **timerp) {
+	isc_nm_timer_t *timer = NULL;
+	isc_nmhandle_t *handle = NULL;
+
+	REQUIRE(timerp != NULL && *timerp != NULL);
+
+	timer = *timerp;
+	*timerp = NULL;
+
+	handle = timer->handle;
+
+	REQUIRE(isc__nm_in_netthread());
+	REQUIRE(VALID_NMHANDLE(handle));
+	REQUIRE(VALID_NMSOCK(handle->sock));
+
+	if (isc_refcount_decrement(&timer->references) == 1) {
+		uv_timer_stop(&timer->timer);
+		uv_close((uv_handle_t *)&timer->timer, timer_destroy);
+	}
+}
+
+static void
+timer_cb(uv_timer_t *uvtimer) {
+	isc_nm_timer_t *timer = uv_handle_get_data((uv_handle_t *)uvtimer);
+
+	REQUIRE(timer->cb != NULL);
+
+	timer->cb(timer->cbarg, ISC_R_TIMEDOUT);
+}
+
+void
+isc_nm_timer_start(isc_nm_timer_t *timer, uint64_t timeout) {
+	int r = uv_timer_start(&timer->timer, timer_cb, timeout, 0);
+	UV_RUNTIME_CHECK(uv_timer_start, r);
+}
+
+void
+isc_nm_timer_stop(isc_nm_timer_t *timer) {
+	int r = uv_timer_stop(&timer->timer);
+	UV_RUNTIME_CHECK(uv_timer_stop, r);
+}

lib/ns/include/ns/client.h

@@ -198,8 +198,6 @@ struct ns_client {
 	int16_t		ednsversion; /* -1 noedns */
 	uint16_t	additionaldepth;
 	void (*cleanup)(ns_client_t *);
-	void (*shutdown)(void *arg, isc_result_t result);
-	void	     *shutdown_arg;
 	ns_query_t    query;
 	isc_time_t    requesttime;
 	isc_stdtime_t now;

lib/ns/xfrout.c

@@ -16,6 +16,7 @@
 
 #include <isc/formatcheck.h>
 #include <isc/mem.h>
+#include <isc/netmgr.h>
 #include <isc/print.h>
 #include <isc/result.h>
 #include <isc/stats.h>
@@ -668,6 +669,12 @@ typedef struct {
 	const char *mnemonic;	/* Style of transfer */
 	uint32_t end_serial;	/* Serial number after XFR is done */
 	struct xfr_stats stats; /*%< Transfer statistics */
+
+	/* Timeouts */
+	uint64_t maxtime; /*%< Maximum XFR timeout (in ms) */
+	isc_nm_timer_t *maxtime_timer;
+
+	uint64_t idletime; /*%< XFR idle timeout (in ms) */
 } xfrout_ctx_t;
 
 static void
@@ -696,7 +703,7 @@ static void
 xfrout_ctx_destroy(xfrout_ctx_t **xfrp);
 
 static void
-xfrout_client_shutdown(void *arg, isc_result_t result);
+xfrout_client_timeout(void *arg, isc_result_t result);
 
 static void
 xfrout_log1(ns_client_t *client, dns_name_t *zonename, dns_rdataclass_t rdclass,
@@ -1146,6 +1153,14 @@ have_stream:
 		}
 	}
 
+	/* Start the timers */
+	if (xfr->maxtime > 0) {
+		xfrout_log(xfr, ISC_LOG_ERROR,
+			   "starting maxtime timer %" PRIu64 " ms",
+			   xfr->maxtime);
+		isc_nm_timer_start(xfr->maxtime_timer, xfr->maxtime);
+	}
+
 	/*
 	 * Hand the context over to sendstream().  Set xfr to NULL;
 	 * sendstream() is responsible for either passing the
@@ -1205,64 +1220,52 @@ xfrout_ctx_create(isc_mem_t *mctx, ns_client_t *client, unsigned int id,
 		  bool verified_tsig, unsigned int maxtime,
 		  unsigned int idletime, bool many_answers,
 		  xfrout_ctx_t **xfrp) {
-	xfrout_ctx_t *xfr;
-	unsigned int len;
-	void *mem;
+	xfrout_ctx_t *xfr = NULL;
+	unsigned int len = NS_CLIENT_TCP_BUFFER_SIZE;
+	void *mem = NULL;
 
 	REQUIRE(xfrp != NULL && *xfrp == NULL);
 
-	UNUSED(maxtime);
-	UNUSED(idletime);
-
 	xfr = isc_mem_get(mctx, sizeof(*xfr));
-	xfr->mctx = NULL;
+	*xfr = (xfrout_ctx_t){
+		.client = client,
+		.id = id,
+		.qname = qname,
+		.qtype = qtype,
+		.qclass = qclass,
+		.maxtime = maxtime * 1000,   /* in milliseconds */
+		.idletime = idletime * 1000, /* In milliseconds */
+		.tsigkey = tsigkey,
+		.lasttsig = lasttsig,
+		.verified_tsig = verified_tsig,
+		.many_answers = many_answers,
+	};
+
 	isc_mem_attach(mctx, &xfr->mctx);
-	xfr->client = client;
-	xfr->id = id;
-	xfr->qname = qname;
-	xfr->qtype = qtype;
-	xfr->qclass = qclass;
-	xfr->zone = NULL;
-	xfr->db = NULL;
-	xfr->ver = NULL;
+
 	if (zone != NULL) { /* zone will be NULL if it's DLZ */
 		dns_zone_attach(zone, &xfr->zone);
 	}
 	dns_db_attach(db, &xfr->db);
 	dns_db_attachversion(db, ver, &xfr->ver);
-	xfr->question_added = false;
-	xfr->end_of_stream = false;
-	xfr->tsigkey = tsigkey;
-	xfr->lasttsig = lasttsig;
-	xfr->verified_tsig = verified_tsig;
-	xfr->many_answers = many_answers;
-	xfr->sends = 0;
-	xfr->shuttingdown = false;
-	xfr->poll = false;
-	xfr->mnemonic = NULL;
-	xfr->buf.base = NULL;
-	xfr->buf.length = 0;
-	xfr->txmem = NULL;
-	xfr->txmemlen = 0;
-	xfr->stream = NULL;
-	xfr->quota = NULL;
 
-	xfr->stats.nmsg = 0;
-	xfr->stats.nrecs = 0;
-	xfr->stats.nbytes = 0;
 	isc_time_now(&xfr->stats.start);
 
+	isc_nm_timer_create(xfr->client->handle, xfrout_client_timeout, xfr,
+			    &xfr->maxtime_timer);
+
 	/*
 	 * Allocate a temporary buffer for the uncompressed response
-	 * message data.  The size should be no more than 65535 bytes
-	 * so that the compressed data will fit in a TCP message,
-	 * and no less than 65535 bytes so that an almost maximum-sized
-	 * RR will fit.  Note that although 65535-byte RRs are allowed
-	 * in principle, they cannot be zone-transferred (at least not
-	 * if uncompressible), because the message and RR headers would
-	 * push the size of the TCP message over the 65536 byte limit.
+	 * message data.  The buffer size must be 65535 bytes
+	 * (NS_CLIENT_TCP_BUFFER_SIZE): small enough that compressed
+	 * data will fit in a single TCP message, and big enough to
+	 * hold a maximum-sized RR.
+	 *
+	 * Note that although 65535-byte RRs are allowed in principle, they
+	 * cannot be zone-transferred (at least not if uncompressible),
+	 * because the message and RR headers would push the size of the
+	 * TCP message over the 65536 byte limit.
 	 */
-	len = 65535;
 	mem = isc_mem_get(mctx, len);
 	isc_buffer_init(&xfr->buf, mem, len);
 
@@ -1270,19 +1273,11 @@ xfrout_ctx_create(isc_mem_t *mctx, ns_client_t *client, unsigned int id,
 	 * Allocate another temporary buffer for the compressed
 	 * response message.
 	 */
-	len = NS_CLIENT_TCP_BUFFER_SIZE;
 	mem = isc_mem_get(mctx, len);
 	isc_buffer_init(&xfr->txbuf, (char *)mem, len);
 	xfr->txmem = mem;
 	xfr->txmemlen = len;
 
-	/*
-	 * Register a shutdown callback with the client, so that we
-	 * can stop the transfer immediately when the client task
-	 * gets a shutdown event.
-	 */
-	xfr->client->shutdown = xfrout_client_shutdown;
-	xfr->client->shutdown_arg = xfr;
 	/*
 	 * These MUST be after the last "goto failure;" / CHECK to
 	 * prevent a double free by the caller.
@@ -1567,6 +1562,10 @@ sendstream(xfrout_ctx_t *xfr) {
 
 		isc_nmhandle_attach(xfr->client->handle,
 				    &xfr->client->sendhandle);
+		if (xfr->idletime > 0) {
+			isc_nmhandle_setwritetimeout(xfr->client->sendhandle,
+						     xfr->idletime);
+		}
 		isc_nm_send(xfr->client->sendhandle, &used, xfrout_senddone,
 			    xfr);
 		xfr->sends++;
@@ -1632,8 +1631,8 @@ xfrout_ctx_destroy(xfrout_ctx_t **xfrp) {
 
 	INSIST(xfr->sends == 0);
 
-	xfr->client->shutdown = NULL;
-	xfr->client->shutdown_arg = NULL;
+	isc_nm_timer_stop(xfr->maxtime_timer);
+	isc_nm_timer_detach(&xfr->maxtime_timer);
 
 	if (xfr->stream != NULL) {
 		xfr->stream->methods->destroy(&xfr->stream);
@@ -1740,9 +1739,12 @@ xfrout_maybe_destroy(xfrout_ctx_t *xfr) {
 }
 
 static void
-xfrout_client_shutdown(void *arg, isc_result_t result) {
+xfrout_client_timeout(void *arg, isc_result_t result) {
 	xfrout_ctx_t *xfr = (xfrout_ctx_t *)arg;
-	xfrout_fail(xfr, result, "aborted");
+
+	xfr->shuttingdown = true;
+	xfrout_log(xfr, ISC_LOG_ERROR, "%s: %s", "aborted",
+		   isc_result_totext(result));
 }
 
 /*