- Secondary servers in a failover pair will now perform ddns removals if

  they had performed ddns updates on a lease that is expiring, or was
  released through the primary.  As part of the same fix, stale binding scopes
  will now be removed if a change in identity of a lease's active client is
  detected, rather than simply if a lease is noticed to have expired (which it
  may have expired without a failover server noticing in some situations).
  [ISC-Bugs #19826b]

3 files changed, 70 insertions, 3 deletions

diff --git a/RELNOTES b/RELNOTES
index 786011ba..6488aae0 100644
--- a/RELNOTES
+++ b/RELNOTES
@@ -78,6 +78,13 @@ suggested fixes to <dhcp-users@isc.org>.
 - Don't look for IPv6 interfaces on Linux when running in DHCPv4 mode.
   Thanks to patches from Matthew Newton and David Cantrell.
 
+- Secondary servers in a failover pair will now perform ddns removals if
+  they had performed ddns updates on a lease that is expiring, or was
+  released through the primary.  As part of the same fix, stale binding scopes
+  will now be removed if a change in identity of a lease's active client is
+  detected, rather than simply if a lease is noticed to have expired (which it
+  may have expired without a failover server noticing in some situations).
+
 			Changes since 4.0.1
 
 - Remove infinite loop in token_print_indent_concat().
diff --git a/server/failover.c b/server/failover.c
index 4384f538..11fff7c8 100644
--- a/server/failover.c
+++ b/server/failover.c
@@ -4931,6 +4931,8 @@ isc_result_t dhcp_failover_process_bind_update (dhcp_failover_state_t *state,
 	int new_binding_state;
 	int send_to_backup = 0;
 	int required_options;
+	isc_boolean_t chaddr_changed = ISC_FALSE;
+	isc_boolean_t ident_changed = ISC_FALSE;
 
 	/* Validate the binding update. */
 	required_options = FTB_ASSIGNED_IP_ADDRESS | FTB_BINDING_STATUS;
@@ -5000,6 +5002,12 @@ isc_result_t dhcp_failover_process_bind_update (dhcp_failover_state_t *state,
 			message = "chaddr too long";
 			goto bad;
 		}
+
+		if ((lt->hardware_addr.hlen != msg->chaddr.count) ||
+		    (memcmp(lt->hardware_addr.hbuf, msg->chaddr.data,
+			    msg->chaddr.count) != 0))
+			chaddr_changed = ISC_TRUE;
+
 		lt -> hardware_addr.hlen = msg -> chaddr.count;
 		memcpy (lt -> hardware_addr.hbuf, msg -> chaddr.data,
 			msg -> chaddr.count);
@@ -5010,6 +5018,7 @@ isc_result_t dhcp_failover_process_bind_update (dhcp_failover_state_t *state,
 		reason = FTR_MISSING_BINDINFO;
 		goto bad;
 	} else if (msg->binding_status == FTS_ABANDONED) {
+		chaddr_changed = ISC_TRUE;
 		lt->hardware_addr.hlen = 0;
 		if (lt->scope)
 			binding_scope_dereference(&lt->scope, MDL);
@@ -5025,6 +5034,12 @@ isc_result_t dhcp_failover_process_bind_update (dhcp_failover_state_t *state,
 			goto bad;
 		}
 
+		if ((lt->uid_len != msg->client_identifier.count) ||
+		    (lt->uid == NULL) || /* Sanity; should never happen. */
+		    (memcmp(lt->uid, msg->client_identifier.data,
+			    lt->uid_len) != 0))
+			ident_changed = ISC_TRUE;
+
 		lt->uid_len = msg->client_identifier.count;
 
 		/* Allocate the lt->uid buffer if we haven't already, or
@@ -5053,15 +5068,45 @@ isc_result_t dhcp_failover_process_bind_update (dhcp_failover_state_t *state,
 	} else if (lt->uid && msg->binding_status != FTS_RESET &&
 		   msg->binding_status != FTS_FREE &&
 		   msg->binding_status != FTS_BACKUP) {
+		ident_changed = ISC_TRUE;
 		if (lt->uid != lt->uid_buf)
 			dfree (lt->uid, MDL);
 		lt->uid = NULL;
 		lt->uid_max = lt->uid_len = 0;
 	}
 
-	/* If the lease was expired, also remove the stale binding scope. */
-	if (lt->scope && lt->ends < cur_time)
-		binding_scope_dereference(&lt->scope, MDL);
+	/*
+	 * A server's configuration can assign a 'binding scope';
+	 *
+	 *	set var = "value";
+	 *
+	 * The problem with these binding scopes is that they are refreshed
+	 * when the server processes a client's DHCP packet.  A local binding
+	 * scope is trash, then, when the lease has been assigned by the
+	 * partner server.  There is no real way to detect this, a peer may
+	 * be updating us (as through potential conflict) with a binding we
+	 * sent them, but we can trivially detect the /problematic/ case;
+	 *
+	 *	lease is free.
+	 *	primary allocates lease to client A, assigns ddns name A.
+	 *	primary fails.
+	 *	secondary enters partner down.
+	 *	lease expires, and is set free.
+	 *	lease is allocated to client B and given ddns name B.
+	 *	primary recovers.
+	 *
+	 * The binding update in this case will be active->active, but the
+	 * client identification on the lease will have changed.  The ddns
+	 * update on client A will have leaked if we just remove the binding
+	 * scope blindly.
+	 */
+	if (msg->binding_status == FTS_ACTIVE &&
+	    (chaddr_changed || ident_changed)) {
+		ddns_removals(lease, NULL);
+
+		if (lease->scope != NULL)
+			binding_scope_dereference(&lease->scope, MDL);
+	}
 
 	/* XXX Times may need to be adjusted based on clock skew! */
 	if (msg -> options_present & FTB_STOS) {
diff --git a/server/mdb.c b/server/mdb.c
index d7acc8f8..b148c264 100644
--- a/server/mdb.c
+++ b/server/mdb.c
@@ -1459,6 +1459,21 @@ void make_binding_state_transition (struct lease *lease)
 	      lease -> binding_state == FTS_ACTIVE &&
 	      lease -> next_binding_state == FTS_RELEASED))) {
 #if defined (NSUPDATE)
+		/*
+		 * Note: ddns_removals() is also iterated when the lease
+		 * enters state 'released' in 'release_lease()'.  The below
+		 * is caught when a peer receives a BNDUPD from a failover
+		 * peer; it may not have received the client's release (it
+		 * may have been offline).
+		 *
+		 * We could remove the call from release_lease() because
+		 * it will also catch here on the originating server after the
+		 * peer acknowledges the state change.  However, there could
+		 * be many hours inbetween, and in this case we /know/ the
+		 * client is no longer using the lease when we receive the
+		 * release message.  This is not true of expiry, where the
+		 * peer may have extended the lease.
+		 */
 		ddns_removals(lease, NULL);
 #endif
 		if (lease -> on_release) {