Modify the code that determines if an outstanding DDNS request

should be cancelled.  This patch results in cancelling the
outstanding request less often.  It fixes the problem caused
by a client doing a release where  the  txt and ptr records
weren't removed from the DNS.
[ISC-BUGS #27858]

diff --git a/RELNOTES b/RELNOTES
index 9f52e7b38b8032b99f96bf8880e86f1adb97c9f7..b845c2be789e5bc2c8473c577c42c56dbafb69ee 100644 (file)
--- a/RELNOTES
+++ b/RELNOTES
@@ -79,7 +79,7 @@ work on other platforms. Please report any problems and suggested fixes to
 - 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].
+  [ISC-Bugs #26287].
 
 - Tidy up the receive calls and eliminate the need for found_pkt
   [ISC-Bugs #25066]
@@ -92,11 +92,18 @@ work on other platforms. Please report any problems and suggested fixes to
 - Add a compile time check for the presence of the noreturn attribute
   and use it for log_fatal if it's available.  This will help code
   checking programs to eliminate false positives.
-  [ISC-Bugs 27539]
+  [ISC-Bugs #27539]
 
 - Fixed many compilation problems ("set, but not used" warnings) for
   gcc 4.6 that may affect Ubuntu 11.10 users. [ISC-Bugs #27588]
 
+- Modify the code that determines if an outstanding DDNS request
+  should be cancelled.  This patch results in cancelling the
+  outstanding request less often.  It fixes the problem caused
+  by a client doing a release where  the  txt and ptr records
+  weren't removed from the DNS.
+  [ISC-BUGS #27858]
+
                        Changes since 4.2.2
 
 - Fix the code that checks for an existing DDNS transaction to cancel

diff --git a/includes/dhcpd.h b/includes/dhcpd.h
index 67b03b5adfbc618f0e5500b10c8e45c84a98d7a5..b7e8bc79b4451d7d3b7e21150a2ec647ea543f73 100644 (file)
--- a/includes/dhcpd.h
+++ b/includes/dhcpd.h
@@ -1932,7 +1932,8 @@ void parse_server_duid_conf(struct parse *cfile);
 /* ddns.c */
 int ddns_updates(struct packet *, struct lease *, struct lease *,
                 struct iasubopt *, struct iasubopt *, struct option_state *);
-int ddns_removals(struct lease *, struct iasubopt *, struct dhcp_ddns_cb *, isc_boolean_t);
+isc_result_t ddns_removals(struct lease *, struct iasubopt *,
+                          struct dhcp_ddns_cb *, isc_boolean_t);
 #if defined (TRACING)
 void trace_ddns_init(void);
 #endif
@@ -3225,7 +3226,10 @@ void make_binding_state_transition (struct lease *);
 int lease_copy (struct lease **, struct lease *, const char *, int);
 void release_lease (struct lease *, struct packet *);
 void abandon_lease (struct lease *, const char *);
+#if 0
+/* this appears to be unused and I plan to remove it SAR */
 void dissociate_lease (struct lease *);
+#endif
 void pool_timer (void *);
 int find_lease_by_uid (struct lease **, const unsigned char *,
                       unsigned, const char *, int);

diff --git a/server/ddns.c b/server/ddns.c
index cf2c044d0428d85e79461d063287f03ed7bde9ad..90217bd948312d53e1c137420c37620045197b3c 100644 (file)
--- a/server/ddns.c
+++ b/server/ddns.c
@@ -717,7 +717,7 @@ ddns_updates(struct packet *packet, struct lease *lease, struct lease *old,
        return result;
 }
 
-/*
+/*%<
  * Utility function to update text strings within a lease.
  *
  * The first issue is to find the proper scope.  Sometimes we shall be
@@ -727,6 +727,19 @@ ddns_updates(struct packet *packet, struct lease *lease, struct lease *old,
  * Lastly, if we needed to find the scope we write it out, if we used a
  * scope that was passed as an argument we don't write it, assuming that
  * our caller (or his ...) will do the write.
+ * 
+ *\li ddns_cb - the control block for the DDNS request
+ *
+ *\li inscope - a pointer to the scope to update.  This may be NULL
+ *    in which case we use the control block to find the lease and
+ *    then the scope.
+ *
+ * Returns
+ *\li ISC_R_SUCCESS
+ *
+ *\li ISC_R_FAILURE - The routine was unable to find an expected scope.
+ *    In some cases (static and inactive leases) we don't expect a scope
+ *    and return success.
  */
 
 isc_result_t
@@ -749,12 +762,11 @@ ddns_update_lease_text(dhcp_ddns_cb_t        *ddns_cb,
 
        /* 
         * If we are processing an expired or released v6 lease
-        * we don't actually have a scope to update
+        * or some types of v4 leases we don't actually have a
+        * scope to update
         */
-       if ((ddns_cb->address.len == 16) &&
-           ((ddns_cb->flags & DDNS_ACTIVE_LEASE) == 0)) {
+       if ((ddns_cb->flags & DDNS_ACTIVE_LEASE) == 0)
                return (ISC_R_SUCCESS);
-       }
 
        if (inscope != NULL) {
                scope = inscope;
@@ -1103,7 +1115,7 @@ ddns_update_lease_ptr(struct lease    *lease,
 
        /* 
         * If we are processing an expired or released v6 lease
-        * we don't actually have a scope to update
+        * we don't actually have a lease to update
         */
        if ((ddns_cb->address.len == 16) &&
            ((ddns_cb->flags & DDNS_ACTIVE_LEASE) == 0)) {
@@ -1618,22 +1630,31 @@ ddns_fwd_srv_rem1(dhcp_ddns_cb_t *ddns_cb,
        ddns_cb_free(ddns_cb, MDL);
 }
 
-
-/*
+/*%<
  * Remove relevant entries from DNS.
  *
- * Return values:
- * 0 - badness occurred and we weren't able to do what was wanted
- * 1 - we were able to do stuff but it's in progress
- * in both cases any additional block has been passed on to it's handler
+ * \li lease  - lease to start with if this is for v4
+ * 
+ * \li lease6 - lease to start with if this is for v6
+ * 
+ * \li add_ddns_cb - control block for additional DDNS work.  This
+ *     is used when the code is going to add a DDNS entry after removing
+ *     the current entry.
  * 
- * active == ISC_TRUE if the lease is still active, and FALSE if the lease
- * is inactive.  This is used to indicate if the lease is inactive or going
- * to inactive in IPv6 so we can avoid trying to update the lease with
- * cb pointers and text information.
+ * \li active - indication about the status of the lease. It is
+ *     ISC_TRUE if the lease is still active, and FALSE if the lease
+ *     is inactive.  This is used to indicate if the lease is inactive or going
+ *     to inactive so we can avoid trying to update the lease with cb pointers
+ *     and text information if it isn't useful.
+ * 
+ * Returns
+ * \li #ISC_R_FAILURE - badness occurred and we weren't able to do what was wanted
+ * \li #ISC_R_SUCCESS - we were able to do stuff but it's in progress
+ *
+ * in both cases any additional block has been passed on to it's handler
  */
 
-int
+isc_result_t
 ddns_removals(struct lease    *lease,
              struct iasubopt *lease6,
              dhcp_ddns_cb_t  *add_ddns_cb,
@@ -1641,22 +1662,92 @@ ddns_removals(struct lease    *lease,
 {
        isc_result_t rcode, execute_add = ISC_R_FAILURE;
        struct binding_scope **scope = NULL;
-       int result = 0;
+       isc_result_t result = ISC_R_FAILURE;
        dhcp_ddns_cb_t        *ddns_cb = NULL;
        struct data_string     leaseid;
 
        /*
-        * Cancel any outstanding requests.  When called
-        * from within the DNS code we probably will have
-        * already done the cancel but if called from outside
-        * - for example as part of a lease expiry - we won't.
+        * See if we need to cancel an outstanding request.  Mostly this is
+        * used to handle the case where this routine is called twice for
+        * the same release or abandon event.
+        * 
+        * When called from the dns code as part of an update request
+        * (add_ddns_cb != NULL) any outstanding requests will have already
+        * been cancelled.
+        * 
+        * If the new request is just a removal and we have an outstanding
+        * request we have several options:
+        *
+        * - we are doing an update or we are doing a removal and the active
+        * flag has changed from TRUE to FALSE.  In these cases we  need to
+        * cancel the old request and start the new one.
+        *
+        * - other wise we are doing a removal with the active flag unchanged.
+        * In this case we can let the current removal continue and do not need
+        * to start a new one.  If the old request included an update to be
+        * done after the removal we need to kill the update part of the
+        * request.
         */
-       if ((lease != NULL) && (lease->ddns_cb != NULL)) {
-               ddns_cancel(lease->ddns_cb, MDL);
-               lease->ddns_cb = NULL;
-       } else if ((lease6 != NULL) && (lease6->ddns_cb != NULL)) {
-               ddns_cancel(lease6->ddns_cb, MDL);
-               lease6->ddns_cb = NULL;
+       
+       if (add_ddns_cb == NULL) {
+               if ((lease != NULL) && (lease->ddns_cb != NULL)) {
+                       ddns_cb = lease->ddns_cb;
+
+                       /*
+                        * Is the old request an update or did the 
+                        * the active flag change?
+                        */
+                       if (((ddns_cb->state == DDNS_STATE_ADD_PTR) ||
+                            (ddns_cb->state == DDNS_STATE_ADD_FW_NXDOMAIN) ||
+                            (ddns_cb->state == DDNS_STATE_ADD_FW_YXDHCID)) ||
+                           ((active == ISC_FALSE) &&
+                            ((ddns_cb->flags & DDNS_ACTIVE_LEASE) != 0))) {
+                               /* Cancel the current request */
+                               ddns_cancel(lease->ddns_cb, MDL);
+                               lease->ddns_cb = NULL;
+                       } else {
+                               /* Remvoval, check and remove updates */
+                               if (ddns_cb->next_op != NULL) {
+                                       ddns_cb_free(ddns_cb->next_op, MDL);
+                                       ddns_cb->next_op = NULL;
+                               }
+#if defined (DEBUG_DNS_UPDATES)
+                               log_info("DDNS %s(%d): removal already in "
+                                        "progress new ddns_cb=%p",
+                                        MDL, ddns_cb);
+#endif
+                               return (ISC_R_SUCCESS);
+                       }
+               } else if ((lease6 != NULL) && (lease6->ddns_cb != NULL)) {
+                       ddns_cb = lease6->ddns_cb;
+
+                       /*
+                        * Is the old request an update or did the 
+                        * the active flag change?
+                        */
+                       if (((ddns_cb->state == DDNS_STATE_ADD_PTR) ||
+                            (ddns_cb->state == DDNS_STATE_ADD_FW_NXDOMAIN) ||
+                            (ddns_cb->state == DDNS_STATE_ADD_FW_YXDHCID)) ||
+                           ((active == ISC_FALSE) &&
+                            ((ddns_cb->flags & DDNS_ACTIVE_LEASE) != 0))) {
+                               /* Cancel the current request */
+                               ddns_cancel(lease6->ddns_cb, MDL);
+                               lease6->ddns_cb = NULL;
+                       } else {
+                               /* Remvoval, check and remove updates */
+                               if (ddns_cb->next_op != NULL) {
+                                       ddns_cb_free(ddns_cb->next_op, MDL);
+                                       ddns_cb->next_op = NULL;
+                               }
+#if defined (DEBUG_DNS_UPDATES)
+                               log_info("DDNS %s(%d): removal already in "
+                                        "progress new ddns_cb=%p",
+                                        MDL, ddns_cb);
+#endif
+                               return (ISC_R_SUCCESS);
+                       }
+               }
+               ddns_cb = NULL;
        }
 
        /* allocate our control block */
@@ -1684,8 +1775,9 @@ ddns_removals(struct lease    *lease,
 
        /*
         * Set the flag bit if the lease is active, that is it isn't
-        * expired or released.  This is used in the IPv6 paths to
-        * determine if we need to update the lease when the response
+        * expired or released.  This is used to determine if we need
+        * to update the scope information for both v4 and v6 and
+        * the lease information for v6 when the response
         * from the DNS code is processed.
         */
        if (active == ISC_TRUE) {
@@ -1780,7 +1872,7 @@ ddns_removals(struct lease    *lease,
                        if (rcode == ISC_R_SUCCESS) {
                                ddns_update_lease_ptr(lease, lease6, ddns_cb,
                                                      ddns_cb, MDL);
-                               return(1);
+                               return (ISC_R_SUCCESS);
                        }
 
                        /*
@@ -1813,14 +1905,14 @@ ddns_removals(struct lease    *lease,
                        add_ddns_cb = NULL;
                }
                else {
-                       result = 1;
+                       result = ISC_R_SUCCESS;
                }
 
                rcode = ddns_modify_ptr(ddns_cb, MDL);
                if (rcode == ISC_R_SUCCESS) {
                        ddns_update_lease_ptr(lease, lease6, ddns_cb, ddns_cb,
                                              MDL);
-                       return(result);
+                       return (result);
                }
 
                /* We weren't able to process the request tag the
@@ -1840,7 +1932,7 @@ ddns_removals(struct lease    *lease,
        if (ddns_cb != NULL) 
                ddns_cb_free(ddns_cb, MDL);
 
-       return(result);
+       return (result);
 }
 
 #endif /* NSUPDATE */

diff --git a/server/failover.c b/server/failover.c
index 8d7fe7896e5e0af0e2e50a1f98318b8ae0318cd2..45e6b62963c262299c4a6ffa066f416aa9ab3cce 100644 (file)
--- a/server/failover.c
+++ b/server/failover.c
@@ -5222,7 +5222,7 @@ isc_result_t dhcp_failover_process_bind_update (dhcp_failover_state_t *state,
         */
        if (msg->binding_status == FTS_ACTIVE &&
            (chaddr_changed || ident_changed)) {
-               ddns_removals(lease, NULL, NULL, ISC_FALSE);
+               (void) ddns_removals(lease, NULL, NULL, ISC_FALSE);
 
                if (lease->scope != NULL)
                        binding_scope_dereference(&lease->scope, MDL);

diff --git a/server/mdb.c b/server/mdb.c
index 5f89d63af87fd625d2e2cccfa51e58031ac6682d..3867ba58272d564309063b81ace6f0346258d8a1 100644 (file)
--- a/server/mdb.c
+++ b/server/mdb.c
@@ -1446,7 +1446,7 @@ void make_binding_state_transition (struct lease *lease)
              lease -> binding_state == FTS_ACTIVE &&
              lease -> next_binding_state != FTS_RELEASED))) {
 #if defined (NSUPDATE)
-               ddns_removals(lease, NULL, NULL, ISC_FALSE);
+               (void) ddns_removals(lease, NULL, NULL, ISC_TRUE);
 #endif
                if (lease -> on_expiry) {
                        execute_statements ((struct binding_value **)0,
@@ -1512,7 +1512,7 @@ void make_binding_state_transition (struct lease *lease)
                 * release message.  This is not true of expiry, where the
                 * peer may have extended the lease.
                 */
-               ddns_removals(lease, NULL, NULL, ISC_FALSE);
+               (void) ddns_removals(lease, NULL, NULL, ISC_TRUE);
 #endif
                if (lease -> on_release) {
                        execute_statements ((struct binding_value **)0,
@@ -1681,7 +1681,7 @@ void release_lease (lease, packet)
        /* If there are statements to execute when the lease is
           released, execute them. */
 #if defined (NSUPDATE)
-       ddns_removals(lease, NULL, NULL, ISC_FALSE);
+       (void) ddns_removals(lease, NULL, NULL, ISC_FALSE);
 #endif
        if (lease -> on_release) {
                execute_statements ((struct binding_value **)0,
@@ -1755,7 +1755,7 @@ void abandon_lease (lease, message)
 {
        struct lease *lt = (struct lease *)0;
 #if defined (NSUPDATE)
-       ddns_removals(lease, NULL, NULL, ISC_FALSE);
+       (void) ddns_removals(lease, NULL, NULL, ISC_FALSE);
 #endif
 
        if (!lease_copy (&lt, lease, MDL))
@@ -1779,6 +1779,14 @@ void abandon_lease (lease, message)
        lease_dereference (&lt, MDL);
 }
 
+#if 0
+/*
+ * This doesn't appear to be in use for anything anymore.
+ * I'm ifdeffing it now and if there are no complaints in
+ * the future it will be removed.
+ * SAR
+ */
+
 /* Abandon the specified lease (set its timeout to infinity and its
    particulars to zero, and re-hash it as appropriate. */
 
@@ -1787,7 +1795,7 @@ void dissociate_lease (lease)
 {
        struct lease *lt = (struct lease *)0;
 #if defined (NSUPDATE)
-       ddns_removals(lease, NULL, NULL, ISC_FALSE);
+       (void) ddns_removals(lease, NULL, NULL, ISC_FALSE);
 #endif
 
        if (!lease_copy (&lt, lease, MDL))
@@ -1812,6 +1820,7 @@ void dissociate_lease (lease)
        supersede_lease (lease, lt, 1, 1, 1);
        lease_dereference (&lt, MDL);
 }
+#endif
 
 /* Timer called when a lease in a particular pool expires. */
 void pool_timer (vpool)

diff --git a/server/mdb6.c b/server/mdb6.c
index d08016afa7b01753f2bd7aaed4ad23372d153c10..a993659465de25175bdfb6516131182a86e75807 100644 (file)
--- a/server/mdb6.c
+++ b/server/mdb6.c
@@ -1057,7 +1057,7 @@ move_lease_to_inactive(struct ipv6_pool *pool, struct iasubopt *lease,
 #if defined (NSUPDATE)
                /* Process events upon expiration. */
                if (pool->pool_type != D6O_IA_PD) {
-                       ddns_removals(NULL, lease, NULL, ISC_FALSE);
+                       (void) ddns_removals(NULL, lease, NULL, ISC_FALSE);
                }
 #endif