Re: [lwip-devel] [bug #36492] Static Analysis on code 1.4.0

[Mason]

Mason wrote:

> bayard wrote:
> 
>> ABR : Buffer overflow, array index of 'hwaddr' may be out of bounds.
>> Array 'hwaddr' of size 6 may use index value(s) 6..15 :
>> lwip/src/core/dhcp.c : 1683 : Critical : Analyze
> 
> I assume line 1698 in the 1.4.x branch.
> 
>   for (i = 0; i < DHCP_CHADDR_LEN; i++) {
>     /* copy netif hardware address, pad with zeroes */
>     dhcp->msg_out->chaddr[i] = (i < netif->hwaddr_len) ? netif->hwaddr[i] : 
> 0/* pad byte*/;
>   }

Simon, Kieran,

I have a patch for dhcp_create_msg.

Instead of setting several fields to 0 (sometimes using loops
to clear arrays one byte (!!) at a time), I propose clearing
the whole struct, and then writing only non-zero fields.

Would you accept it?


--- dhcp.c.orig 2012-05-14 14:10:21.578125000 +0200
+++ dhcp.c      2012-05-23 12:35:20.880351400 +0200
@@ -1672,39 +1672,21 @@
               ("transaction id xid(%"X32_F")\n", xid));
 
   dhcp->msg_out = (struct dhcp_msg *)dhcp->p_out->payload;
+  memset(dhcp->msg_out, 0, sizeof *dhcp->msg_out);
 
   dhcp->msg_out->op = DHCP_BOOTREQUEST;
   /* TODO: make link layer independent */
   dhcp->msg_out->htype = DHCP_HTYPE_ETH;
   dhcp->msg_out->hlen = netif->hwaddr_len;
-  dhcp->msg_out->hops = 0;
   dhcp->msg_out->xid = htonl(dhcp->xid);
-  dhcp->msg_out->secs = 0;
-  /* we don't need the broadcast flag since we can receive unicast traffic
-     before being fully configured! */
-  dhcp->msg_out->flags = 0;
-  ip_addr_set_zero(&dhcp->msg_out->ciaddr);
   /* set ciaddr to netif->ip_addr based on message_type and state */
   if ((message_type == DHCP_INFORM) || (message_type == DHCP_DECLINE) ||
       ((message_type == DHCP_REQUEST) && /* DHCP_BOUND not used for sending! */
        ((dhcp->state==DHCP_RENEWING) || dhcp->state==DHCP_REBINDING))) {
     ip_addr_copy(dhcp->msg_out->ciaddr, netif->ip_addr);
   }
-  ip_addr_set_zero(&dhcp->msg_out->yiaddr);
-  ip_addr_set_zero(&dhcp->msg_out->siaddr);
-  ip_addr_set_zero(&dhcp->msg_out->giaddr);
-  for (i = 0; i < DHCP_CHADDR_LEN; i++) {
-    /* copy netif hardware address, pad with zeroes */
-    dhcp->msg_out->chaddr[i] = (i < netif->hwaddr_len) ? netif->hwaddr[i] : 
0/* pad byte*/;
-  }


-- 
Regards.