4533. [bug] dns_client_update should terminate on prerequiste

failures (NXDOMAIN, YXDOMAIN, NXRRSET, YXRRSET)
                        and also on BADZONE.  [RT #43865]

CHANGES

@@ -1,3 +1,7 @@
+4533.	[bug]		dns_client_update should terminate on prerequiste
+			failures (NXDOMAIN, YXDOMAIN, NXRRSET, YXRRSET)
+			and also on BADZONE.  [RT #43865]
+
 4532.	[contrib]	Make gen-data-queryperf.py python 3 compatible.
 			[RT #43836]
 

bin/tests/system/conf.sh.in

@@ -65,6 +65,7 @@ KEYDELETE=$TOP/bin/tests/system/tkey/keydelete
 LWTEST=$TOP/bin/tests/system/lwresd/lwtest
 MAKEJOURNAL=$TOP/bin/tests/makejournal
 PIPEQUERIES=$TOP/bin/tests/system/pipelined/pipequeries
+SAMPLEUPDATE=$TOP/lib/samples/sample-update
 
 # The "stress" test is not run by default since it creates enough
 # load on the machine to make it unusable to other users.
@@ -166,6 +167,7 @@ export RANDFILE
 export RESOLVE
 export RNDC
 export RRCHECKER
+export SAMPLEUPDATE
 export SIGNER
 export SUBDIRS
 export TESTSOCK6

bin/tests/system/nsupdate/clean.sh

@@ -34,3 +34,8 @@ rm -f ns3/nsec3param.test.db
 rm -f ns3/too-big.test.db
 rm -f nsupdate.out*
 rm -f typelist.out.*
+rm -f ns1/sample.db
+rm -f ns2/sample.db
+rm -f update.out.*
+rm -f check.out.*
+rm -f update.out.*

bin/tests/system/nsupdate/ns1/named.conf

@@ -124,3 +124,9 @@ zone "many.test" {
 	allow-update { any; };
 	file "many.test.db";
 };
+
+zone "sample" {
+	type master;
+	allow-update { any; };
+	file "sample.db";
+};

bin/tests/system/nsupdate/ns1/sample.db.in

@@ -0,0 +1,8 @@
+@ 0 SOA ns1 . 0 0 0 0 0
+@ 0 NS ns1
+ns1 0 A 10.53.0.1
+; a RRset that exists
+exists 0 TXT This RRset exists.
+; nxdomain
+; A named without a TXT RRset
+no-txt 0 A 1.2.3.4

bin/tests/system/nsupdate/ns2/named.conf

@@ -55,4 +55,10 @@ view primary {
 		file "update.bk";
 		allow-transfer { any; };
 	};
+
+	zone "sample" {
+		type master;
+		allow-update { any; };
+		file "sample.db";
+	};
 };

bin/tests/system/nsupdate/ns2/sample.db.in

@@ -0,0 +1,10 @@
+@ 0 SOA ns2 . 0 0 0 0 0
+@ 0 NS ns2
+ns2 0 A 10.53.0.2
+;
+; These prerequistes are reversed, relative to ns1/sample.db.in:
+; 'exists' does not exist.
+nxdomain 0 TXT This RRset exists.
+
+; a name with a TXT RRset
+no-txt 0 TXT This RRset exists

bin/tests/system/nsupdate/setup.sh

@@ -58,3 +58,6 @@ $DDNSCONFGEN -q -r $RANDFILE -a hmac-sha512 -k sha512-key -z keytests.nil > ns1/
 
 cp -f ns1/many.test.db.in ns1/many.test.db
 rm -f ns1/many.test.db.jnl
+
+cp ns1/sample.db.in ns1/sample.db
+cp ns2/sample.db.in ns2/sample.db

bin/tests/system/nsupdate/tests.sh

@@ -701,5 +701,76 @@ grep "status: NXDOMAIN" dig.out.ns3.test$n > /dev/null || ret=1
 grep "records in zone (4) exceeds max-records (3)" ns3/named.run > /dev/null || ret=1
 [ $ret = 0 ] || { echo I:failed; status=1; }
 
+#
+#  Add client library tests here
+#
+n=`expr $n + 1`
+echo "I:check that dns_client_update handles prerequisite NXDOMAIN failure ($n)"
+$SAMPLEUPDATE -P 5300 -a 10.53.0.1 -a 10.53.0.2 -p "nxdomain exists.sample" \
+	add "nxdomain-exists.sample 0 in a 1.2.3.4" > update.out.test$n 2>&1
+$SAMPLEUPDATE -P 5300 -a 10.53.0.2 -p "nxdomain exists.sample" \
+	add "check-nxdomain-exists.sample 0 in a 1.2.3.4" > update.out.check$n 2>&1
+$DIG +tcp @10.53.0.1 -p 5300 a nxdomain-exists.sample > dig.out.ns1.test$n
+$DIG +tcp @10.53.0.2 -p 5300 a nxdomain-exists.sample > dig.out.ns2.test$n
+$DIG +tcp @10.53.0.2 -p 5300 a check-nxdomain-exists.sample > check.out.ns2.test$n
+grep "update failed: YXDOMAIN" update.out.test$n > /dev/null || ret=1
+grep "update succeeded" update.out.check$n > /dev/null || ret=1
+grep "status: NXDOMAIN" dig.out.ns1.test$n > /dev/null || ret=1
+grep "status: NXDOMAIN" dig.out.ns2.test$n > /dev/null || ret=1
+grep "status: NOERROR" check.out.ns2.test$n > /dev/null || ret=1
+[ $ret = 0 ] || { echo I:failed; status=1; }
+
+n=`expr $n + 1`
+echo "I:check that dns_client_update handles prerequisite YXDOMAIN failure ($n)"
+$SAMPLEUPDATE -P 5300 -a 10.53.0.1 -a 10.53.0.2 -p "yxdomain nxdomain.sample" \
+	add "yxdomain-nxdomain.sample 0 in a 1.2.3.4" > update.out.test$n 2>&1
+$SAMPLEUPDATE -P 5300 -a 10.53.0.2 -p "yxdomain nxdomain.sample" \
+	add "check-yxdomain-nxdomain.sample 0 in a 1.2.3.4" > update.out.check$n 2>&1
+$DIG +tcp @10.53.0.1 -p 5300 a nxdomain-exists.sample > dig.out.ns1.test$n
+$DIG +tcp @10.53.0.2 -p 5300 a nxdomain-exists.sample > dig.out.ns2.test$n
+$DIG +tcp @10.53.0.2 -p 5300 a check-nxdomain-exists.sample > check.out.ns2.test$n
+grep "update failed: NXDOMAIN" update.out.test$n > /dev/null || ret=1
+grep "update succeeded" update.out.check$n > /dev/null || ret=1
+grep "status: NXDOMAIN" dig.out.ns1.test$n > /dev/null || ret=1
+grep "status: NXDOMAIN" dig.out.ns2.test$n > /dev/null || ret=1
+grep "status: NOERROR" check.out.ns2.test$n > /dev/null || ret=1
+[ $ret = 0 ] || { echo I:failed; status=1; }
+
+n=`expr $n + 1`
+echo "I:check that dns_client_update handles prerequisite NXRRSET failure ($n)"
+$SAMPLEUPDATE -P 5300 -a 10.53.0.1 -a 10.53.0.2 -p "nxrrset exists.sample TXT This RRset exists." \
+	add "nxrrset-exists.sample 0 in a 1.2.3.4" > update.out.test$n 2>&1
+$SAMPLEUPDATE -P 5300 -a 10.53.0.2 -p "nxrrset exists.sample TXT This RRset exists." \
+	add "check-nxrrset-exists.sample 0 in a 1.2.3.4" > update.out.check$n 2>&1
+$DIG +tcp @10.53.0.1 -p 5300 a nxrrset-exists.sample > dig.out.ns1.test$n
+$DIG +tcp @10.53.0.2 -p 5300 a nxrrset-exists.sample > dig.out.ns2.test$n
+$DIG +tcp @10.53.0.2 -p 5300 a check-nxrrset-exists.sample > check.out.ns2.test$n
+grep "update failed: YXRRSET" update.out.test$n > /dev/null || ret=1
+grep "update succeeded" update.out.check$n > /dev/null || ret=1
+grep "status: NXDOMAIN" dig.out.ns1.test$n > /dev/null || ret=1
+grep "status: NXDOMAIN" dig.out.ns2.test$n > /dev/null || ret=1
+grep "status: NOERROR" check.out.ns2.test$n > /dev/null || ret=1
+[ $ret = 0 ] || { echo I:failed; status=1; }
+
+n=`expr $n + 1`
+echo "I:check that dns_client_update handles prerequisite YXRRSET failure ($n)"
+$SAMPLEUPDATE -P 5300 -a 10.53.0.1 -a 10.53.0.2 -p "yxrrset no-txt.sample TXT" \
+	add "yxrrset-nxrrset.sample 0 in a 1.2.3.4" > update.out.test$n 2>&1
+$SAMPLEUPDATE -P 5300 -a 10.53.0.2 -p "yxrrset no-txt.sample TXT" \
+	add "check-yxrrset-nxrrset.sample 0 in a 1.2.3.4" > update.out.check$n 2>&1
+$DIG +tcp @10.53.0.1 -p 5300 a yxrrset-nxrrset.sample > dig.out.ns1.test$n
+$DIG +tcp @10.53.0.2 -p 5300 a yxrrset-nxrrset.sample > dig.out.ns2.test$n
+$DIG +tcp @10.53.0.2 -p 5300 a check-yxrrset-nxrrset.sample > check.out.ns2.test$n
+grep "update failed: NXRRSET" update.out.test$n > /dev/null || ret=1
+grep "update succeeded" update.out.check$n > /dev/null || ret=1
+grep "status: NXDOMAIN" dig.out.ns1.test$n > /dev/null || ret=1
+grep "status: NXDOMAIN" dig.out.ns2.test$n > /dev/null || ret=1
+grep "status: NOERROR" check.out.ns2.test$n > /dev/null || ret=1
+[ $ret = 0 ] || { echo I:failed; status=1; }
+
+#
+# End client library tests here
+#
+
 echo "I:exit status: $status"
 [ $status -eq 0 ] || exit 1

lib/dns/client.c

@@ -1978,8 +1978,21 @@ update_done(isc_task_t *task, isc_event_t *event) {
 	LOCK(&uctx->lock);
 	uctx->currentserver = ISC_LIST_NEXT(uctx->currentserver, link);
 	dns_request_destroy(&uctx->updatereq);
-	if (result != ISC_R_SUCCESS && !uctx->canceled &&
+	/*
-	    uctx->currentserver != NULL) {
+	 * Moving on to the next server shouldn't change the result
+	 * for NXDOMAIN, YXDOMAIN, NXRRSET and YXRRSET as they
+	 * indicate a prerequisite failure.  REFUSED should also
+	 * be consistent across all servers but often isn't as that
+	 * is policy rather that zone content driven (slaves that
+	 * aren't willing to forward should return NOTIMPL).  NOTZONE
+	 * indicates that we stuffed up the request construction so
+	 * don't retry.
+	 */
+	if (result != ISC_R_SUCCESS && result != DNS_R_NXDOMAIN &&
+	    result != DNS_R_YXDOMAIN && result != DNS_R_YXRRSET &&
+	    result != DNS_R_NXRRSET && result != DNS_R_NOTZONE &&
+	    !uctx->canceled && uctx->currentserver != NULL)
+	{
 		dns_message_renderreset(uctx->updatemsg);
 		dns_message_settsigkey(uctx->updatemsg, NULL);
 

lib/samples/sample-update.c

@@ -60,6 +60,8 @@ static const dns_rdataclass_t default_rdataclass = dns_rdataclass_in;
 static isc_bufferlist_t usedbuffers;
 static ISC_LIST(dns_rdatalist_t) usedrdatalists;
 
+static const char *port = "53";
+
 static void setup_tsec(char *keyfile, isc_mem_t *mctx);
 static void update_addordelete(isc_mem_t *mctx, char *cmdline,
 			       isc_boolean_t isdelete, dns_name_t *name);
@@ -80,20 +82,49 @@ usage(void) {
 	exit(1);
 }
 
+static isc_boolean_t
+addserver(const char *server, isc_sockaddrlist_t *list,
+	   isc_sockaddr_t *sockaddr)
+{
+	struct addrinfo hints, *res;
+	int gaierror;
+
+	memset(&hints, 0, sizeof(hints));
+	hints.ai_family = AF_UNSPEC;
+	hints.ai_socktype = SOCK_DGRAM;
+	hints.ai_protocol = IPPROTO_UDP;
+#ifdef AI_NUMERICHOST
+	hints.ai_flags |= AI_NUMERICHOST;
+#endif
+#ifdef AI_NUMERICSERV
+	hints.ai_flags |= AI_NUMERICSERV;
+#endif
+	gaierror = getaddrinfo(server, port, &hints, &res);
+	if (gaierror != 0) {
+		fprintf(stderr, "getaddrinfo(%s) failed: %s\n",
+			server, gai_strerror(gaierror));
+		return (ISC_FALSE);
+	}
+	INSIST(res->ai_addrlen <= sizeof(sockaddr->type));
+	memmove(&sockaddr->type, res->ai_addr, res->ai_addrlen);
+	freeaddrinfo(res);
+	sockaddr->length = (unsigned int)res->ai_addrlen;
+	ISC_LINK_INIT(sockaddr, link);
+	ISC_LIST_APPEND(*list, sockaddr, link);
+	return (ISC_TRUE);
+}
+
 int
 main(int argc, char *argv[]) {
 	int ch;
-	struct addrinfo hints, *res;
-	int gaierror;
 	dns_client_t *client = NULL;
 	char *zonenamestr = NULL;
 	char *keyfilename = NULL;
 	char *prereqstr = NULL;
-	isc_sockaddrlist_t auth_servers;
+	isc_sockaddr_t sa_auth[10], sa_recursive[10];
-	char *auth_server = NULL;
+	unsigned int nsa_auth = 0, nsa_recursive = 0;
-	char *recursive_server = NULL;
-	isc_sockaddr_t sa_auth, sa_recursive;
 	isc_sockaddrlist_t rec_servers;
+	isc_sockaddrlist_t auth_servers;
 	isc_result_t result;
 	isc_boolean_t isdelete;
 	isc_buffer_t b, *buf;
@@ -106,19 +137,32 @@ main(int argc, char *argv[]) {
 	dns_namelist_t updatelist, prereqlist, *prereqlistp = NULL;
 	isc_mem_t *umctx = NULL;
 
-	while ((ch = isc_commandline_parse(argc, argv, "a:k:p:r:z:")) != EOF) {
+	ISC_LIST_INIT(auth_servers);
+	ISC_LIST_INIT(rec_servers);
+
+	while ((ch = isc_commandline_parse(argc, argv, "a:k:p:P:r:z:")) != EOF) {
 		switch (ch) {
 		case 'k':
 			keyfilename = isc_commandline_argument;
 			break;
 		case 'a':
-			auth_server = isc_commandline_argument;
+			if (nsa_auth < sizeof(sa_auth)/sizeof(*sa_auth) &&
+			    addserver(isc_commandline_argument, &auth_servers,
+				      &sa_auth[nsa_auth]))
+				nsa_auth++;
 			break;
 		case 'p':
 			prereqstr = isc_commandline_argument;
 			break;
+		case 'P':
+			port = isc_commandline_argument;
+			break;
 		case 'r':
-			recursive_server = isc_commandline_argument;
+			if (nsa_recursive < 
+				sizeof(sa_recursive)/sizeof(*sa_recursive) &&
+			    addserver(isc_commandline_argument, &rec_servers,
+				      &sa_recursive[nsa_recursive]))
+				nsa_recursive++;
 			break;
 		case 'z':
 			zonenamestr = isc_commandline_argument;
@@ -143,8 +187,9 @@ main(int argc, char *argv[]) {
 		exit(1);
 	}
 
-	if (auth_server == NULL && recursive_server == NULL) {
+	if (ISC_LIST_HEAD(auth_servers) == NULL &&
-		fprintf(stderr, "authoritative or recursive server "
+	    ISC_LIST_HEAD(rec_servers) == NULL) {
+		fprintf(stderr, "authoritative or recursive servers "
 			"must be specified\n");
 		usage();
 	}
@@ -153,7 +198,6 @@ main(int argc, char *argv[]) {
 	ISC_LIST_INIT(usedbuffers);
 	ISC_LIST_INIT(usedrdatalists);
 	ISC_LIST_INIT(prereqlist);
-	ISC_LIST_INIT(auth_servers);
 	isc_lib_register();
 	result = dns_lib_init();
 	if (result != ISC_R_SUCCESS) {
@@ -172,60 +216,6 @@ main(int argc, char *argv[]) {
 		exit(1);
 	}
 
-	/* Set the authoritative server */
-	if (auth_server != NULL) {
-		memset(&hints, 0, sizeof(hints));
-		hints.ai_family = AF_UNSPEC;
-		hints.ai_socktype = SOCK_DGRAM;
-		hints.ai_protocol = IPPROTO_UDP;
-#ifdef AI_NUMERICHOST
-		hints.ai_flags = AI_NUMERICHOST;
-#endif
-		gaierror = getaddrinfo(auth_server, "53", &hints, &res);
-		if (gaierror != 0) {
-			fprintf(stderr, "getaddrinfo failed: %s\n",
-				gai_strerror(gaierror));
-			exit(1);
-		}
-		INSIST(res->ai_addrlen <= sizeof(sa_auth.type));
-		memmove(&sa_auth.type, res->ai_addr, res->ai_addrlen);
-		freeaddrinfo(res);
-		sa_auth.length = (unsigned int)res->ai_addrlen;
-		ISC_LINK_INIT(&sa_auth, link);
-
-		ISC_LIST_APPEND(auth_servers, &sa_auth, link);
-	}
-
-	/* Set the recursive server */
-	if (recursive_server != NULL) {
-		memset(&hints, 0, sizeof(hints));
-		hints.ai_family = AF_UNSPEC;
-		hints.ai_socktype = SOCK_DGRAM;
-		hints.ai_protocol = IPPROTO_UDP;
-#ifdef AI_NUMERICHOST
-		hints.ai_flags = AI_NUMERICHOST;
-#endif
-		gaierror = getaddrinfo(recursive_server, "53", &hints, &res);
-		if (gaierror != 0) {
-			fprintf(stderr, "getaddrinfo failed: %s\n",
-				gai_strerror(gaierror));
-			exit(1);
-		}
-		INSIST(res->ai_addrlen <= sizeof(sa_recursive.type));
-		memmove(&sa_recursive.type, res->ai_addr, res->ai_addrlen);
-		freeaddrinfo(res);
-		sa_recursive.length = (unsigned int)res->ai_addrlen;
-		ISC_LINK_INIT(&sa_recursive, link);
-		ISC_LIST_INIT(rec_servers);
-		ISC_LIST_APPEND(rec_servers, &sa_recursive, link);
-		result = dns_client_setservers(client, dns_rdataclass_in,
-					       NULL, &rec_servers);
-		if (result != ISC_R_SUCCESS) {
-			fprintf(stderr, "set server failed: %d\n", result);
-			exit(1);
-		}
-	}
-
 	/* Construct zone name */
 	zname = NULL;
 	if (zonenamestr != NULL) {
@@ -264,8 +254,8 @@ main(int argc, char *argv[]) {
 	result = dns_client_update(client,
 				   default_rdataclass, /* XXX: fixed */
 				   zname, prereqlistp, &updatelist,
-				   (auth_server == NULL) ? NULL :
+				   (ISC_LIST_HEAD(auth_servers) == NULL) ?
-				   &auth_servers, tsec, 0);
+				    NULL : &auth_servers, tsec, 0);
 	if (result != ISC_R_SUCCESS) {
 		fprintf(stderr,
 			"update failed: %s\n", dns_result_totext(result));