Re: DHCP option parsing bug

[Jay Foster <jay@systech.com>]

[-- Attachment #1: Type: text/plain, Size: 675 bytes --]

On 9/18/2009 7:49 AM, Gary Thomas wrote:
> On 09/17/2009 05:08 PM, Jay Foster wrote:
>> The attached patch fixes the DHCP option parsing to deal with DHCP
>> servers that don't terminate their options with an END tag. The eCos
>> implementation relies heavily on this. RFC 2132 doesn't specify if this
>> END tag is required or not. Up until now, I have never seen a DHCP
>> server do this, but have run into now.
>>
>> There are other ways to implement this, but I chose this approach as the
>> least invasive.
>
> How about abstracting the 'recvfrom()+test+repair' sequence
> to a function, since there are _four_ identical occurrences?
>
Attached is a revised patch.
Jay


[-- Attachment #2: dhcp.pat --]
[-- Type: text/plain, Size: 6427 bytes --]

Index: ecos/packages/net/common/current/ChangeLog
===================================================================
RCS file: /cvs/ecos/ecos-opt/net/net/common/current/ChangeLog,v
retrieving revision 1.86
diff -u -5 -p -r1.86 ChangeLog
--- ecos/packages/net/common/current/ChangeLog	3 Jul 2009 12:40:25 -0000	1.86
+++ ecos/packages/net/common/current/ChangeLog	22 Sep 2009 16:21:10 -0000
@@ -1,5 +1,10 @@
+2009-09-17	Jay Foster <jay@systech.com>
+
+	* src/dhcp_prot.c (do_dhcp): Fix packet parsing for DHCP servers
+	that do not terminate the options with an END tag.
+
 2009-06-23  Rene Schipp von Branitz Nielsen <rbn@vitesse.com>
 
 	* src/ifaddrs.c (getifaddrs): If socket() call for IPv6 fails,
 	a stray pointer was freed.
 
Index: ecos/packages/net/common/current/src/dhcp_prot.c
===================================================================
RCS file: /cvs/ecos/ecos-opt/net/net/common/current/src/dhcp_prot.c,v
retrieving revision 1.21
diff -u -5 -p -r1.21 dhcp_prot.c
--- ecos/packages/net/common/current/src/dhcp_prot.c	29 Jan 2009 17:49:57 -0000	1.21
+++ ecos/packages/net/common/current/src/dhcp_prot.c	22 Sep 2009 16:21:11 -0000
@@ -652,20 +652,48 @@ static void set_default_dhcp_tags( struc
     // Explicitly specify our max message size.
     set_fixed_tag( xmit, TAG_DHCP_MAX_MSGSZ, BP_MINPKTSZ, 2 );
 }
 
 // ------------------------------------------------------------------------
+// Get BOOTP/DHCP response.
+// Wait up to the amount of time specified by *tvp.
+
+static int
+get_response(int s, struct bootp *response, struct sockaddr_in *from, struct timeval *tvp)
+{
+    int pktlen;
+    socklen_t addrlen;
+
+    setsockopt(s, SOL_SOCKET, SO_RCVTIMEO, tvp, sizeof(*tvp));
+
+    addrlen = sizeof(*from);
+    pktlen = recvfrom(s, response, sizeof(*response), 0, (struct sockaddr *)from, &addrlen);
+    /* Some DHCP servers don't terminate the options list with
+     * an END tag.  Append one if we can.
+     */
+    if ((pktlen >= 0) && (pktlen < sizeof(*response)))
+    {
+        /* Do not count the added END tag in the returned packet length.
+         * The returned packet length is the number of bytes received
+         * from the DHCP server.  The added END tag is only used
+         * internally for packet parsing purposes.
+         */
+        ((unsigned char *)response)[pktlen] = TAG_END;
+    }
+    return pktlen;
+}
+
+// ------------------------------------------------------------------------
 // the DHCP state machine - this does all the work
 
 int
 do_dhcp(const char *intf, struct bootp *res,
         cyg_uint8 *pstate, struct dhcp_lease *lease)
 {
     struct ifreq ifr;
     struct sockaddr_in cli_addr, broadcast_addr, server_addr, rx_addr;
     int s = -1;
-    socklen_t addrlen;
     int one = 1;
     unsigned char mincookie[] = {99,130,83,99,255} ;
     struct timeval tv;
     struct timeout_state timeout_scratch;
     cyg_uint8 oldstate = *pstate;
@@ -842,15 +870,11 @@ do_dhcp(const char *intf, struct bootp *
             // This is a separate state so that we can listen again
             // *without* retransmitting.
             
             // listen for the DHCPOFFER reply
 
-            setsockopt(s, SOL_SOCKET, SO_RCVTIMEO, &tv, sizeof(tv));
-
-            addrlen = sizeof(rx_addr);
-            if (recvfrom(s, received, sizeof(struct bootp), 0,
-                         (struct sockaddr *)&rx_addr, &addrlen) < 0) {
+            if (get_response(s, received, &rx_addr, &tv) < 0) {
                 // No packet arrived (this time)
                 if ( seen_bootp_reply ) { // then already have a bootp reply
                     // Save the good packet in *xmit
                     bcopy( received, xmit, dhcp_size(received) );
                     *pstate = DHCPSTATE_BOOTP_FALLBACK;
@@ -951,15 +975,11 @@ do_dhcp(const char *intf, struct bootp *
 
         case DHCPSTATE_REQUEST_RECV:
             // wait for an ACK or a NACK - retry by going back to
             // DHCPSTATE_REQUESTING; NACK means go back to INIT.
 
-            setsockopt(s, SOL_SOCKET, SO_RCVTIMEO, &tv, sizeof(tv));
-
-            addrlen = sizeof(rx_addr);
-            if (recvfrom(s, received, sizeof(struct bootp), 0,
-                         (struct sockaddr *)&rx_addr, &addrlen) < 0) {
+            if (get_response(s, received, &rx_addr, &tv) < 0) {
                 // No packet arrived
                 // go to the next larger timeout and re-send:
                 if ( ! next_timeout( &tv, &timeout_scratch ) ) {
                     *pstate = DHCPSTATE_FAILED;
                     break;
@@ -1097,15 +1117,11 @@ do_dhcp(const char *intf, struct bootp *
         case DHCPSTATE_RENEW_RECV:
             // wait for an ACK or a NACK - retry by going back to
             // DHCPSTATE_RENEWING; NACK means go to NOTBOUND.
             // No answer means just wait for T2, to broadcast.
 
-            setsockopt(s, SOL_SOCKET, SO_RCVTIMEO, &tv, sizeof(tv));
-
-            addrlen = sizeof(rx_addr);
-            if (recvfrom(s, received, sizeof(struct bootp), 0,
-                         (struct sockaddr *)&rx_addr, &addrlen) < 0) {
+            if (get_response(s, received, &rx_addr, &tv) < 0) {
                 // No packet arrived
                 // go to the next larger timeout and re-send:
                 if ( ! next_timeout( &tv, &timeout_scratch ) ) {
                     // If we timed out completely, just give up until T2
                     // expires - retain the lease meanwhile.  The normal
@@ -1205,15 +1221,11 @@ do_dhcp(const char *intf, struct bootp *
         case DHCPSTATE_REBIND_RECV:
             // wait for an ACK or a NACK - retry by going back to
             // DHCPSTATE_REBINDING; NACK means go to NOTBOUND.
             // No answer means just wait for expiry; we tried!
 
-            setsockopt(s, SOL_SOCKET, SO_RCVTIMEO, &tv, sizeof(tv));
-
-            addrlen = sizeof(rx_addr);
-            if (recvfrom(s, received, sizeof(struct bootp), 0,
-                         (struct sockaddr *)&rx_addr, &addrlen) < 0) {
+            if (get_response(s, received, &rx_addr, &tv) < 0) {
                 // No packet arrived
                 // go to the next larger timeout and re-send:
                 if ( ! next_timeout( &tv, &timeout_scratch ) ) {
                     // If we timed out completely, just give up until EX
                     // expires - retain the lease meanwhile.  The normal