analyse: critical bug in the counting code

The problem is, that you simply count all lease occurrences in
dhcpd.leases, but only the last ones for each ip address are
valid.  The lease file is more like a logfile of what has been
done, than a real database.  To fix the counting issue, I'm using
a single hash (from uthash.h [1]) for the counting.  This way
only the last lease entry for each IP gets into my counting
structure.

When you remove the duplicates in prepare_data(), you don't have
the information anymore, if the active lease entry or the free
lease entry came last.  Simply deleting each ip from the touches
array, that is already in the leases array, gives you a big
chance to count wrong.  Another way of fixing this would be to
not only store the ips in your arrays, but a structure containing
the ip and a global lease entry counter.  Then you could delete
all entries except for the latest.

[1] http://uthash.sourceforge.net/

Reported-by: Huangy
Signed-off-by: Enno Gr�per <groepeen@cms.hu-berlin.de>
Signed-off-by: Sami Kerola <kerolasa@iki.fi>

README

@@ -25,6 +25,9 @@ This is dhcpd-pools which is made for ISC dhcpd pool range analysis.
 	small some other dhcp analyzer might be more suitable for
 	you.
 
+== Dependencies to other projects
+
+	http://uthash.sourceforge.net/
 
 == Test data wanted
 

THANKS

@@ -21,3 +21,4 @@ Ahmed AL Dakhil
 Adam Ciarcinski
 Rezso Gajdóczy
 Robert Viou
+Enno Gröper

TODO

@@ -1,32 +1,10 @@
 ### Bugs
 
-> 1. from huangyu: china,fujian province,fuzhou
->
-> according to my observation, every lease,for example,ip
-> 192.168.0.123 perhaps appears many times in dhcpd.leases,but
-> only the last time's state is the final valid state of this ip
-> 192.168.0.123.
->
-> No matter it appears how many times in dhcpd.leases,and no
-> matter it has how many different state in dhcpd.leases. no
-> matter it had been assigned to how many different mac-address.
-> As long as it's last appearance's binding state is free in
-> dhcpd.leases,then ,it's free,in other word,it's not active.
->
-> and i notice that if a active lease time expired,dhcpd daemon
-> will set this lease's binding state from active to free
-> directly,without released state in this period.
->
-> in other words, when a lease's binding state "active" follow by
-> "free",not follow by "released";dhcpd-pools still think it's
-> active.
->
-> and another bug maybe is:
->
-> i found dhcpd-pools didn't consider if the active lease's ends
-> time is passed so it would still think the lease is active even
-> if dhcpd daemon is stopped for a long time,the leases marked by
-> "active" state in lease_file has expired in fact.
+Reported-by: Huangyu: I found dhcpd-pools didn't consider if the
+   active lease's ends time is passed so it would still think the
+   lease is active even if dhcpd daemon is stopped for a long time,
+   the leases marked by "active" state in lease_file has expired in
+   fact.
 
 ### Feature requests
 

configure.ac

@@ -38,6 +38,8 @@ AC_CHECK_HEADERS([\
 	unistd.h \
 ])
 
+AC_CHECK_HEADER(uthash.h, [], [AC_MSG_ERROR([Unable to find uthash.h])])
+
 # Checks for typedefs, structures, and compiler characteristics.
 AC_C_INLINE
 AC_HEADER_STDBOOL

src/Makefile.am

@@ -3,6 +3,15 @@
 bin_PROGRAMS = dhcpd-pools
 AC_PROG_RANLIB = resolv
 
-dhcpd_pools_SOURCES = dhcpd-pools.h defaults.h dhcpd-pools.c analyze.c getdata.c other.c sort.c output.c
+dhcpd_pools_SOURCES = \
+	analyze.c \
+	defaults.h \
+	dhcpd-pools.c \
+	dhcpd-pools.h \
+	getdata.c \
+	hash.c \
+	other.c \
+	output.c \
+	sort.c
 
 INCLUDES = -I. -I..

src/analyze.c

@@ -42,45 +42,15 @@
 #include "dhcpd-pools.h"
 
 /* Clean up data */
+int ip_sort(struct leases_t *a, struct leases_t *b)
+{
+	return (a->ip - b->ip);
+}
+
 int prepare_data(void)
 {
-	unsigned long int i, j, k;
-
 	/* Sort leases */
-	qsort(leases, (size_t) num_leases, sizeof(uint32_t), &intcomp);
-
-	/* Get rid of lease dublicates */
-	for (k = j = i = 0; i < num_leases; i++) {
-		if (j != leases[i]) {
-			leases[k] = leases[i];
-			j = leases[i];
-			k++;
-		}
-	}
-	num_leases = k;
-
-	/* Delete touched IPs that are in use. */
-	j = 0;
-	for (i = 0; i < num_touches; i++) {
-		if (bsearch
-		    (&touches[i], leases, (size_t) num_leases,
-		     sizeof(uint32_t), &intcomp) == NULL) {
-			touches[j] = touches[i];
-			j++;
-		}
-	}
-	num_touches = j;
-	qsort(touches, (size_t) num_touches, sizeof(uint32_t), &intcomp);
-
-	/* Sort ranges */
-	qsort(ranges, (size_t) num_ranges, sizeof(struct range_t), &rangecomp);
-
-	/* Sort backups */
-	if (0 < num_backups) {
-		qsort(backups, (size_t) num_backups, sizeof(uint32_t),
-		      &intcomp);
-	}
-
+	HASH_SORT(leases, ip_sort);
 	return 0;
 }
 
@@ -88,47 +58,43 @@ int prepare_data(void)
 int do_counting(void)
 {
 	struct range_t *range_p;
-	unsigned long i, j, k, l, block_size;
+	struct leases_t *l;
+	unsigned long i, k, block_size;
 
 	range_p = ranges;
 
 	/* Walk through ranges */
-	for (i = j = k = l = 0; i < num_ranges; i++) {
+	for (i = 0; i < num_ranges; i++) {
 		/* Count IPs in use */
-		for (; leases[j] < range_p->last_ip && j < num_leases; j++) {
-			if (leases[j] < range_p->first_ip) {
+		for (l = leases; l != NULL && range_p->last_ip >= l->ip; l = l->hh.next) {
+			if (l->ip < range_p->first_ip) {
+				/* should not be necessary */
 				continue;
 			}
-			/* IP with in range */
-			range_p->count++;
-			if (range_p->shared_net) {
-				range_p->shared_net->used++;
-			}
-		}
-
-		/* Count touched IPs */
-		for (; touches[k] < range_p->last_ip && k < num_touches; k++) {
-			if (touches[k] < range_p->first_ip) {
-				continue;
-			}
-			/* IP with in range */
-			range_p->touched++;
-			if (range_p->shared_net) {
-				range_p->shared_net->touched++;
-			}
-		}
-
-		/* Count backup IPs */
-		if (0 < num_backups) {
-			for (; backups[l] < range_p->last_ip
-			     && l < num_touches; l++) {
-				if (touches[l] < range_p->first_ip) {
-					continue;
-				}
-				/* IP with in range */
+			/* IP in range */
+			switch (l->type) {
+			case ACTIVE:
+				range_p->count++;
+				break;
+			case FREE:
+				range_p->touched++;
+				break;
+			case BACKUP:
 				range_p->backups++;
-				if (range_p->shared_net) {
+				break;
+			}
+
+			if (range_p->shared_net) {
+				switch (l->type) {
+				case ACTIVE:
+					range_p->shared_net->used++;
+					break;
+				case FREE:
+					range_p->shared_net->touched++;
+					break;
+				case BACKUP:
 					range_p->shared_net->backups++;
+					break;
 				}
 			}
 		}
@@ -140,15 +106,6 @@ int do_counting(void)
 			range_p->shared_net->available += block_size;
 		}
 
-		/* Go backwards one step so that not even a one IP will be
-		 * missed. This is possibly always unnecessary. */
-		if (j) {
-			j--;
-		}
-		if (k) {
-			k--;
-		}
-
 		range_p++;
 	}
 

src/dhcpd-pools.h

@@ -40,6 +40,7 @@
 #include <arpa/inet.h>
 #include <stddef.h>
 #include <stdio.h>
+#include <uthash.h>
 
 /* Feature test switches */
 #define _POSIX_SOURCE 1
@@ -122,6 +123,16 @@ struct macaddr_t {
 	char *ip;
 	struct macaddr_t *next;
 };
+enum ltype {
+	ACTIVE,
+	FREE,
+	BACKUP
+};
+struct leases_t {
+	uint32_t ip;	/* ip as key */
+	enum ltype type;
+	UT_hash_handle hh;
+};
 
 /* Global variables */
 struct configuration_t config;
@@ -133,11 +144,9 @@ struct shared_network_t *shared_networks;
 unsigned int num_shared_networks;
 struct range_t *ranges;
 unsigned int num_ranges;
-uint32_t *leases;
+struct leases_t *leases;
 unsigned long int num_leases;
-uint32_t *touches;
 unsigned long int num_touches;
-uint32_t *backups;
 unsigned long int num_backups;
 struct macaddr_t *macaddr;
 
@@ -200,5 +209,10 @@ int output_xml(void);
 int output_csv(void);
 /* Memory release, file closing etc */
 void clean_up(void);
+/* Hash functions */
+void add_lease(int ip, enum ltype type);
+struct leases_t * find_lease(int ip);
+void delete_lease(struct leases_t * lease);
+void delete_all_leases();
 
 #endif				/* DHCPD_POOLS_H */

src/getdata.c

@@ -71,10 +71,8 @@ int parse_leases(void)
 	struct in_addr inp;
 	struct stat lease_file_stats;
 	struct macaddr_t *macaddr_p = NULL;
-	size_t leasesmallocsize;
-	size_t touchesmallocsize;
-	size_t backupsmallocsize;
 	int sw_active_lease = 0;
+	struct leases_t * lease;
 
 	num_touches = num_leases = num_backups = 0;
 
@@ -104,14 +102,6 @@ int parse_leases(void)
 	if (stat(config.dhcpdlease_file, &lease_file_stats)) {
 		err(EXIT_FAILURE, "parse_leases: %s", config.dhcpdlease_file);
 	}
-	leasesmallocsize = (lease_file_stats.st_size / 250) + MAXLEN - 2;
-	touchesmallocsize = (lease_file_stats.st_size / 250) + MAXLEN - 2;
-	backupsmallocsize = (lease_file_stats.st_size / 120) + MAXLEN - 2;
-	leases = safe_malloc(sizeof(uint32_t) * leasesmallocsize);
-	touches = safe_malloc(sizeof(uint32_t) * touchesmallocsize);
-
-	memset(leases, 0, sizeof(uint32_t) * leasesmallocsize);
-	memset(touches, 0, sizeof(uint32_t) * touchesmallocsize);
 
 	line = safe_malloc(sizeof(char) * MAXLEN);
 	ipstring = safe_malloc(sizeof(char) * MAXLEN);
@@ -136,40 +126,24 @@ int parse_leases(void)
 		}
 		/* Copy IP to correct array */
 		else if (xstrstr(line, "  binding state active", 22)) {
-			leases[num_leases] = htonl(inp.s_addr);
-			num_leases++;
-			if (leasesmallocsize < num_leases) {
-				leasesmallocsize =
-				    sizeof(uint32_t) * num_leases * 2;
-				leases = safe_realloc(leases, leasesmallocsize);
-				leasesmallocsize /= sizeof(uint32_t);
+			/* remove old entry, if exists */
+			if ((lease = find_lease(htonl(inp.s_addr))) != NULL) {
+				delete_lease(lease);
 			}
+			add_lease(htonl(inp.s_addr),ACTIVE);
 			sw_active_lease = 1;
 		} else if (xstrstr(line, "  binding state free", 20)) {
-			touches[num_touches] = htonl(inp.s_addr);
-			num_touches++;
-			if (touchesmallocsize < num_touches) {
-				touchesmallocsize =
-				    sizeof(uint32_t) * num_touches * 2;
-				touches =
-				    safe_realloc(touches, touchesmallocsize);
-				touchesmallocsize /= sizeof(uint32_t);
+			/* remove old entry, if exists */
+			if ((lease = find_lease(htonl(inp.s_addr))) != NULL) {
+				delete_lease(lease);
 			}
+			add_lease(htonl(inp.s_addr),FREE);
 		} else if (xstrstr(line, "  binding state backup", 22)) {
-			if (num_backups == 0) {
-				backups =
-				    safe_malloc(sizeof(uint32_t) *
-						backupsmallocsize);
-			}
-			backups[num_backups] = htonl(inp.s_addr);
-			num_backups++;
-			if (backupsmallocsize < num_backups) {
-				backupsmallocsize =
-				    sizeof(uint32_t) * num_backups * 2;
-				backups =
-				    safe_realloc(backups, backupsmallocsize);
-				backupsmallocsize /= sizeof(uint32_t);
+			/* remove old entry, if exists */
+			if ((lease = find_lease(htonl(inp.s_addr))) != NULL) {
+				delete_lease(lease);
 			}
+			add_lease(htonl(inp.s_addr),BACKUP);
 		}
 
 		if ((macaddr != NULL)

src/hash.c

@@ -0,0 +1,80 @@
+/*
+ * The dhcpd-pools has BSD 2-clause license which also known as "Simplified
+ * BSD License" or "FreeBSD License".
+ *
+ * Copyright 2012- Enno Gröper. All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are
+ * met:
+ *
+ *    1. Redistributions of source code must retain the above copyright
+ *       notice, this list of conditions and the following disclaimer.
+ *
+ *    2. Redistributions in binary form must reproduce the above copyright
+ *       notice, this list of conditions and the following disclaimer in the
+ *       documentation and/or other materials provided with the
+ *       distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
+ * PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR AND CONTRIBUTORS OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
+ * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
+ * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
+ * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
+ * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING
+ * NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
+ * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ *
+ * The views and conclusions contained in the software and documentation are
+ * those of the authors and should not be interpreted as representing
+ * official policies, either expressed or implied, of Sami Kerola.
+ */
+
+#include "dhcpd-pools.h"
+
+void add_lease(int ip, enum ltype type)
+{
+	struct leases_t *l;
+	l = safe_malloc(sizeof(struct leases_t));
+	l->ip = ip;
+	l->type = type;
+	HASH_ADD_INT(leases, ip, l);
+}
+
+struct leases_t *find_lease(int ip)
+{
+	struct leases_t *l;
+
+	HASH_FIND_INT(leases, &ip, l);
+	return l;
+}
+
+void delete_lease(struct leases_t *lease)
+{
+	HASH_DEL(leases, lease);
+	free(lease);
+}
+
+/* uthash >= 1.9.2
+void delete_all_leases()
+{
+	struct leases_t *l, *tmp;
+	HASH_ITER(hh, leases, l, tmp) {
+		HASH_DEL(leases, l);
+		free(l);
+	}
+}
+*/
+
+void delete_all_leases()
+{
+	struct leases_t *l;
+	while (leases) {
+		l = leases;
+		HASH_DEL(leases, l);	/* leases advances to next on delete */
+		free(l);
+	}
+}

src/other.c

@@ -148,9 +148,7 @@ void clean_up(void)
 	free(config.dhcpdlease_file);
 	free(config.output_file);
 	free(ranges);
-	free(leases);
-	free(backups);
-	free(touches);
+	delete_all_leases();
 	free(shared_networks);
 }