Merge branch '3674-nsupdate--t-timeout-does-not-work' into 'main'

Resolve "nsupdate -t timeout does not work"

Closes #3674

See merge request isc-projects/bind9!7279

CHANGES

@@ -1,3 +1,6 @@
+6141.	[bug]		Fix several issues in nsupdate timeout handling and
+			update the -t option's documentation. [GL #3674]
+
 6140.	[func]		Implement automatic parental-agents ('checkds yes').
 			[GL #3901]
 

bin/nsupdate/nsupdate.c

@@ -94,12 +94,11 @@
 
 #include "../dig/readline.h"
 
-#define MAXCMD	     (128 * 1024)
+#define MAXCMD	 (128 * 1024)
-#define MAXWIRE	     (64 * 1024)
+#define MAXWIRE	 (64 * 1024)
-#define INITTEXT     (2 * 1024)
+#define INITTEXT (2 * 1024)
-#define MAXTEXT	     (128 * 1024)
+#define MAXTEXT	 (128 * 1024)
-#define FIND_TIMEOUT 5
+#define TTL_MAX	 2147483647U /* Maximum signed 32 bit integer. */
-#define TTL_MAX	     2147483647U /* Maximum signed 32 bit integer. */
 
 #define DNSDEFAULTPORT 53
 
@@ -1243,9 +1242,6 @@ parse_args(int argc, char **argv) {
 					isc_commandline_argument);
 				exit(1);
 			}
-			if (udp_timeout == 0) {
-				udp_timeout = UINT_MAX;
-			}
 			break;
 		case 'r':
 			result = isc_parse_uint32(&udp_retries,
@@ -2721,11 +2717,11 @@ recvsoa(void *arg) {
 			srcaddr = localaddr4;
 		}
 
-		result = dns_request_create(
+		result = dns_request_create(requestmgr, soaquery, srcaddr, addr,
-			requestmgr, soaquery, srcaddr, addr, req_transport,
+					    req_transport, req_tls_ctx_cache,
-			req_tls_ctx_cache, options, NULL, FIND_TIMEOUT * 20,
+					    options, NULL, timeout, udp_timeout,
-			FIND_TIMEOUT, 3, isc_loop_main(loopmgr), recvsoa,
+					    udp_retries, isc_loop_main(loopmgr),
-			reqinfo, &request);
+					    recvsoa, reqinfo, &request);
 		check_result(result, "dns_request_create");
 		requests++;
 		return;
@@ -2961,7 +2957,7 @@ sendrequest(isc_sockaddr_t *destaddr, dns_message_t *msg,
 	result = dns_request_create(
 		requestmgr, msg, srcaddr, destaddr, req_transport,
 		req_tls_ctx_cache, options, default_servers ? NULL : tsigkey,
-		FIND_TIMEOUT * 20, FIND_TIMEOUT, 3, isc_loop_main(loopmgr),
+		timeout, udp_timeout, udp_retries, isc_loop_main(loopmgr),
 		recvsoa, reqinfo, request);
 	check_result(result, "dns_request_create");
 	requests++;
@@ -3168,11 +3164,10 @@ send_gssrequest(isc_sockaddr_t *destaddr, dns_message_t *msg,
 		srcaddr = localaddr4;
 	}
 
-	result = dns_request_create(requestmgr, msg, srcaddr, destaddr,
+	result = dns_request_create(
-				    req_transport, req_tls_ctx_cache, options,
+		requestmgr, msg, srcaddr, destaddr, req_transport,
-				    tsigkey, FIND_TIMEOUT * 20, FIND_TIMEOUT, 3,
+		req_tls_ctx_cache, options, tsigkey, timeout, udp_timeout,
-				    isc_loop_main(loopmgr), recvgss, reqinfo,
+		udp_retries, isc_loop_main(loopmgr), recvgss, reqinfo, request);
-				    request);
 	check_result(result, "dns_request_create");
 	if (debugging) {
 		show_message(stdout, msg, "Outgoing update query:");
@@ -3506,6 +3501,8 @@ getinput(void *arg) {
 
 int
 main(int argc, char **argv) {
+	uint32_t timeoutms;
+
 	style = &dns_master_style_debug;
 
 	input = stdin;
@@ -3528,6 +3525,10 @@ main(int argc, char **argv) {
 
 	parse_args(argc, argv);
 
+	/* Set the network manager timeouts in milliseconds. */
+	timeoutms = timeout * 1000;
+	isc_nm_settimeouts(netmgr, timeoutms, timeoutms, timeoutms, timeoutms);
+
 	setup_system();
 
 	isc_loopmgr_setup(loopmgr, getinput, NULL);

bin/nsupdate/nsupdate.rst

@@ -187,7 +187,11 @@ Options
 .. option:: -t timeout
 
    This option sets the maximum time an update request can take before it is aborted. The
-   default is 300 seconds. If zero, the timeout is disabled.
+   default is 300 seconds. If zero, the timeout is disabled for TCP mode. For UDP mode,
+   the option :option:`-u` takes precedence over this option, unless the option :option:`-u`
+   is set to zero, in which case the interval is computed from the :option:`-t` timeout interval
+   and the number of UDP retries. For UDP mode, the timeout can not be disabled, and will
+   be rounded up to 1 second in case if both :option:`-t` and :option:`-u` are set to zero.
 
 .. option:: -T
 

bin/tests/system/nsupdate/ans4/ans.pl

@@ -31,6 +31,8 @@ if (!$localport) { $localport = 5300; }
 
 my $udpsock = IO::Socket::INET->new(LocalAddr => "$server_addr",
    LocalPort => $localport, Proto => "udp", Reuse => 1) or die "$!";
+my $tcpsock = IO::Socket::INET->new(LocalAddr => "$server_addr",
+   LocalPort => $localport, Proto => "tcp", Listen => 5, Reuse => 1) or die "$!";
 
 print "listening on $server_addr:$localport.\n";
 
@@ -49,6 +51,7 @@ for (;;) {
 
 	$rin = '';
 	vec($rin, fileno($udpsock), 1) = 1;
+	vec($rin, fileno($tcpsock), 1) = 1;
 
 	select($rout = $rin, undef, undef, undef);
 
@@ -56,5 +59,7 @@ for (;;) {
 		printf "UDP request\n";
 		my $buf;
 		$udpsock->recv($buf, 512);
+	} elsif (vec($rout, fileno($tcpsock), 1)) {
+		printf "TCP request\n";
 	}
 }

bin/tests/system/nsupdate/tests.sh

@@ -1276,15 +1276,97 @@ grep "records in zone (4) exceeds max-records (3)" ns3/named.run > /dev/null ||
 
 n=$((n + 1))
 ret=0
-echo_i "check whether valid addresses are used for primary failover ($n)"
+echo_i "check whether valid addresses are used for primary failover (UDP with defaults) ($n)"
-$NSUPDATE -t 1 <<END > nsupdate.out.test$n 2>&1 && ret=1
+t1=$($PERL -e 'print time()')
+$NSUPDATE <<END > nsupdate.out.test$n 2>&1 && ret=1
 server 10.53.0.4 ${PORT}
 zone unreachable.
 update add unreachable. 600 A 192.0.2.1
 send
 END
+t2=`$PERL -e 'print time()'`
 grep "; Communication with 10.53.0.4#${PORT} failed: timed out" nsupdate.out.test$n > /dev/null 2>&1 || ret=1
 grep "not implemented" nsupdate.out.test$n > /dev/null 2>&1 && ret=1
+elapsed=$((t2 - t1))
+# Check that default timeout value is respected, there should be 4 tries with 3 seconds each.
+test $elapsed -lt 12 && ret=1
+test $elapsed -gt 15 && ret=1
+[ $ret = 0 ] || { echo_i "failed"; status=1; }
+
+n=$((n + 1))
+ret=0
+echo_i "check whether valid addresses are used for primary failover (UDP with -u udptimeout) ($n)"
+t1=$($PERL -e 'print time()')
+$NSUPDATE -u 4 -r 1 <<END > nsupdate.out.test$n 2>&1 && ret=1
+server 10.53.0.4 ${PORT}
+zone unreachable.
+update add unreachable. 600 A 192.0.2.1
+send
+END
+t2=`$PERL -e 'print time()'`
+grep "; Communication with 10.53.0.4#${PORT} failed: timed out" nsupdate.out.test$n > /dev/null 2>&1 || ret=1
+grep "not implemented" nsupdate.out.test$n > /dev/null 2>&1 && ret=1
+elapsed=$((t2 - t1))
+# Check that given timeout value is respected, there should be 2 tries with 4 seconds each.
+test $elapsed -lt 8 && ret=1
+test $elapsed -gt 12 && ret=1
+[ $ret = 0 ] || { echo_i "failed"; status=1; }
+
+n=$((n + 1))
+ret=0
+echo_i "check whether valid addresses are used for primary failover (UDP with -t timeout) ($n)"
+t1=$($PERL -e 'print time()')
+$NSUPDATE -u 0 -t 8 -r 1 <<END > nsupdate.out.test$n 2>&1 && ret=1
+server 10.53.0.4 ${PORT}
+zone unreachable.
+update add unreachable. 600 A 192.0.2.1
+send
+END
+t2=`$PERL -e 'print time()'`
+grep "; Communication with 10.53.0.4#${PORT} failed: timed out" nsupdate.out.test$n > /dev/null 2>&1 || ret=1
+grep "not implemented" nsupdate.out.test$n > /dev/null 2>&1 && ret=1
+elapsed=$((t2 - t1))
+# Check that given timeout value is respected, there should be 2 tries with 4 seconds each.
+test $elapsed -lt 8 && ret=1
+test $elapsed -gt 12 && ret=1
+[ $ret = 0 ] || { echo_i "failed"; status=1; }
+
+n=$((n + 1))
+ret=0
+echo_i "check whether valid addresses are used for primary failover (UDP with -u udptimeout -t timeout) ($n)"
+t1=$($PERL -e 'print time()')
+$NSUPDATE -u 4 -t 30 -r 1 <<END > nsupdate.out.test$n 2>&1 && ret=1
+server 10.53.0.4 ${PORT}
+zone unreachable.
+update add unreachable. 600 A 192.0.2.1
+send
+END
+t2=`$PERL -e 'print time()'`
+grep "; Communication with 10.53.0.4#${PORT} failed: timed out" nsupdate.out.test$n > /dev/null 2>&1 || ret=1
+grep "not implemented" nsupdate.out.test$n > /dev/null 2>&1 && ret=1
+elapsed=$((t2 - t1))
+# Check that given timeout value is respected, there should be 2 tries with 4 seconds each, as -u takes precedence over -t.
+test $elapsed -lt 8 && ret=1
+test $elapsed -gt 12 && ret=1
+[ $ret = 0 ] || { echo_i "failed"; status=1; }
+
+n=$((n + 1))
+ret=0
+echo_i "check whether valid addresses are used for primary failover (TCP with -t timeout) ($n)"
+t1=$($PERL -e 'print time()')
+$NSUPDATE -t 8 -v <<END > nsupdate.out.test$n 2>&1 && ret=1
+server 10.53.0.4 ${PORT}
+zone unreachable.
+update add unreachable. 600 A 192.0.2.1
+send
+END
+t2=`$PERL -e 'print time()'`
+grep "; Communication with 10.53.0.4#${PORT} failed: timed out" nsupdate.out.test$n > /dev/null 2>&1 || ret=1
+grep "not implemented" nsupdate.out.test$n > /dev/null 2>&1 && ret=1
+elapsed=$((t2 - t1))
+# Check that given timeout value is respected, there should be 1 try with 8 seconds.
+test $elapsed -lt 8 && ret=1
+test $elapsed -gt 12 && ret=1
 [ $ret = 0 ] || { echo_i "failed"; status=1; }
 
 n=$((n + 1))

lib/dns/request.c

@@ -421,6 +421,8 @@ dns_request_createraw(dns_requestmgr_t *requestmgr, isc_buffer_t *msgbuf,
 	REQUIRE(cb != NULL);
 	REQUIRE(requestp != NULL && *requestp == NULL);
 	REQUIRE(timeout > 0);
+	REQUIRE(udpretries != UINT_MAX);
+
 	if (srcaddr != NULL) {
 		REQUIRE(isc_sockaddr_pf(srcaddr) == isc_sockaddr_pf(destaddr));
 	}
@@ -447,7 +449,7 @@ dns_request_createraw(dns_requestmgr_t *requestmgr, isc_buffer_t *msgbuf,
 	request->cb = cb;
 	request->arg = arg;
 	request->result = ISC_R_FAILURE;
-	request->udpcount = udpretries;
+	request->udpcount = udpretries + 1;
 
 	isc_buffer_usedregion(msgbuf, &r);
 	if (r.length < DNS_MESSAGE_HEADERLEN || r.length > 65535) {
@@ -460,7 +462,7 @@ dns_request_createraw(dns_requestmgr_t *requestmgr, isc_buffer_t *msgbuf,
 		request->timeout = timeout * 1000;
 	} else {
 		if (udptimeout == 0) {
-			udptimeout = timeout / (udpretries + 1);
+			udptimeout = timeout / request->udpcount;
 		}
 		if (udptimeout == 0) {
 			udptimeout = 1;
@@ -569,6 +571,7 @@ dns_request_create(dns_requestmgr_t *requestmgr, dns_message_t *message,
 	REQUIRE(cb != NULL);
 	REQUIRE(requestp != NULL && *requestp == NULL);
 	REQUIRE(timeout > 0);
+	REQUIRE(udpretries != UINT_MAX);
 
 	mctx = requestmgr->mctx;
 
@@ -598,9 +601,7 @@ dns_request_create(dns_requestmgr_t *requestmgr, dns_message_t *message,
 	request->cb = cb;
 	request->arg = arg;
 	request->result = ISC_R_FAILURE;
-	request->udpcount = udpretries;
+	request->udpcount = udpretries + 1;
-
-	request->udpcount = udpretries;
 
 	if (key != NULL) {
 		dns_tsigkey_attach(key, &request->tsigkey);
@@ -615,8 +616,8 @@ dns_request_create(dns_requestmgr_t *requestmgr, dns_message_t *message,
 		tcp = true;
 		request->timeout = timeout * 1000;
 	} else {
-		if (udptimeout == 0 && udpretries != 0) {
+		if (udptimeout == 0) {
-			udptimeout = timeout / (udpretries + 1);
+			udptimeout = timeout / request->udpcount;
 		}
 		if (udptimeout == 0) {
 			udptimeout = 1;
@@ -971,9 +972,13 @@ req_response(isc_result_t result, isc_region_t *region, void *arg) {
 	req_log(ISC_LOG_DEBUG(3), "req_response: request %p: %s", request,
 		isc_result_totext(result));
 
+	REQUIRE(VALID_REQUEST(request));
+
 	if (result == ISC_R_TIMEDOUT) {
 		LOCK(&request->requestmgr->locks[request->hash]);
-		if (request->udpcount != 0) {
+		if (request->udpcount > 1 &&
+		    (request->flags & DNS_REQUEST_F_TCP) == 0)
+		{
 			request->udpcount -= 1;
 			dns_dispatch_resume(request->dispentry,
 					    request->timeout);
@@ -988,8 +993,6 @@ req_response(isc_result_t result, isc_region_t *region, void *arg) {
 		goto done;
 	}
 
-	REQUIRE(VALID_REQUEST(request));
-
 	LOCK(&request->requestmgr->locks[request->hash]);
 
 	if (result != ISC_R_SUCCESS) {