Virtual: fix race condition in DHCP server which resulted in multiple clients receiving the same IP

A race condition in the DHCP server caused it to offer the same IP address to multiple clients when they connected at the same time, because an offered IP address was considered free until the final step (DHCP_ACK).

This commit introduces a list to keep track of the pending leases created during DHCP_OFFER, so that an IP address is guaranteed to be offered to a single client.

src/Cedar/Virtual.c

@@ -8956,8 +8956,8 @@ void FreeDhcpServer(VH *v)
 		return;
 	}
 
-	// Remove the all lease entries
+	// Empty the leases lists
-	for (i = 0;i < LIST_NUM(v->DhcpLeaseList);i++)
+	for (i = 0; i < LIST_NUM(v->DhcpLeaseList); ++i)
 	{
 		DHCP_LEASE *d = LIST_DATA(v->DhcpLeaseList, i);
 		FreeDhcpLease(d);
@@ -8965,6 +8965,15 @@ void FreeDhcpServer(VH *v)
 
 	ReleaseList(v->DhcpLeaseList);
 	v->DhcpLeaseList = NULL;
+
+	for (i = 0; i < LIST_NUM(v->DhcpPendingLeaseList); ++i)
+	{
+		DHCP_LEASE *d = LIST_DATA(v->DhcpPendingLeaseList, i);
+		FreeDhcpLease(d);
+	}
+
+	ReleaseList(v->DhcpPendingLeaseList);
+	v->DhcpPendingLeaseList = NULL;
 }
 
 // Initialize the DHCP server
@@ -8978,6 +8987,29 @@ void InitDhcpServer(VH *v)
 
 	// Create a list
 	v->DhcpLeaseList = NewList(CompareDhcpLeaseList);
+	v->DhcpPendingLeaseList = NewList(CompareDhcpLeaseList);
+}
+
+// Search for a pending DHCP lease item by the IP address
+DHCP_LEASE *SearchDhcpPendingLeaseByIp(VH *v, UINT ip)
+{
+	UINT i;
+	// Validate arguments
+	if (v == NULL)
+	{
+		return NULL;
+	}
+
+	for (i = 0; i < LIST_NUM(v->DhcpPendingLeaseList); ++i)
+	{
+		DHCP_LEASE *d = LIST_DATA(v->DhcpPendingLeaseList, i);
+		if (d->IpAddress == ip)
+		{
+			return d;
+		}
+	}
+
+	return NULL;
 }
 
 // Search for a DHCP lease item by the IP address
@@ -8990,7 +9022,7 @@ DHCP_LEASE *SearchDhcpLeaseByIp(VH *v, UINT ip)
 		return NULL;
 	}
 
-	for (i = 0;i < LIST_NUM(v->DhcpLeaseList);i++)
+	for (i = 0; i < LIST_NUM(v->DhcpLeaseList); ++i)
 	{
 		DHCP_LEASE *d = LIST_DATA(v->DhcpLeaseList, i);
 		if (d->IpAddress == ip)
@@ -9002,6 +9034,22 @@ DHCP_LEASE *SearchDhcpLeaseByIp(VH *v, UINT ip)
 	return NULL;
 }
 
+// Search for a pending DHCP lease item by the MAC address
+DHCP_LEASE *SearchDhcpPendingLeaseByMac(VH *v, UCHAR *mac)
+{
+	DHCP_LEASE *d, t;
+	// Validate arguments
+	if (v == NULL || mac == NULL)
+	{
+		return NULL;
+	}
+
+	Copy(&t.MacAddress, mac, 6);
+	d = Search(v->DhcpPendingLeaseList, &t);
+
+	return d;
+}
+
 // Search for a DHCP lease item by the MAC address
 DHCP_LEASE *SearchDhcpLeaseByMac(VH *v, UCHAR *mac)
 {
@@ -9099,9 +9147,8 @@ void PollingDhcpServer(VH *v)
 	}
 	v->LastDhcpPolling = v->Now;
 
-	// Remove expired entries
+LIST_CLEANUP:
-FIRST_LIST:
+	for (i = 0; i < LIST_NUM(v->DhcpLeaseList); ++i)
-	for (i = 0;i < LIST_NUM(v->DhcpLeaseList);i++)
 	{
 		DHCP_LEASE *d = LIST_DATA(v->DhcpLeaseList, i);
 
@@ -9109,7 +9156,21 @@ FIRST_LIST:
 		{
 			FreeDhcpLease(d);
 			Delete(v->DhcpLeaseList, d);
-			goto FIRST_LIST;
+			goto LIST_CLEANUP;
+		}
+	}
+
+PENDING_LIST_CLEANUP:
+	// Remove expired entries
+	for (i = 0; i < LIST_NUM(v->DhcpPendingLeaseList); ++i)
+	{
+		DHCP_LEASE *d = LIST_DATA(v->DhcpPendingLeaseList, i);
+
+		if (d->ExpireTime < v->Now)
+		{
+			FreeDhcpLease(d);
+			Delete(v->DhcpPendingLeaseList, d);
+			goto PENDING_LIST_CLEANUP;
 		}
 	}
 }
@@ -9151,6 +9212,11 @@ UINT ServeDhcpDiscover(VH *v, UCHAR *mac, UINT request_ip)
 	{
 		// IP address is specified
 		DHCP_LEASE *d = SearchDhcpLeaseByIp(v, request_ip);
+		if (d == NULL)
+		{
+			d = SearchDhcpPendingLeaseByIp(v, request_ip);
+		}
+
 		if (d != NULL)
 		{
 			// If an entry for the same IP address already exists,
@@ -9187,6 +9253,11 @@ UINT ServeDhcpDiscover(VH *v, UCHAR *mac, UINT request_ip)
 		// If there is any entry with the same MAC address
 		// that are already registered, use it with priority
 		DHCP_LEASE *d = SearchDhcpLeaseByMac(v, mac);
+		if (d == NULL)
+		{
+			d = SearchDhcpPendingLeaseByMac(v, mac);
+		}
+
 		if (d != NULL)
 		{
 			// Examine whether the found IP address is in the allocation region
@@ -9234,7 +9305,7 @@ UINT GetFreeDhcpIpAddress(VH *v)
 	for (i = ip_start; i <= ip_end;i++)
 	{
 		UINT ip = Endian32(i);
-		if (SearchDhcpLeaseByIp(v, ip) == NULL)
+		if (SearchDhcpLeaseByIp(v, ip) == NULL && SearchDhcpPendingLeaseByIp(v, ip) == NULL)
 		{
 			// A free IP address is found
 			return ip;
@@ -9284,7 +9355,7 @@ UINT GetFreeDhcpIpAddressByRandom(VH *v, UCHAR *mac)
 
 		new_ip = Endian32(ip_start + (rand_int % (ip_end - ip_start + 1)));
 
-		if (SearchDhcpLeaseByIp(v, new_ip) == NULL)
+		if (SearchDhcpLeaseByIp(v, new_ip) == NULL && SearchDhcpPendingLeaseByIp(v, new_ip) == NULL)
 		{
 			// A free IP address is found
 			return new_ip;
@@ -9401,8 +9472,9 @@ void VirtualDhcpServer(VH *v, PKT *p)
 			if (opt->Opcode == DHCP_REQUEST)
 			{
 				DHCP_LEASE *d;
-				char mac[MAX_SIZE];
+				char client_mac[MAX_SIZE];
-				char str[MAX_SIZE];
+				char client_ip[MAX_SIZE];
+
 				// Remove old records with the same IP address
 				d = SearchDhcpLeaseByIp(v, ip);
 				if (d != NULL)
@@ -9411,17 +9483,22 @@ void VirtualDhcpServer(VH *v, PKT *p)
 					Delete(v->DhcpLeaseList, d);
 				}
 
+				d = SearchDhcpPendingLeaseByIp(v, ip);
+				if (d != NULL)
+				{
+					FreeDhcpLease(d);
+					Delete(v->DhcpPendingLeaseList, d);
+				}
+
 				// Create a new entry
-				d = NewDhcpLease(v->DhcpExpire, p->MacAddressSrc,
+				d = NewDhcpLease(v->DhcpExpire, p->MacAddressSrc, ip, v->DhcpMask, opt->Hostname);
-					ip, v->DhcpMask,
-					opt->Hostname);
 				d->Id = ++v->DhcpId;
 				Add(v->DhcpLeaseList, d);
-				MacToStr(mac, sizeof(mac), d->MacAddress);
 
-				IPToStr32(str, sizeof(str), d->IpAddress);
+				MacToStr(client_mac, sizeof(client_mac), d->MacAddress);
+				IPToStr32(client_ip, sizeof(client_ip), d->IpAddress);
 
-				NLog(v, "LH_NAT_DHCP_CREATED", d->Id, mac, str, d->Hostname, v->DhcpExpire / 1000);
+				NLog(v, "LH_NAT_DHCP_CREATED", d->Id, client_mac, client_ip, d->Hostname, v->DhcpExpire / 1000);
 			}
 
 			// Respond
@@ -9510,12 +9587,25 @@ void VirtualDhcpServer(VH *v, PKT *p)
 					char client_mac[MAX_SIZE];
 					char client_ip[64];
 					IP ips;
+
 					BinToStr(client_mac, sizeof(client_mac), p->MacAddressSrc, 6);
 					UINTToIP(&ips, ip);
 					IPToStr(client_ip, sizeof(client_ip), &ips);
-					Debug("DHCP %s : %s given %s\n",
+
-						ret.Opcode == DHCP_OFFER ? "DHCP_OFFER" : "DHCP_ACK",
+					if (ret.Opcode == DHCP_OFFER)
-						client_mac, client_ip);
+					{
+						// DHCP_OFFER
+						DHCP_LEASE *d = NewDhcpLease(5000, p->MacAddressSrc, ip, v->DhcpMask, opt->Hostname);
+						d->Id = LIST_NUM(v->DhcpPendingLeaseList);
+						Add(v->DhcpPendingLeaseList, d);
+
+						Debug("VirtualDhcpServer(): %s has been marked as pending for %s\n", client_ip, client_mac);
+					}
+					else
+					{
+						// DHCP_ACK
+						Debug("VirtualDhcpServer(): %s has been assigned to %s\n", client_ip, client_mac);
+					}
 				}
 
 				// Build a DHCP option

src/Cedar/Virtual.h

@@ -296,6 +296,7 @@ struct VH
 	UINT DhcpDns2;					// DNS server address 2
 	char DhcpDomain[MAX_HOST_NAME_LEN + 1];	// Assigned domain name
 	LIST *DhcpLeaseList;			// DHCP lease list
+	LIST *DhcpPendingLeaseList;		// Pending DHCP lease list
 	UINT64 LastDhcpPolling;			// Time which the DHCP list polled last
 	bool SaveLog;					// Save a log
 	DHCP_CLASSLESS_ROUTE_TABLE PushRoute;	// Pushing routing table
@@ -511,7 +512,9 @@ int CompareDhcpLeaseList(void *p1, void *p2);
 DHCP_LEASE *NewDhcpLease(UINT expire, UCHAR *mac_address, UINT ip, UINT mask, char *hostname);
 void FreeDhcpLease(DHCP_LEASE *d);
 DHCP_LEASE *SearchDhcpLeaseByMac(VH *v, UCHAR *mac);
+DHCP_LEASE *SearchDhcpPendingLeaseByMac(VH *v, UCHAR *mac);
 DHCP_LEASE *SearchDhcpLeaseByIp(VH *v, UINT ip);
+DHCP_LEASE *SearchDhcpPendingLeaseByIp(VH *v, UINT ip);
 UINT ServeDhcpDiscover(VH *v, UCHAR *mac, UINT request_ip);
 UINT GetFreeDhcpIpAddress(VH *v);
 UINT GetFreeDhcpIpAddressByRandom(VH *v, UCHAR *mac);