dnsmasq: fix generating shared IPv4 address range

Change behavior for the network-address and broadcast-address.
Users should not specify such addresses, but if they do, generate
something more sensible.

Also, if the address was in network larger then /24, the
generated address range was rather unexpected. Change behavior
here.

There are no particularly strong reasons for the chosen range.
It just seems suitable. The decision to hand out at most a /24
is because it is likely to be plenty, and because that is what
the previous code did -- at least, if the address was in the
first /24 of the subnet. See how the result for 192.168.0.1/20
is unchanged, but 192.168.1.1/20 changes.

2 files changed, 101 insertions, 59 deletions

diff --git a/src/dnsmasq/nm-dnsmasq-utils.c b/src/dnsmasq/nm-dnsmasq-utils.c
index e4f4324b85..cee52c3def 100644
--- a/src/dnsmasq/nm-dnsmasq-utils.c
+++ b/src/dnsmasq/nm-dnsmasq-utils.c
@@ -35,11 +35,12 @@ nm_dnsmasq_utils_get_range (const NMPlatformIP4Address *addr,
 {
 	guint32 host = addr->address;
 	guint8 prefix = addr->plen;
-	guint32 netmask = nm_utils_ip4_prefix_to_netmask (prefix);
-	guint32 first, last, reserved;
+	guint32 netmask;
+	guint32 first, last, mid, reserved;
+	const guint32 NUM = 256;
 
-	g_return_val_if_fail (out_first != NULL, FALSE);
-	g_return_val_if_fail (out_last != NULL, FALSE);
+	g_return_val_if_fail (out_first, FALSE);
+	g_return_val_if_fail (out_last, FALSE);
 
 	if (prefix > 30) {
 		if (out_error_desc)
@@ -47,29 +48,69 @@ nm_dnsmasq_utils_get_range (const NMPlatformIP4Address *addr,
 		return FALSE;
 	}
 
-	/* Find the first available address *after* the local machine's IP */
-	first = (host & netmask) + htonl (1);
+	if (prefix < 24) {
+		/* if the subnet is larger then /24, we partition it and treat it
+		 * like it would be a /24.
+		 *
+		 * Hence, the resulting range will always be between x.x.x.1/24
+		 * and x.x.x.254/24, with x.x.x.0 being the network address of the
+		 * host.
+		 *
+		 * In this case, only a /24 portion of the subnet is used.
+		 * No particular reason for that, but it's unlikely that a user
+		 * would use NetworkManager's shared method when having hundered
+		 * of DHCP clients. So, restrict the range to the same /24 in
+		 * which the host address lies.
+		 */
+		prefix = 24;
+	}
+
+	netmask = nm_utils_ip4_prefix_to_netmask (prefix);
+
+	/* treat addresses in host-order from here on. */
+	netmask = ntohl (netmask);
+	host = ntohl (host);
 
-	/* Shortcut: allow a max of 253 addresses; the - htonl(1) here is to assure
-	 * that we don't set 'last' to the broadcast address of the network. */
-	if (prefix < 24)
-		last = (host | ~nm_utils_ip4_prefix_to_netmask (24)) - htonl (1);
-	else
-		last = (host | ~netmask) - htonl(1);
+	/* if host is the network or broadcast address, coerce it to
+	 * one above or below. Usually, we wouldn't expect the user
+	 * to pick such an address. */
+	if (host == (host & netmask))
+		host++;
+	else if (host == (host | ~netmask))
+		host--;
 
-	/* Figure out which range (either above the host address or below it)
-	 * has more addresses.  Reserve some addresses for static IPs.
+	/* Exclude the network and broadcast address. */
+	first = (host &  netmask) + 1;
+	last =  (host | ~netmask) - 1;
+
+	/* Depending on whether host is above or below the middle of
+	 * the subnet, the larger part if handed out.
+	 *
+	 * If the host is in the lower half, the range starts
+	 * at the lower end with the host (plus reserved), until the
+	 * broadcast address
+	 *
+	 * If the host is in the upper half, the range starts above
+	 * the network-address and goes up until the host (except reserved).
+	 *
+	 * reserved is up to 8 addresses, 10% of the determined range.
 	 */
-	if (ntohl (host) - ntohl (first) > ntohl (last) - ntohl (host)) {
-		/* Range below the host's IP address */
-		reserved = (guint32) ((ntohl (host) - ntohl (first)) / 10);
-		last = host - htonl (MIN (reserved, 8)) - htonl (1);
+	mid =   (host & netmask) | (((first + last) / 2) & ~netmask);
+	if (host > mid) {
+		/* use lower range */
+		reserved = NM_MIN (((host - first) / 10), 8);
+		last = host - 1 - reserved;
+		first = NM_MAX (first, last > NUM ? last - NUM : 0);
 	} else {
-		/* Range above host's IP address */
-		reserved = (guint32) ((ntohl (last) - ntohl (host)) / 10);
-		first = host + htonl (MIN (reserved, 8)) + htonl (1);
+		/* use upper range */
+		reserved = NM_MIN (((last - host) / 10), 8);
+		first = host + 1 + reserved;
+		last = NM_MIN (last, first < 0xFFFFFFFF - NUM ? first + NUM : 0xFFFFFFFF);
 	}
 
+	first = htonl (first);
+	last = htonl (last);
+
 	nm_utils_inet4_ntop (first, out_first);
 	nm_utils_inet4_ntop (last, out_last);
 
diff --git a/src/dnsmasq/tests/test-dnsmasq-utils.c b/src/dnsmasq/tests/test-dnsmasq-utils.c
index e62a20a016..b311ccb4a0 100644
--- a/src/dnsmasq/tests/test-dnsmasq-utils.c
+++ b/src/dnsmasq/tests/test-dnsmasq-utils.c
@@ -41,6 +41,7 @@ test_address_ranges (void)
 		g_assert (!_error_desc); \
 		g_assert_cmpstr (_first, ==, (expected_first"")); \
 		g_assert_cmpstr (_last, ==, (expected_last"")); \
+		g_assert_cmpint ((ntohl (nmtst_inet4_from_string (_last)) - ntohl (nmtst_inet4_from_string (_first))), <=, 244); \
 	} G_STMT_END
 
 #define _test_address_range_fail(addr, plen) \
@@ -58,68 +59,68 @@ test_address_ranges (void)
 
 	_test_address_range_fail ("1.2.3.1", 31);
 
-	_test_address_range ("0.0.0.0", 30, "0.0.0.1",  "0.0.0.247");
+	_test_address_range ("0.0.0.0", 30, "0.0.0.2",  "0.0.0.2");
 	_test_address_range ("0.0.0.1", 30, "0.0.0.2",  "0.0.0.2");
 	_test_address_range ("0.0.0.2", 30, "0.0.0.1",  "0.0.0.1");
-	_test_address_range ("0.0.0.3", 30, "0.0.0.12", "0.0.0.2");
-	_test_address_range ("1.2.3.0", 30, "1.2.3.1",  "1.2.3.247");
+	_test_address_range ("0.0.0.3", 30, "0.0.0.1",  "0.0.0.1");
+	_test_address_range ("1.2.3.0", 30, "1.2.3.2",  "1.2.3.2");
 	_test_address_range ("1.2.3.1", 30, "1.2.3.2",  "1.2.3.2");
 	_test_address_range ("1.2.3.2", 30, "1.2.3.1",  "1.2.3.1");
-	_test_address_range ("1.2.3.3", 30, "1.2.3.12", "1.2.3.2");
-	_test_address_range ("1.2.3.4", 30, "1.2.3.5",  "1.2.3.251");
+	_test_address_range ("1.2.3.3", 30, "1.2.3.1",  "1.2.3.1");
+	_test_address_range ("1.2.3.4", 30, "1.2.3.6",  "1.2.3.6");
 	_test_address_range ("1.2.3.5", 30, "1.2.3.6",  "1.2.3.6");
 	_test_address_range ("1.2.3.6", 30, "1.2.3.5",  "1.2.3.5");
-	_test_address_range ("1.2.3.7", 30, "1.2.3.16", "1.2.3.6");
-	_test_address_range ("1.2.3.8", 30, "1.2.3.9",  "1.2.3.255");
+	_test_address_range ("1.2.3.7", 30, "1.2.3.5",  "1.2.3.5");
+	_test_address_range ("1.2.3.8", 30, "1.2.3.10", "1.2.3.10");
 	_test_address_range ("1.2.3.9", 30, "1.2.3.10", "1.2.3.10");
-	_test_address_range ("255.255.255.0", 30, "255.255.255.1",  "255.255.255.247");
+	_test_address_range ("255.255.255.0", 30, "255.255.255.2",  "255.255.255.2");
 	_test_address_range ("255.255.255.1", 30, "255.255.255.2",  "255.255.255.2");
 	_test_address_range ("255.255.255.2", 30, "255.255.255.1",  "255.255.255.1");
-	_test_address_range ("255.255.255.3", 30, "255.255.255.12", "255.255.255.2");
-	_test_address_range ("255.255.255.248", 30, "255.255.255.249",  "255.255.255.239");
+	_test_address_range ("255.255.255.3", 30, "255.255.255.1",  "255.255.255.1");
+	_test_address_range ("255.255.255.248", 30, "255.255.255.250",  "255.255.255.250");
 	_test_address_range ("255.255.255.249", 30, "255.255.255.250",  "255.255.255.250");
 	_test_address_range ("255.255.255.250", 30, "255.255.255.249",  "255.255.255.249");
-	_test_address_range ("255.255.255.251", 30, "255.255.255.4",    "255.255.255.250");
-	_test_address_range ("255.255.255.252", 30, "255.255.255.253",  "255.255.255.243");
+	_test_address_range ("255.255.255.251", 30, "255.255.255.249",  "255.255.255.249");
+	_test_address_range ("255.255.255.252", 30, "255.255.255.254",  "255.255.255.254");
 	_test_address_range ("255.255.255.253", 30, "255.255.255.254",  "255.255.255.254");
 	_test_address_range ("255.255.255.254", 30, "255.255.255.253",  "255.255.255.253");
-	_test_address_range ("255.255.255.255", 30, "255.255.255.8",    "255.255.255.254");
+	_test_address_range ("255.255.255.255", 30, "255.255.255.253",  "255.255.255.253");
 
-	_test_address_range ("0.0.0.0", 29, "0.0.0.1",  "0.0.0.247");
+	_test_address_range ("0.0.0.0", 29, "0.0.0.2",  "0.0.0.6");
 	_test_address_range ("0.0.0.1", 29, "0.0.0.2",  "0.0.0.6");
 	_test_address_range ("0.0.0.2", 29, "0.0.0.3",  "0.0.0.6");
 	_test_address_range ("0.0.0.3", 29, "0.0.0.4",  "0.0.0.6");
 	_test_address_range ("0.0.0.4", 29, "0.0.0.1",  "0.0.0.3");
 	_test_address_range ("0.0.0.5", 29, "0.0.0.1",  "0.0.0.4");
 	_test_address_range ("0.0.0.6", 29, "0.0.0.1",  "0.0.0.5");
-	_test_address_range ("0.0.0.7", 29, "0.0.0.16", "0.0.0.6");
-	_test_address_range ("0.0.0.8", 29, "0.0.0.9",  "0.0.0.255");
+	_test_address_range ("0.0.0.7", 29, "0.0.0.1",  "0.0.0.5");
+	_test_address_range ("0.0.0.8", 29, "0.0.0.10", "0.0.0.14");
 	_test_address_range ("0.0.0.9", 29, "0.0.0.10", "0.0.0.14");
-	_test_address_range ("1.2.3.0", 29, "1.2.3.1",  "1.2.3.247");
+	_test_address_range ("1.2.3.0", 29, "1.2.3.2",  "1.2.3.6");
 	_test_address_range ("1.2.3.1", 29, "1.2.3.2",  "1.2.3.6");
 	_test_address_range ("1.2.3.2", 29, "1.2.3.3",  "1.2.3.6");
 	_test_address_range ("1.2.3.3", 29, "1.2.3.4",  "1.2.3.6");
 	_test_address_range ("1.2.3.4", 29, "1.2.3.1",  "1.2.3.3");
 	_test_address_range ("1.2.3.5", 29, "1.2.3.1",  "1.2.3.4");
 	_test_address_range ("1.2.3.6", 29, "1.2.3.1",  "1.2.3.5");
-	_test_address_range ("1.2.3.7", 29, "1.2.3.16", "1.2.3.6");
-	_test_address_range ("1.2.3.8", 29, "1.2.3.9",  "1.2.3.255");
+	_test_address_range ("1.2.3.7", 29, "1.2.3.1",  "1.2.3.5");
+	_test_address_range ("1.2.3.8", 29, "1.2.3.10", "1.2.3.14");
 	_test_address_range ("1.2.3.9", 29, "1.2.3.10", "1.2.3.14");
-	_test_address_range ("255.255.255.248", 29, "255.255.255.249",  "255.255.255.239");
+	_test_address_range ("255.255.255.248", 29, "255.255.255.250",  "255.255.255.254");
 	_test_address_range ("255.255.255.249", 29, "255.255.255.250",  "255.255.255.254");
 	_test_address_range ("255.255.255.250", 29, "255.255.255.251",  "255.255.255.254");
 	_test_address_range ("255.255.255.251", 29, "255.255.255.252",  "255.255.255.254");
 	_test_address_range ("255.255.255.252", 29, "255.255.255.249",  "255.255.255.251");
 	_test_address_range ("255.255.255.253", 29, "255.255.255.249",  "255.255.255.252");
 	_test_address_range ("255.255.255.254", 29, "255.255.255.249",  "255.255.255.253");
-	_test_address_range ("255.255.255.255", 29, "255.255.255.8",    "255.255.255.254");
+	_test_address_range ("255.255.255.255", 29, "255.255.255.249",  "255.255.255.253");
 
 	_test_address_range ("1.2.3.1", 29, "1.2.3.2", "1.2.3.6");
 	_test_address_range ("1.2.3.1", 28, "1.2.3.3", "1.2.3.14");
 	_test_address_range ("1.2.3.1", 26, "1.2.3.8", "1.2.3.62");
 
-	_test_address_range ("192.167.255.255", 24, "192.167.255.8", "192.167.255.254");
-	_test_address_range ("192.168.0.0",     24, "192.168.0.1",   "192.168.0.247");
+	_test_address_range ("192.167.255.255", 24, "192.167.255.1", "192.167.255.245");
+	_test_address_range ("192.168.0.0",     24, "192.168.0.10",  "192.168.0.254");
 	_test_address_range ("192.168.0.1",     24, "192.168.0.10",  "192.168.0.254");
 	_test_address_range ("192.168.0.2",     24, "192.168.0.11",  "192.168.0.254");
 	_test_address_range ("192.168.0.99",    24, "192.168.0.108", "192.168.0.254");
@@ -129,19 +130,19 @@ test_address_ranges (void)
 	_test_address_range ("192.168.0.129",   24, "192.168.0.1",   "192.168.0.120");
 	_test_address_range ("192.168.0.130",   24, "192.168.0.1",   "192.168.0.121");
 	_test_address_range ("192.168.0.254",   24, "192.168.0.1",   "192.168.0.245");
-	_test_address_range ("192.168.0.255",   24, "192.168.0.8",   "192.168.0.254");
-	_test_address_range ("192.168.1.0",     24, "192.168.1.1",   "192.168.1.247");
+	_test_address_range ("192.168.0.255",   24, "192.168.0.1",   "192.168.0.245");
+	_test_address_range ("192.168.1.0",     24, "192.168.1.10",  "192.168.1.254");
 	_test_address_range ("192.168.1.1",     24, "192.168.1.10",  "192.168.1.254");
 	_test_address_range ("192.168.1.2",     24, "192.168.1.11",  "192.168.1.254");
 	_test_address_range ("192.168.1.10",    24, "192.168.1.19",  "192.168.1.254");
 	_test_address_range ("192.168.15.253",  24, "192.168.15.1",  "192.168.15.244");
 	_test_address_range ("192.168.15.254",  24, "192.168.15.1",  "192.168.15.245");
-	_test_address_range ("192.168.15.255",  24, "192.168.15.8",  "192.168.15.254");
-	_test_address_range ("192.168.16.0",    24, "192.168.16.1",  "192.168.16.247");
+	_test_address_range ("192.168.15.255",  24, "192.168.15.1",  "192.168.15.245");
+	_test_address_range ("192.168.16.0",    24, "192.168.16.10", "192.168.16.254");
 	_test_address_range ("192.168.16.1",    24, "192.168.16.10", "192.168.16.254");
 
-	_test_address_range ("192.167.255.255", 20, "192.167.255.8", "192.167.255.254");
-	_test_address_range ("192.168.0.0",     20, "192.168.0.1",   "192.168.0.247");
+	_test_address_range ("192.167.255.255", 20, "192.167.255.1", "192.167.255.245");
+	_test_address_range ("192.168.0.0",     20, "192.168.0.10",  "192.168.0.254");
 	_test_address_range ("192.168.0.1",     20, "192.168.0.10",  "192.168.0.254");
 	_test_address_range ("192.168.0.2",     20, "192.168.0.11",  "192.168.0.254");
 	_test_address_range ("192.168.0.126",   20, "192.168.0.135", "192.168.0.254");
@@ -150,15 +151,15 @@ test_address_ranges (void)
 	_test_address_range ("192.168.0.129",   20, "192.168.0.1",   "192.168.0.120");
 	_test_address_range ("192.168.0.130",   20, "192.168.0.1",   "192.168.0.121");
 	_test_address_range ("192.168.0.254",   20, "192.168.0.1",   "192.168.0.245");
-	_test_address_range ("192.168.0.255",   20, "192.168.0.8",   "192.168.0.254");
-	_test_address_range ("192.168.1.0",     20, "192.168.0.1",   "192.168.1.247");
-	_test_address_range ("192.168.1.1",     20, "192.168.0.1",   "192.168.1.248");
-	_test_address_range ("192.168.1.2",     20, "192.168.0.1",   "192.168.1.249");
-	_test_address_range ("192.168.1.10",    20, "192.168.0.1",   "192.168.1.1");
-	_test_address_range ("192.168.15.253",  20, "192.168.0.1",   "192.168.15.244");
-	_test_address_range ("192.168.15.254",  20, "192.168.0.1",   "192.168.15.245");
-	_test_address_range ("192.168.15.255",  20, "192.168.15.8",  "192.168.15.254");
-	_test_address_range ("192.168.16.0",    20, "192.168.16.1",  "192.168.16.247");
+	_test_address_range ("192.168.0.255",   20, "192.168.0.1",   "192.168.0.245");
+	_test_address_range ("192.168.1.0",     20, "192.168.1.10",  "192.168.1.254");
+	_test_address_range ("192.168.1.1",     20, "192.168.1.10",  "192.168.1.254");
+	_test_address_range ("192.168.1.2",     20, "192.168.1.11",  "192.168.1.254");
+	_test_address_range ("192.168.1.10",    20, "192.168.1.19",  "192.168.1.254");
+	_test_address_range ("192.168.15.253",  20, "192.168.15.1",  "192.168.15.244");
+	_test_address_range ("192.168.15.254",  20, "192.168.15.1",  "192.168.15.245");
+	_test_address_range ("192.168.15.255",  20, "192.168.15.1",  "192.168.15.245");
+	_test_address_range ("192.168.16.0",    20, "192.168.16.10", "192.168.16.254");
 	_test_address_range ("192.168.16.1",    20, "192.168.16.10", "192.168.16.254");
 }