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 8618441c6a00c65c5eb940c39df8e4abe077f4f4..e55d4094d0e50b574e08da9a485a7e26cbcfa96c 100644 (file)
--- a/RELNOTES
+++ b/RELNOTES
@@ -76,6 +76,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 c17ef6f88290b02684df9e2caedea035d15c9a24..afe896fc9bbd5b113b017bfafae74fb07cb09a3d 100644 (file)
--- 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
@@ -3704,7 +3704,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;
                }
@@ -3724,7 +3724,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;
        }
        
@@ -3882,7 +3882,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;
                        }
@@ -4019,7 +4019,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;
 
@@ -4052,7 +4052,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 3992b6c4347347d84790955797d1d3c7fbe5305f..53e85a4f278cd76f8bcf34d12cc78c19fc37edc9 100644 (file)
--- 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 566e1e30cd03747ab5c96b18b5a6c1d61751a6c0..453be2f8cc1546fe823449034e434c30b3ed94a7 100644 (file)
--- 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
@@ -3525,13 +3525,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 d92a7ef1b4ae08690c9f9b7ee40cb55d2c9f1a44..1f1017fe1acbf14a934833e231221645ed1c8611 100644 (file)
--- a/server/ddns.c
+++ b/server/ddns.c
@@ -4,7 +4,7 @@
 
 /*
  * 
- * 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
  *
@@ -104,12 +104,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 {
@@ -1263,6 +1263,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:
@@ -1337,7 +1342,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;
                        }
@@ -1410,12 +1415,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:
@@ -1423,11 +1427,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:
@@ -1478,12 +1481,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);
                }
@@ -1528,7 +1531,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;
                        }
@@ -1568,7 +1571,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;
                }
@@ -1578,6 +1581,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 */
@@ -1588,7 +1595,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;
                        }
@@ -1642,10 +1649,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;
        }
 
@@ -1766,7 +1773,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);
@@ -1806,7 +1813,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);