From: Shawn Routhier <sar@isc.org>
Date: Fri, 3 Feb 2012 22:46:43 +0000 (+0000)
Subject: In the DDNS code handle error conditions more gracefully and add more
X-Git-Tag: v4_2_4b1~32
X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=c248e5e9264ababa7e813102ba64efc260c81d67;p=thirdparty%2Fdhcp.git

In the DDNS code handle error conditions more gracefully and add more
logging code.  The major change is to handle unexpected cancel events
from the DNS client code.
[ISC-Bugs 26287].
---

diff --git a/RELNOTES b/RELNOTES
index 48071dd9c..8e3c09637 100644
--- a/RELNOTES
+++ b/RELNOTES
@@ -66,6 +66,11 @@ work on other platforms. Please report any problems and suggested fixes to
   the server being out of addresses in pools with particular ranges.
   [ISC-Bugs #26498]
 
+- In the DDNS code handle error conditions more gracefully and add more
+  logging code.  The major change is to handle unexpected cancel events
+  from the DNS client code.
+  [ISC-Bugs 26287].
+
 			Changes since 4.2.2
 
 - Fix the code that checks for an existing DDNS transaction to cancel
diff --git a/client/dhclient.c b/client/dhclient.c
index 48707d1d3..38534dc3d 100644
--- a/client/dhclient.c
+++ b/client/dhclient.c
@@ -3,7 +3,7 @@
    DHCP Client. */
 
 /*
- * Copyright (c) 2004-2011 by Internet Systems Consortium, Inc. ("ISC")
+ * Copyright (c) 2004-2012 by Internet Systems Consortium, Inc. ("ISC")
  * Copyright (c) 1995-2003 by Internet Software Consortium
  *
  * Permission to use, copy, modify, and distribute this software for any
@@ -3706,7 +3706,7 @@ client_dns_remove_action(dhcp_ddns_cb_t *ddns_cb,
 		/* Do the second stage of the FWD removal */
 		ddns_cb->state = DDNS_STATE_REM_FW_NXRR;
 
-		result = ddns_modify_fwd(ddns_cb);
+		result = ddns_modify_fwd(ddns_cb, MDL);
 		if (result == ISC_R_SUCCESS) {
 			return;
 		}
@@ -3726,7 +3726,7 @@ client_dns_remove(struct client_state *client,
 
 	/* if we have an old ddns request for this client, cancel it */
 	if (client->ddns_cb != NULL) {
-		ddns_cancel(client->ddns_cb);
+		ddns_cancel(client->ddns_cb, MDL);
 		client->ddns_cb = NULL;
 	}
 	
@@ -3884,7 +3884,7 @@ client_dns_update_action(dhcp_ddns_cb_t *ddns_cb,
 			ddns_cb->state = DDNS_STATE_ADD_FW_YXDHCID;
 			ddns_cb->cur_func = client_dns_update_action;
 
-			result = ddns_modify_fwd(ddns_cb);
+			result = ddns_modify_fwd(ddns_cb, MDL);
 			if (result == ISC_R_SUCCESS) {
 				return;
 			}
@@ -4021,7 +4021,7 @@ client_dns_update(struct client_state *client, dhcp_ddns_cb_t *ddns_cb)
 	 * Perform updates.
 	 */
 	if (ddns_cb->fwd_name.len && ddns_cb->dhcid.len) {
-		rcode = ddns_modify_fwd(ddns_cb);
+		rcode = ddns_modify_fwd(ddns_cb, MDL);
 	} else
 		rcode = ISC_R_FAILURE;
 
@@ -4054,7 +4054,7 @@ dhclient_schedule_updates(struct client_state *client,
 
 	/* cancel any outstanding ddns requests */
 	if (client->ddns_cb != NULL) {
-		ddns_cancel(client->ddns_cb);
+		ddns_cancel(client->ddns_cb, MDL);
 		client->ddns_cb = NULL;
 	}
 
diff --git a/common/dns.c b/common/dns.c
index 3992b6c43..53e85a4f2 100644
--- a/common/dns.c
+++ b/common/dns.c
@@ -3,7 +3,7 @@
    Domain Name Service subroutines. */
 
 /*
- * Copyright (c) 2009-2011 by Internet Systems Consortium, Inc. ("ISC")
+ * Copyright (c) 2009-2012 by Internet Systems Consortium, Inc. ("ISC")
  * Copyright (c) 2004-2007 by Internet Systems Consortium, Inc. ("ISC")
  * Copyright (c) 2001-2003 by Internet Software Consortium
  *
@@ -813,6 +813,12 @@ void repudiate_zone (struct dns_zone **zone)
 	   XXX zones.   This isn't a big problem since we're not yet
 	   XXX caching zones... :'} */
 
+	/* verify that we have a pointer at least */
+	if ((zone == NULL) || (*zone == NULL)) {
+		log_info("Null argument to repudiate zone");
+		return;
+	}
+
 	(*zone) -> timeout = cur_time - 1;
 	dns_zone_dereference (zone, MDL);
 }
@@ -1319,6 +1325,24 @@ void ddns_interlude(isc_task_t  *taskp,
 	 * need to clean up. */
 	if ((eresult == ISC_R_CANCELED) ||
 	    ((ddns_cb->flags & DDNS_ABORT) != 0)) {
+#if defined (DEBUG_DNS_UPDATES)
+		log_info("DDNS: completeing transaction cancellation cb=%p, "
+			 "flags=%x, %s",
+			 ddns_cb, ddns_cb->flags, isc_result_totext(eresult));
+#endif
+		if ((ddns_cb->flags & DDNS_ABORT) == 0) {
+			log_info("DDNS: cleaning up lease pointer for a cancel "
+				 "cb=%p", ddns_cb);
+			/* 
+			 * We shouldn't actually be able to get here but
+			 * we are.  This means we haven't cleaned up
+			 * the lease pointer so we need to do that before
+			 * freeing the cb.  
+			 */
+			ddns_cb->cur_func(ddns_cb, eresult);
+			return;
+		}
+
 		if (ddns_cb->next_op != NULL) {
 			/* if necessary cleanup up next op block */
 			ddns_cb_free(ddns_cb->next_op, MDL);
@@ -1333,6 +1357,8 @@ void ddns_interlude(isc_task_t  *taskp,
 		int i;
 		/* Our zone information was questionable,
 		 * repudiate it and try again */
+		log_error("DDNS: bad zone information, repudiating zone %s",
+			  ddns_cb->zone_name);
 		repudiate_zone(&ddns_cb->zone);
 		ddns_cb->zone_name[0]    = 0;
 		ISC_LIST_INIT(ddns_cb->zone_server_list);
@@ -1340,20 +1366,18 @@ void ddns_interlude(isc_task_t  *taskp,
 			ISC_LINK_INIT(&ddns_cb->zone_addrs[i], link);
 		}
 
-		if ((ddns_cb->state &
-		     (DDNS_STATE_ADD_PTR | DDNS_STATE_REM_PTR)) != 0) {
-			result = ddns_modify_ptr(ddns_cb);
+		if ((ddns_cb->state == DDNS_STATE_ADD_PTR) ||
+		    (ddns_cb->state == DDNS_STATE_REM_PTR)) {
+			result = ddns_modify_ptr(ddns_cb, MDL);
 		} else {
-			result = ddns_modify_fwd(ddns_cb);
+			result = ddns_modify_fwd(ddns_cb, MDL);
 		}
 
 		if (result != ISC_R_SUCCESS) {
-			/* if we couldn't redo the query toss it */
-			if (ddns_cb->next_op != NULL) {
-				/* cleanup up next op block */
-				ddns_cb_free(ddns_cb->next_op, MDL);
-				}
-			ddns_cb_free(ddns_cb, MDL);
+			/* if we couldn't redo the query log it and
+			 * let the next function clean it up */
+			log_info("DDNS: Failed to retry after zone failure");
+			ddns_cb->cur_func(ddns_cb, result);
 		}
 		return;
 	} else {
@@ -1371,7 +1395,7 @@ void ddns_interlude(isc_task_t  *taskp,
  */
 
 isc_result_t
-ddns_modify_fwd(dhcp_ddns_cb_t *ddns_cb)
+ddns_modify_fwd(dhcp_ddns_cb_t *ddns_cb, const char *file, int line)
 {
 	isc_result_t result;
 	dns_tsec_t *tsec_key = NULL;
@@ -1541,6 +1565,13 @@ ddns_modify_fwd(dhcp_ddns_cb_t *ddns_cb)
 #endif
 
  cleanup:
+#if defined (DEBUG_DNS_UPDATES)
+	if (result != ISC_R_SUCCESS) {
+		log_info("DDNS: %s(%d): error in ddns_modify_fwd %s for %p",
+			 file, line, isc_result_totext(result), ddns_cb);
+	}
+#endif
+
 	if (dataspace != NULL) {
 		isc_mem_put(dhcp_gbl_ctx.mctx, dataspace,
 			    sizeof(*dataspace) * 4);
@@ -1550,7 +1581,7 @@ ddns_modify_fwd(dhcp_ddns_cb_t *ddns_cb)
 
 
 isc_result_t
-ddns_modify_ptr(dhcp_ddns_cb_t *ddns_cb)
+ddns_modify_ptr(dhcp_ddns_cb_t *ddns_cb, const char *file, int line)
 {
 	isc_result_t result;
 	dns_tsec_t *tsec_key  = NULL;
@@ -1730,6 +1761,13 @@ ddns_modify_ptr(dhcp_ddns_cb_t *ddns_cb)
 #endif
 
  cleanup:
+#if defined (DEBUG_DNS_UPDATES)
+	if (result != ISC_R_SUCCESS) {
+		log_info("DDNS: %s(%d): error in ddns_modify_ptr %s for %p",
+			 file, line, isc_result_totext(result), ddns_cb);
+	}
+#endif
+
 	if (dataspace != NULL) {
 		isc_mem_put(dhcp_gbl_ctx.mctx, dataspace,
 			    sizeof(*dataspace) * 2);
@@ -1738,13 +1776,18 @@ ddns_modify_ptr(dhcp_ddns_cb_t *ddns_cb)
 }
 
 void
-ddns_cancel(dhcp_ddns_cb_t *ddns_cb) {
+ddns_cancel(dhcp_ddns_cb_t *ddns_cb, const char *file, int line) {
 	ddns_cb->flags |= DDNS_ABORT;
 	if (ddns_cb->transaction != NULL) {
 		dns_client_cancelupdate((dns_clientupdatetrans_t *)
 					ddns_cb->transaction);
 	}
 	ddns_cb->lease = NULL;
+
+#if defined (DEBUG_DNS_UPDATES)
+	log_info("DDNS: %s(%d): cancelling transaction for  %p",
+		 file, line,  ddns_cb);
+#endif
 }
 
 #endif /* NSUPDATE */
diff --git a/includes/dhcpd.h b/includes/dhcpd.h
index a7bb8c28c..f0a69e98e 100644
--- a/includes/dhcpd.h
+++ b/includes/dhcpd.h
@@ -3,7 +3,7 @@
    Definitions for dhcpd... */
 
 /*
- * Copyright (c) 2004-2011 by Internet Systems Consortium, Inc. ("ISC")
+ * Copyright (c) 2004-2012 by Internet Systems Consortium, Inc. ("ISC")
  * Copyright (c) 1996-2003 by Internet Software Consortium
  *
  * Permission to use, copy, modify, and distribute this software for any
@@ -3543,13 +3543,13 @@ void ddns_cb_forget_zone (dhcp_ddns_cb_t *ddns_cb);
 //void *key_from_zone(struct dns_zone *zone);
 
 isc_result_t
-ddns_modify_fwd(dhcp_ddns_cb_t *ddns_cb);
+ddns_modify_fwd(dhcp_ddns_cb_t *ddns_cb, const char *file, int line);
 
 isc_result_t
-ddns_modify_ptr(dhcp_ddns_cb_t *ddns_cb);
+ddns_modify_ptr(dhcp_ddns_cb_t *ddns_cb, const char *file, int line);
 
 void
-ddns_cancel(dhcp_ddns_cb_t *ddns_cb);
+ddns_cancel(dhcp_ddns_cb_t *ddns_cb, const char *file, int line);
 
 #define MAX_ADDRESS_STRING_LEN \
    (sizeof("ffff:ffff:ffff:ffff:ffff:ffff:255.255.255.255"))
diff --git a/server/ddns.c b/server/ddns.c
index 8cdbc3bd5..beccff8d5 100644
--- a/server/ddns.c
+++ b/server/ddns.c
@@ -3,7 +3,7 @@
    Dynamic DNS updates. */
 
 /*
- * Copyright (c) 2009-2011 by Internet Systems Consortium, Inc. ("ISC")
+ * Copyright (c) 2009-2012 by Internet Systems Consortium, Inc. ("ISC")
  * Copyright (c) 2004-2007 by Internet Systems Consortium, Inc. ("ISC")
  * Copyright (c) 2000-2003 by Internet Software Consortium
  *
@@ -103,12 +103,12 @@ ddns_updates(struct packet *packet, struct lease *lease, struct lease *old,
 
 	if (lease != NULL) {
 		if ((old != NULL) && (old->ddns_cb != NULL)) {
-			ddns_cancel(old->ddns_cb);
+			ddns_cancel(old->ddns_cb, MDL);
 			old->ddns_cb = NULL;
 		}
 	} else if (lease6 != NULL) {
 		if ((old6 != NULL) && (old6->ddns_cb != NULL)) {
-			ddns_cancel(old6->ddns_cb);
+			ddns_cancel(old6->ddns_cb, MDL);
 			old6->ddns_cb = NULL;
 		}
 	} else {
@@ -1262,6 +1262,11 @@ ddns_ptr_remove(dhcp_ddns_cb_t *ddns_cb,
 
 		/* trigger any add operation */
 		result = ISC_R_SUCCESS;
+#if defined (DEBUG_DNS_UPDATES)
+		log_info("DDNS: removed map or no reverse map to remove %.*s",
+			 (int)ddns_cb->rev_name.len,
+			 (const char *)ddns_cb->rev_name.data);
+#endif
 		break;
 
 	default:
@@ -1336,7 +1341,7 @@ ddns_fwd_srv_add2(dhcp_ddns_cb_t *ddns_cb,
 			ddns_cb->state = DDNS_STATE_ADD_PTR;
 			ddns_cb->cur_func = ddns_ptr_add;
 			
-			result = ddns_modify_ptr(ddns_cb);
+			result = ddns_modify_ptr(ddns_cb, MDL);
 			if (result == ISC_R_SUCCESS) {
 				return;
 			}
@@ -1409,12 +1414,11 @@ ddns_fwd_srv_add1(dhcp_ddns_cb_t *ddns_cb,
 			ddns_cb->state = DDNS_STATE_ADD_PTR;
 			ddns_cb->cur_func = ddns_ptr_add;
 			
-			result = ddns_modify_ptr(ddns_cb);
+			result = ddns_modify_ptr(ddns_cb, MDL);
 			if (result == ISC_R_SUCCESS) {
 				return;
 			}
 		}
-			
 		break;
 
 	case DNS_R_YXDOMAIN:
@@ -1422,11 +1426,10 @@ ddns_fwd_srv_add1(dhcp_ddns_cb_t *ddns_cb,
 		ddns_cb->state = DDNS_STATE_ADD_FW_YXDHCID;
 		ddns_cb->cur_func = ddns_fwd_srv_add2;
 			
-		result = ddns_modify_fwd(ddns_cb);
+		result = ddns_modify_fwd(ddns_cb, MDL);
 		if (result == ISC_R_SUCCESS) {
 			return;
 		}
-
 		break;
 
 	default:
@@ -1477,12 +1480,12 @@ ddns_fwd_srv_connector(struct lease          *lease,
 		if (ddns_cb->flags & DDNS_UPDATE_ADDR) {
 			ddns_cb->state    = DDNS_STATE_ADD_FW_NXDOMAIN;
 			ddns_cb->cur_func = ddns_fwd_srv_add1;
-			result = ddns_modify_fwd(ddns_cb);
+			result = ddns_modify_fwd(ddns_cb, MDL);
 		} else if ((ddns_cb->flags & DDNS_UPDATE_PTR) &&
 			 (ddns_cb->rev_name.len != 0)) {
 			ddns_cb->state    = DDNS_STATE_ADD_PTR;
 			ddns_cb->cur_func = ddns_ptr_add;
-			result = ddns_modify_ptr(ddns_cb);
+			result = ddns_modify_ptr(ddns_cb, MDL);
 		} else {
 			ddns_update_lease_text(ddns_cb, inscope);
 		}
@@ -1527,7 +1530,7 @@ ddns_fwd_srv_rem2(dhcp_ddns_cb_t *ddns_cb,
 			ddns_cb->state = DDNS_STATE_REM_PTR;
 			ddns_cb->cur_func = ddns_ptr_remove;
 			
-			eresult = ddns_modify_ptr(ddns_cb);
+			eresult = ddns_modify_ptr(ddns_cb, MDL);
 			if (eresult == ISC_R_SUCCESS) {
 				return;
 			}
@@ -1567,7 +1570,7 @@ ddns_fwd_srv_rem1(dhcp_ddns_cb_t *ddns_cb,
 		/* Do the second step of the FWD removal */
 		ddns_cb->state    = DDNS_STATE_REM_FW_NXRR;
 		ddns_cb->cur_func = ddns_fwd_srv_rem2;
-		result = ddns_modify_fwd(ddns_cb);
+		result = ddns_modify_fwd(ddns_cb, MDL);
 		if (result == ISC_R_SUCCESS) {
 			return;
 		}
@@ -1577,6 +1580,10 @@ ddns_fwd_srv_rem1(dhcp_ddns_cb_t *ddns_cb,
 	case DNS_R_NXDOMAIN:
 		ddns_update_lease_text(ddns_cb, NULL);
 
+#if defined (DEBUG_DNS_UPDATES)
+		log_info("DDNS: no forward map to remove. %p", ddns_cb);
+#endif
+
 		/* Do the next operation */
 		if ((ddns_cb->flags & DDNS_UPDATE_PTR) != 0) {
 			/* if we have zone information get rid of it */
@@ -1587,7 +1594,7 @@ ddns_fwd_srv_rem1(dhcp_ddns_cb_t *ddns_cb,
 			ddns_cb->state    = DDNS_STATE_REM_PTR;
 			ddns_cb->cur_func = ddns_ptr_remove;
 			
-			result = ddns_modify_ptr(ddns_cb);
+			result = ddns_modify_ptr(ddns_cb, MDL);
 			if (result == ISC_R_SUCCESS) {
 				return;
 			}
@@ -1641,10 +1648,10 @@ ddns_removals(struct lease    *lease,
 	 * - for example as part of a lease expiry - we won't.
 	 */
 	if ((lease != NULL) && (lease->ddns_cb != NULL)) {
-		ddns_cancel(lease->ddns_cb);
+		ddns_cancel(lease->ddns_cb, MDL);
 		lease->ddns_cb = NULL;
 	} else if ((lease6 != NULL) && (lease6->ddns_cb != NULL)) {
-		ddns_cancel(lease6->ddns_cb);
+		ddns_cancel(lease6->ddns_cb, MDL);
 		lease6->ddns_cb = NULL;
 	}
 
@@ -1765,7 +1772,7 @@ ddns_removals(struct lease    *lease,
 			ddns_cb->state    = DDNS_STATE_REM_FW_YXDHCID;
 			ddns_cb->cur_func = ddns_fwd_srv_rem1;
 
-			rcode = ddns_modify_fwd(ddns_cb);
+			rcode = ddns_modify_fwd(ddns_cb, MDL);
 			if (rcode == ISC_R_SUCCESS) {
 				ddns_update_lease_ptr(lease, lease6, ddns_cb,
 						      ddns_cb, MDL);
@@ -1805,7 +1812,7 @@ ddns_removals(struct lease    *lease,
 			result = 1;
 		}
 
-		rcode = ddns_modify_ptr(ddns_cb);
+		rcode = ddns_modify_ptr(ddns_cb, MDL);
 		if (rcode == ISC_R_SUCCESS) {
 			ddns_update_lease_ptr(lease, lease6, ddns_cb, ddns_cb,
 					      MDL);