5 files changed, 66 insertions, 58 deletions

diff --git a/networking/udhcp/clientpacket.c b/networking/udhcp/clientpacket.c
index 21c1a7bd5..84a6765ee 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 6f051c4b0..45354e727 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 0f17feb2a..76d6e3737 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 8c80485be..9e624318f 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 7ebef3553..94dabf4d1 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 */