Rewrite get_option(): new version fixes at least one bug, and is much easier

to understand and verify as correct.  It also returns the length of the
extracted option.

ndhc/README

@@ -52,3 +52,13 @@ go into an inactive state (until it is killed, or receives a SIGUSR1). You do
 not need to sleep between sending signals, as signals received are processed
 sequentially in the order they are received.
 
+DHCP pitfalls
+-------------
+
+Send a packet that has an options field set to:
+DHCP-OPTION-OVERLOAD:3
+
+Then in the file and sname fields:
+DHCP-OPTION-OVERLOAD:3
+
+I suspect some bad dhcp programs will hang given this input.

ndhc/ifchange.c

@@ -171,6 +171,7 @@ static void translate_option(int sockfd, struct dhcpMessage *packet,
     unsigned char *p;
     int i;
     struct dhcp_option *opt = NULL;
+    ssize_t optlen;
 
     if (!packet)
         return;
@@ -187,7 +188,7 @@ static void translate_option(int sockfd, struct dhcpMessage *packet,
     memset(buf, '\0', sizeof(buf));
     memset(buf2, '\0', sizeof(buf2));
 
-    p = get_option(packet, code);
+    p = get_option(packet, code, &optlen);
     if (fill_options(buf2, p, opt, sizeof buf2 - 1) == -1)
         return;
     snprintf(buf, sizeof buf, "%s:", buf2);

ndhc/options.c

@@ -113,68 +113,83 @@ unsigned char *alloc_dhcp_client_id_option(unsigned char type,
 	return alloc_option(DHCP_CLIENT_ID, data, sizeof data);
 }
 
-/* Get an option with bounds checking (warning, result is not aligned) */
-uint8_t* get_option(struct dhcpMessage *packet, int code)
+// Worker function for get_option().
+static uint8_t *do_get_option(uint8_t *buf, ssize_t buflen, int code,
+							  char *overload, ssize_t *optlen)
 {
-	uint8_t *optionptr;
-	int len, rem, overload = 0;
-	enum {
-		OPTION_FIELD = 0,
-		FILE_FIELD	 = 1,
-		SNAME_FIELD	 = 2
-	};
-	enum {
-		FILE_FIELD101  = FILE_FIELD  * 0x101,
-		SNAME_FIELD101 = SNAME_FIELD * 0x101,
-	};
-
-	/* option bytes: [code][len][data1][data2]..[dataLEN] */
-	optionptr = packet->options;
-	rem = sizeof packet->options;
-	while (1) {
-		if (rem <= 0) {
-			log_warning("Bad packet, malformed option field.");
-			return NULL;
-		}
-		if (optionptr[OPT_CODE] == DHCP_PADDING) {
-			rem--;
-			optionptr++;
+	/* option bytes: [code][len]([data1][data2]..[dataLEN]) */
+	*overload = 0;
+	while (buflen > 0) {
+		// Advance over padding.
+		if (buf[0] == DHCP_PADDING) {
+			buflen--;
+			buf++;
 			continue;
 		}
-		if (optionptr[OPT_CODE] == DHCP_END) {
-			if ((overload & FILE_FIELD101) == FILE_FIELD) {
-				/* can use packet->file, and didn't look at it yet */
-				overload |= FILE_FIELD101; /* "we looked at it" */
-				optionptr = packet->file;
-				rem = sizeof packet->file;
-				continue;
-			}
-			if ((overload & SNAME_FIELD101) == SNAME_FIELD) {
-				/* can use packet->sname, and didn't look at it yet */
-				overload |= SNAME_FIELD101; /* "we looked at it" */
-				optionptr = packet->sname;
-				rem = sizeof packet->sname;
-				continue;
-			}
-			break;
+
+		// We hit the end.
+		if (buf[0] == DHCP_END) {
+			*optlen = 0;
+			return NULL;
 		}
-		len = 2 + optionptr[OPT_LEN];
-		rem -= len;
-		if (rem < 0)
-			continue; /* complain and return NULL */
 
-		if (optionptr[OPT_CODE] == code)
-			return optionptr + OPT_DATA;
+		buflen -= buf[1] + 2;
+		if (buflen < 0) {
+			log_warning("Bad dhcp data: option length would exceed options field length");
+			*optlen = 0;
+			return NULL;
+		}
 
-		if (optionptr[OPT_CODE] == DHCP_OPTION_OVERLOAD) {
-			overload |= optionptr[OPT_DATA];
+		if (buf[0] == code) {
+			*optlen = buf[1];
+			return buf + 2;
+		}
+
+		if (buf[0] == DHCP_OPTION_OVERLOAD) {
+			if (buf[1] == 1)
+				*overload |= buf[2];
 			/* fall through */
 		}
-		optionptr += len;
+		buf += buf[1] + 2;
 	}
+	log_warning("Bad dhcp data: unmarked end of options field");
+	*optlen = 0;
 	return NULL;
 }
 
+/* Get an option with bounds checking (warning, result is not aligned) */
+uint8_t *get_option(struct dhcpMessage *packet, int code, ssize_t *optlen)
+{
+	uint8_t *option, *buf;
+	ssize_t buflen;
+	char overload, parsed_ff = 0;
+
+	buf = packet->options;
+	buflen = sizeof packet->options;
+
+	option = do_get_option(buf, buflen, code, &overload, optlen);
+	if (option)
+		return option;
+
+	if (overload & 1) {
+		parsed_ff = 1;
+		option = do_get_option(packet->file, sizeof packet->file,
+							   code, &overload, optlen);
+		if (option)
+			return option;
+	}
+	if (overload & 2) {
+		option = do_get_option(packet->sname, sizeof packet->sname,
+							   code, &overload, optlen);
+		if (option)
+			return option;
+		if (!parsed_ff && overload & 1)
+			option = do_get_option(packet->file, sizeof packet->file,
+								   code, &overload, optlen);
+	}
+	return option;
+}
+
 /* return the position of the 'end' option */
 int end_option(uint8_t *optionptr)
 {

ndhc/options.h

@@ -79,7 +79,7 @@ unsigned char *alloc_option(unsigned char code, unsigned char *optdata,
 unsigned char *alloc_dhcp_client_id_option(unsigned char type,
 										   unsigned char *idstr, size_t idstrlen);
 
-uint8_t *get_option(struct dhcpMessage *packet, int code);
+uint8_t *get_option(struct dhcpMessage *packet, int code, ssize_t *optlen);
 int end_option(uint8_t *optionptr);
 int add_option_string(unsigned char *optionptr, unsigned char *string);
 int add_simple_option(unsigned char *optionptr, unsigned char code, uint32_t data);

ndhc/packet.c

@@ -207,9 +207,10 @@ static void init_selecting_packet(struct client_state_t *cs,
                                   unsigned char *message)
 {
     unsigned char *temp = NULL;
+    ssize_t optlen;
     /* Must be a DHCPOFFER to one of our xid's */
     if (*message == DHCPOFFER) {
-        if ((temp = get_option(packet, DHCP_SERVER_ID))) {
+        if ((temp = get_option(packet, DHCP_SERVER_ID, &optlen))) {
             /* Memcpy to a temp buffer to force alignment */
             memcpy(&cs->serverAddr, temp, 4);
             cs->xid = packet->xid;
@@ -230,8 +231,9 @@ static void dhcp_ack_or_nak_packet(struct client_state_t *cs,
                                    unsigned char *message)
 {
     unsigned char *temp = NULL;
+    ssize_t optlen;
     if (*message == DHCPACK) {
-        if (!(temp = get_option(packet, DHCP_LEASE_TIME))) {
+        if (!(temp = get_option(packet, DHCP_LEASE_TIME, &optlen))) {
             log_line("No lease time received, assuming 1h.");
             cs->lease = 60 * 60;
         } else {
@@ -268,6 +270,7 @@ void handle_packet(struct client_state_t *cs)
     unsigned char *message = NULL;
     int len;
     struct dhcpMessage packet;
+    ssize_t optlen;
 
     if (cs->listenMode == LM_KERNEL)
         len = get_packet(&packet, cs->listenFd);
@@ -290,7 +293,7 @@ void handle_packet(struct client_state_t *cs)
         return;
     }
 
-    if ((message = get_option(&packet, DHCP_MESSAGE_TYPE)) == NULL) {
+    if ((message = get_option(&packet, DHCP_MESSAGE_TYPE, &optlen)) == NULL) {
         log_line("couldnt get option from packet -- ignoring");
         return;
     }