udhcp: dname_dec may return NULL, account for that case

Other random cleanips included...

Signed-off-by: Denys Vlasenko <vda.linux@googlemail.com>

5 files changed, 66 insertions, 58 deletions

diff --git a/networking/udhcp/clientpacket.c b/networking/udhcp/clientpacket.c
index 21c1a7b..84a6765 100644
--- a/networking/udhcp/clientpacket.c
+++ b/networking/udhcp/clientpacket.c
@@ -166,7 +166,7 @@ int FAST_FUNC send_select(uint32_t xid, uint32_t server, uint32_t requested)
 }
 
 
-/* Unicasts or broadcasts a DHCP renew message */
+/* Unicast or broadcast a DHCP renew message */
 int FAST_FUNC send_renew(uint32_t xid, uint32_t server, uint32_t ciaddr)
 {
 	struct dhcp_packet packet;
@@ -186,7 +186,7 @@ int FAST_FUNC send_renew(uint32_t xid, uint32_t server, uint32_t ciaddr)
 }
 
 
-/* Unicasts a DHCP release message */
+/* Unicast a DHCP release message */
 int FAST_FUNC send_release(uint32_t server, uint32_t ciaddr)
 {
 	struct dhcp_packet packet;
diff --git a/networking/udhcp/domain_codec.c b/networking/udhcp/domain_codec.c
index 6f051c4..45354e7 100644
--- a/networking/udhcp/domain_codec.c
+++ b/networking/udhcp/domain_codec.c
@@ -25,16 +25,9 @@
  */
 char* FAST_FUNC dname_dec(const uint8_t *cstr, int clen, const char *pre)
 {
-	const uint8_t *c;
-	int crtpos, retpos, depth, plen = 0, len = 0;
+	char *ret = ret; /* for compiler */
 	char *dst = NULL;
 
-	if (!cstr)
-		return NULL;
-
-	if (pre)
-		plen = strlen(pre);
-
 	/* We make two passes over the cstr string. First, we compute
 	 * how long the resulting string would be. Then we allocate a
 	 * new buffer of the required length, and fill it in with the
@@ -42,59 +35,71 @@ char* FAST_FUNC dname_dec(const uint8_t *cstr, int clen, const char *pre)
 	 * having to deal with requiring callers to supply their own
 	 * buffer, then having to check if it's sufficiently large, etc.
 	 */
-
-	while (!dst) {
-
-		if (len > 0) {	/* second pass? allocate dst buffer and copy pre */
-			dst = xmalloc(len + plen);
-			memcpy(dst, pre, plen);
-		}
+	while (1) {
+		/* note: "return NULL" below are leak-safe since
+		 * dst isn't yet allocated */
+		const uint8_t *c;
+		unsigned crtpos, retpos, depth, len;
 
 		crtpos = retpos = depth = len = 0;
-
 		while (crtpos < clen) {
 			c = cstr + crtpos;
 
-			if ((*c & NS_CMPRSFLGS) != 0) {	/* pointer */
-				if (crtpos + 2 > clen)		/* no offset to jump to? abort */
+			if (*c & NS_CMPRSFLGS) {
+				/* pointer */
+				if (crtpos + 2 > clen) /* no offset to jump to? abort */
 					return NULL;
-				if (retpos == 0)			/* toplevel? save return spot */
+				if (retpos == 0) /* toplevel? save return spot */
 					retpos = crtpos + 2;
 				depth++;
-				crtpos = ((*c & 0x3f) << 8) | (*(c + 1) & 0xff); /* jump */
-			} else if (*c) {			/* label */
-				if (crtpos + *c + 1 > clen)		/* label too long? abort */
+				crtpos = ((c[0] & 0x3f) << 8) | (c[1] & 0xff); /* jump */
+			} else if (*c) {
+				/* label */
+				if (crtpos + *c + 1 > clen) /* label too long? abort */
 					return NULL;
 				if (dst)
-					memcpy(dst + plen + len, c + 1, *c);
+					memcpy(dst + len, c + 1, *c);
 				len += *c + 1;
 				crtpos += *c + 1;
 				if (dst)
-					*(dst + plen + len - 1) = '.';
-			} else {					/* null: end of current domain name */
-				if (retpos == 0) {			/* toplevel? keep going */
+					dst[len - 1] = '.';
+			} else {
+				/* null: end of current domain name */
+				if (retpos == 0) {
+					/* toplevel? keep going */
 					crtpos++;
-				} else {					/* return to toplevel saved spot */
+				} else {
+					/* return to toplevel saved spot */
 					crtpos = retpos;
 					retpos = depth = 0;
 				}
 				if (dst)
-					*(dst + plen + len - 1) = ' ';
+					dst[len - 1] = ' ';
 			}
 
-			if (depth > NS_MAXDNSRCH || /* too many jumps? abort, it's a loop */
-				len > NS_MAXDNAME * NS_MAXDNSRCH) /* result too long? abort */
+			if (depth > NS_MAXDNSRCH /* too many jumps? abort, it's a loop */
+			 || len > NS_MAXDNAME * NS_MAXDNSRCH /* result too long? abort */
+			) {
 				return NULL;
+			}
 		}
 
-		if (!len)			/* expanded string has 0 length? abort */
+		if (!len) /* expanded string has 0 length? abort */
 			return NULL;
 
-		if (dst)
-			*(dst + plen + len - 1) = '\0';
+		if (!dst) { /* first pass? */
+			/* allocate dst buffer and copy pre */
+			unsigned plen = strlen(pre);
+			ret = dst = xmalloc(plen + len);
+			memcpy(dst, pre, plen);
+			dst += plen;
+		} else {
+			dst[len - 1] = '\0';
+			break;
+		}
 	}
 
-	return dst;
+	return ret;
 }
 
 /* Convert a domain name (src) from human-readable "foo.blah.com" format into
diff --git a/networking/udhcp/options.c b/networking/udhcp/options.c
index 0f17feb..76d6e37 100644
--- a/networking/udhcp/options.c
+++ b/networking/udhcp/options.c
@@ -115,7 +115,7 @@ const uint8_t dhcp_option_lengths[] ALIGN1 = {
 	[OPTION_U16] =     2,
 	[OPTION_S16] =     2,
 	[OPTION_U32] =     4,
-	[OPTION_S32] =     4
+	[OPTION_S32] =     4,
 };
 
 
diff --git a/networking/udhcp/options.h b/networking/udhcp/options.h
index 8c80485..9e62431 100644
--- a/networking/udhcp/options.h
+++ b/networking/udhcp/options.h
@@ -19,11 +19,13 @@ enum {
 	OPTION_U16,
 	OPTION_S16,
 	OPTION_U32,
-	OPTION_S32
+	OPTION_S32,
 };
 
-#define OPTION_REQ      0x10 /* have the client request this option */
-#define OPTION_LIST     0x20 /* There can be a list of 1 or more of these */
+/* Client requests this option by default */
+#define OPTION_REQ      0x10
+/* There can be a list of 1 or more of these */
+#define OPTION_LIST     0x20
 
 /*****************************************************************/
 /* Do not modify below here unless you know what you are doing!! */
diff --git a/networking/udhcp/script.c b/networking/udhcp/script.c
index 7ebef35..94dabf4 100644
--- a/networking/udhcp/script.c
+++ b/networking/udhcp/script.c
@@ -14,7 +14,7 @@
 
 
 /* get a rough idea of how long an option will be (rounding up...) */
-static const uint8_t max_option_length[] = {
+static const uint8_t len_of_option_as_string[] = {
 	[OPTION_IP] =		sizeof("255.255.255.255 "),
 	[OPTION_IP_PAIR] =	sizeof("255.255.255.255 ") * 2,
 	[OPTION_STRING] =	1,
@@ -30,17 +30,10 @@ static const uint8_t max_option_length[] = {
 };
 
 
-static inline int upper_length(int length, int opt_index)
-{
-	return max_option_length[opt_index] *
-		(length / dhcp_option_lengths[opt_index]);
-}
-
-
 /* note: ip is a pointer to an IP in network order, possibly misaliged */
 static int sprint_nip(char *dest, const char *pre, const uint8_t *ip)
 {
-	return sprintf(dest, "%s%d.%d.%d.%d", pre, ip[0], ip[1], ip[2], ip[3]);
+	return sprintf(dest, "%s%u.%u.%u.%u", pre, ip[0], ip[1], ip[2], ip[3]);
 }
 
 
@@ -57,9 +50,10 @@ static int mton(uint32_t mask)
 }
 
 
-/* Allocate and fill with the text of option 'option'. */
-static char *alloc_fill_opts(uint8_t *option, const struct dhcp_option *type_p, const char *opt_name)
+/* Create "opt_name=opt_value" string */
+static char *xmalloc_optname_optval(uint8_t *option, const struct dhcp_option *type_p, const char *opt_name)
 {
+	unsigned upper_length;
 	int len, type, optlen;
 	uint16_t val_u16;
 	int16_t val_s16;
@@ -67,14 +61,16 @@ static char *alloc_fill_opts(uint8_t *option, const struct dhcp_option *type_p,
 	int32_t val_s32;
 	char *dest, *ret;
 
-	len = option[OPT_LEN - 2];
+	/* option points to OPT_DATA, need to go back and get OPT_LEN */
+	len = option[OPT_LEN - OPT_DATA];
 	type = type_p->flags & TYPE_MASK;
 	optlen = dhcp_option_lengths[type];
+	upper_length = len_of_option_as_string[type] * (len / optlen);
 
-	dest = ret = xmalloc(upper_length(len, type) + strlen(opt_name) + 2);
+	dest = ret = xmalloc(upper_length + strlen(opt_name) + 2);
 	dest += sprintf(ret, "%s=", opt_name);
 
-	for (;;) {
+	while (len >= optlen) {
 		switch (type) {
 		case OPTION_IP_PAIR:
 			dest += sprint_nip(dest, "", option);
@@ -114,15 +110,20 @@ static char *alloc_fill_opts(uint8_t *option, const struct dhcp_option *type_p,
 		case OPTION_STR1035:
 			/* unpack option into dest; use ret for prefix (i.e., "optname=") */
 			dest = dname_dec(option, len, ret);
-			free(ret);
-			return dest;
+			if (dest) {
+				free(ret);
+				return dest;
+			}
+			/* error. return "optname=" string */
+			return ret;
 #endif
 		}
 		option += optlen;
 		len -= optlen;
 		if (len <= 0)
 			break;
-		dest += sprintf(dest, " ");
+		*dest++ = ' ';
+		*dest = '\0';
 	}
 	return ret;
 }
@@ -174,7 +175,7 @@ static char **fill_envp(struct dhcp_packet *packet)
 		temp = get_option(packet, dhcp_options[i].code);
 		if (!temp)
 			goto next;
-		*curr = alloc_fill_opts(temp, &dhcp_options[i], opt_name);
+		*curr = xmalloc_optname_optval(temp, &dhcp_options[i], opt_name);
 		putenv(*curr++);
 
 		/* Fill in a subnet bits option for things like /24 */