From: Thomas Markwalder <tmark@isc.org>
Date: Wed, 29 Jul 2015 12:32:50 +0000 (-0400)
Subject: [master] Fixed server crash after billing class is deleted
X-Git-Tag: v4_3_3b1~10
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=6a39bcf0be695fa2e0b62312ea8bdc830a08f7bc;p=thirdparty%2Fdhcp.git

[master] Fixed server crash after billing class is deleted

    Merges in rt39978.
---

diff --git a/RELNOTES b/RELNOTES
index f4e2c41b7..e670384a0 100644
--- a/RELNOTES
+++ b/RELNOTES
@@ -174,6 +174,11 @@ by Eric Young (eay@cryptsoft.com).
   within the same subnet and many people configure them in that fashion.
   [ISC-Bugs #40077]
 
+- Fixed a server crash that could occur when the server attempts to remove
+  the billing class from the last lease billed to a dynamic class after said
+  class has been deleted.
+  [ISC-Bugs #39978]
+
 			Changes since 4.3.2rc2
 - None
 
diff --git a/client/dhclient.c b/client/dhclient.c
index d14a9cf78..be2af6ad6 100644
--- a/client/dhclient.c
+++ b/client/dhclient.c
@@ -870,11 +870,9 @@ void classify (packet, class)
 {
 }
 
-int unbill_class (lease, class)
+void unbill_class (lease)
 	struct lease *lease;
-	struct class *class;
 {
-	return 0;
 }
 
 int find_subnet (struct subnet **sp,
diff --git a/includes/dhcpd.h b/includes/dhcpd.h
index 4e75421e7..20bf30e94 100644
--- a/includes/dhcpd.h
+++ b/includes/dhcpd.h
@@ -3127,7 +3127,7 @@ void classify (struct packet *, struct class *);
 isc_result_t unlink_class (struct class **class);
 isc_result_t find_class (struct class **, const char *,
 			 const char *, int);
-int unbill_class (struct lease *, struct class *);
+void unbill_class (struct lease *);
 int bill_class (struct lease *, struct class *);
 
 /* execute.c */
diff --git a/server/class.c b/server/class.c
index 48d4a6cb3..5c1d8e73b 100644
--- a/server/class.c
+++ b/server/class.c
@@ -3,7 +3,7 @@
    Handling for client classes. */
 
 /*
- * Copyright (c) 2009,2012-2014 by Internet Systems Consortium, Inc. ("ISC")
+ * Copyright (c) 2009,2012-2015 by Internet Systems Consortium, Inc. ("ISC")
  * Copyright (c) 2004,2007 by Internet Systems Consortium, Inc. ("ISC")
  * Copyright (c) 1998-2003 by Internet Software Consortium
  *
@@ -235,7 +235,7 @@ isc_result_t unlink_class(struct class **class) {
 	return ISC_R_NOTFOUND;
 }
 
-	
+
 isc_result_t find_class (struct class **class, const char *name,
 			 const char *file, int line)
 {
@@ -251,24 +251,53 @@ isc_result_t find_class (struct class **class, const char *name,
 	return ISC_R_NOTFOUND;
 }
 
-int unbill_class (lease, class)
+/* Removes the billing class from a lease
+ *
+ * Note that because classes can be created and removed dynamically, it is
+ * possible that the class to which a lease was billed has since been deleted.
+ * To cover the case where the lease is the last reference to a deleted class
+ * we remove the lease reference from the class first, then the class from the
+ * lease.  To protect ourselves from the reverse situation, where the class is
+ * the last reference to the lease (unlikely), we create a guard reference to
+ * the lease, then remove it at the end.
+ */
+void unbill_class (lease)
 	struct lease *lease;
-	struct class *class;
 {
 	int i;
+	struct class* class = lease->billing_class;
+	struct lease* refholder = NULL;
 
-	for (i = 0; i < class -> lease_limit; i++)
-		if (class -> billed_leases [i] == lease)
+	/* if there's no billing to remove, nothing to do */
+	if (class == NULL) {
+		return;
+	}
+
+	/* Find the lease in the list of the class's billed leases */
+	for (i = 0; i < class->lease_limit; i++) {
+		if (class->billed_leases[i] == lease)
 			break;
-	if (i == class -> lease_limit) {
+	}
+
+	/* Create guard reference, so class cannot be last reference to lease */
+	lease_reference(&refholder, lease, MDL);
+
+	/* If the class doesn't have the lease, then something is broken
+	 * programmatically.  We'll log it but skip the lease dereference. */
+	if (i == class->lease_limit) {
 		log_error ("lease %s unbilled with no billing arrangement.",
-		      piaddr (lease -> ip_addr));
-		return 0;
+			   piaddr(lease->ip_addr));
+	} else {
+		/* Remove the lease from the class */
+		lease_dereference(&class->billed_leases[i], MDL);
+		class->leases_consumed--;
 	}
-	class_dereference (&lease -> billing_class, MDL);
-	lease_dereference (&class -> billed_leases [i], MDL);
-	class -> leases_consumed--;
-	return 1;
+
+	/* Remove the class from the lease */
+	class_dereference(&lease->billing_class, MDL);
+
+	/* Ditch our guard reference */
+	lease_dereference(&refholder, MDL);
 }
 
 int bill_class (lease, class)
@@ -279,7 +308,7 @@ int bill_class (lease, class)
 
 	if (lease -> billing_class) {
 		log_error ("lease billed with existing billing arrangement.");
-		unbill_class (lease, lease -> billing_class);
+		unbill_class (lease);
 	}
 
 	if (class -> leases_consumed == class -> lease_limit)
diff --git a/server/dhcp.c b/server/dhcp.c
index 5db75f755..1f007e294 100644
--- a/server/dhcp.c
+++ b/server/dhcp.c
@@ -2321,7 +2321,7 @@ void ack_lease (packet, lease, offer, when, msg, ms_nulltp, hp)
 				    lease -> billing_class)
 					break;
 			if (i == packet -> class_count) {
-				unbill_class (lease, lease -> billing_class);
+				unbill_class(lease);
 				/* Active lease billing change negates reuse */
 				if (lease->binding_state == FTS_ACTIVE) {
 					lease->cannot_reuse = 1;
@@ -2373,7 +2373,7 @@ void ack_lease (packet, lease, offer, when, msg, ms_nulltp, hp)
 			if (offer == DHCPOFFER &&
 			    lease->billing_class != NULL &&
 			    lease->binding_state != FTS_ACTIVE)
-				unbill_class(lease, lease->billing_class);
+				unbill_class(lease);
 
 			/* Lease billing change negates reuse */
 			if (lease->billing_class != NULL) {
diff --git a/server/mdb.c b/server/mdb.c
index 6a637b5ef..219db506f 100644
--- a/server/mdb.c
+++ b/server/mdb.c
@@ -1185,8 +1185,8 @@ int supersede_lease (comp, lease, commit, propogate, pimmediate, from_pool)
 
 	/* If the lease has been billed to a class, remove the billing. */
 	if (comp -> billing_class != lease -> billing_class) {
-		if (comp -> billing_class)
-			unbill_class (comp, comp -> billing_class);
+		if (comp->billing_class)
+			unbill_class(comp);
 		if (lease -> billing_class)
 			bill_class (comp, lease -> billing_class);
 	}
@@ -1502,8 +1502,8 @@ void make_binding_state_transition (struct lease *lease)
 				(&lease->on_star.on_release, MDL);
 		/* Get rid of client-specific bindings that are only
 		   correct when the lease is active. */
-		if (lease -> billing_class)
-			unbill_class (lease, lease -> billing_class);
+		if (lease->billing_class)
+			unbill_class(lease);
 		if (lease -> agent_options)
 			option_chain_head_dereference (&lease -> agent_options,
 						       MDL);
@@ -1566,8 +1566,8 @@ void make_binding_state_transition (struct lease *lease)
 
 		/* Get rid of client-specific bindings that are only
 		   correct when the lease is active. */
-		if (lease -> billing_class)
-			unbill_class (lease, lease -> billing_class);
+		if (lease->billing_class)
+			unbill_class(lease);
 		if (lease -> agent_options)
 			option_chain_head_dereference (&lease -> agent_options,
 						       MDL);