udhcp: dname_dec may return NULL, account for that case

Other random cleanips included...

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

diff --git a/networking/udhcp/clientpacket.c b/networking/udhcp/clientpacket.c
index 21c1a7bd56a15987ee70aa17f66cd051797eba4d..84a6765ee2eccb02c5306824bbb3df0f88c81e08 100644 (file)
--- 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 6f051c4b051846db498531076fcc6180369dc228..45354e727181c365bd51739b1c9027c0370e239f 100644 (file)
--- 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 0f17feb2a9fac3762e336951d59f335fb6a2e5be..76d6e373703e91bc33a4229ea716dc5889bea6bb 100644 (file)
--- 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 8c80485be3c9516a7e114578fd53d83f12aefbf1..9e624318f6b6e23dc79a62a8c5746cefb934609b 100644 (file)
--- 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 7ebef355394e3823b51c49c185b124269e6f4ab0..94dabf4d17058a6637d473041a773b5ffd5badf1 100644 (file)
--- 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 */