* DHCP option parsing bug
@ 2009-09-17 23:08 Jay Foster
2009-09-18 14:49 ` Gary Thomas
0 siblings, 1 reply; 4+ messages in thread
From: Jay Foster @ 2009-09-17 23:08 UTC (permalink / raw)
To: eCos Patches; +Cc: Jay Foster
[-- Attachment #1: Type: text/plain, Size: 417 bytes --]
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.
Jay
[-- Attachment #2: dhcp.pat --]
[-- Type: text/plain, Size: 7942 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 17 Sep 2009 23:00:48 -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 17 Sep 2009 23:00:49 -0000
@@ -669,11 +669,11 @@ do_dhcp(const char *intf, struct bootp *
struct timeval tv;
struct timeout_state timeout_scratch;
cyg_uint8 oldstate = *pstate;
cyg_uint8 msgtype = 0, seen_bootp_reply = 0;
unsigned int length;
-
+ int pktlen;
cyg_uint32 xid;
#define CHECK_XID() ( /* and other details */ \
received->bp_xid != xid || /* not the same transaction */ \
received->bp_htype != xmit->bp_htype || /* not the same ESA type */ \
@@ -845,12 +845,13 @@ do_dhcp(const char *intf, struct bootp *
// 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) {
+ pktlen = recvfrom(s, received, sizeof(struct bootp), 0,
+ (struct sockaddr *)&rx_addr, &addrlen);
+ if (pktlen < 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;
@@ -864,10 +865,17 @@ do_dhcp(const char *intf, struct bootp *
break;
}
*pstate = DHCPSTATE_INIT; // to retransmit
break;
}
+ /* Some DHCP servers don't terminate the options list with
+ * an END tag. Append one if we can.
+ */
+ if (pktlen < sizeof(struct bootp))
+ {
+ ((unsigned char *)received)[pktlen] = TAG_END;
+ }
// Check for well-formed packet with correct termination (not truncated)
length = dhcp_size( received );
#ifdef CYGDBG_NET_DHCP_CHATTER
diag_printf( "---------DHCPSTATE_SELECTING received:\n" );
if ( length <= 0 )
@@ -954,21 +962,29 @@ do_dhcp(const char *intf, struct bootp *
// 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) {
+ pktlen = recvfrom(s, received, sizeof(struct bootp), 0,
+ (struct sockaddr *)&rx_addr, &addrlen);
+ if (pktlen < 0) {
// No packet arrived
// go to the next larger timeout and re-send:
if ( ! next_timeout( &tv, &timeout_scratch ) ) {
*pstate = DHCPSTATE_FAILED;
break;
}
*pstate = DHCPSTATE_REQUESTING;
break;
}
+ /* Some DHCP servers don't terminate the options list with
+ * an END tag. Append one if we can.
+ */
+ if (pktlen < sizeof(struct bootp))
+ {
+ ((unsigned char *)received)[pktlen] = TAG_END;
+ }
// Check for well-formed packet with correct termination (not truncated)
length = dhcp_size( received );
#ifdef CYGDBG_NET_DHCP_CHATTER
diag_printf( "---------DHCPSTATE_REQUEST_RECV received:\n" );
if ( length <= 0 )
@@ -1100,12 +1116,13 @@ do_dhcp(const char *intf, struct bootp *
// 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) {
+ pktlen = recvfrom(s, received, sizeof(struct bootp), 0,
+ (struct sockaddr *)&rx_addr, &addrlen);
+ if (pktlen < 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
@@ -1115,10 +1132,17 @@ do_dhcp(const char *intf, struct bootp *
break;
}
*pstate = DHCPSTATE_RENEWING;
break;
}
+ /* Some DHCP servers don't terminate the options list with
+ * an END tag. Append one if we can.
+ */
+ if (pktlen < sizeof(struct bootp))
+ {
+ ((unsigned char *)received)[pktlen] = TAG_END;
+ }
// Check for well-formed packet with correct termination (not truncated)
length = dhcp_size( received );
#ifdef CYGDBG_NET_DHCP_CHATTER
diag_printf( "---------DHCPSTATE_RENEW_RECV received:\n" );
if ( length <= 0 )
@@ -1208,12 +1232,13 @@ do_dhcp(const char *intf, struct bootp *
// 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) {
+ pktlen = recvfrom(s, received, sizeof(struct bootp), 0,
+ (struct sockaddr *)&rx_addr, &addrlen);
+ if (pktlen < 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
@@ -1223,10 +1248,17 @@ do_dhcp(const char *intf, struct bootp *
break;
}
*pstate = DHCPSTATE_REBINDING;
break;
}
+ /* Some DHCP servers don't terminate the options list with
+ * an END tag. Append one if we can.
+ */
+ if (pktlen < sizeof(struct bootp))
+ {
+ ((unsigned char *)received)[pktlen] = TAG_END;
+ }
// Check for well-formed packet with correct termination (not truncated)
length = dhcp_size( received );
#ifdef CYGDBG_NET_DHCP_CHATTER
diag_printf( "---------DHCPSTATE_REBIND_RECV received:\n" );
if ( length <= 0 )
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: DHCP option parsing bug
2009-09-17 23:08 DHCP option parsing bug Jay Foster
@ 2009-09-18 14:49 ` Gary Thomas
2009-09-22 16:29 ` Jay Foster
0 siblings, 1 reply; 4+ messages in thread
From: Gary Thomas @ 2009-09-18 14:49 UTC (permalink / raw)
To: Jay Foster; +Cc: eCos Patches
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?
--
------------------------------------------------------------
Gary Thomas | Consulting for the
MLB Associates | Embedded world
------------------------------------------------------------
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: DHCP option parsing bug
2009-09-18 14:49 ` Gary Thomas
@ 2009-09-22 16:29 ` Jay Foster
2009-09-23 14:29 ` Gary Thomas
0 siblings, 1 reply; 4+ messages in thread
From: Jay Foster @ 2009-09-22 16:29 UTC (permalink / raw)
To: Gary Thomas; +Cc: eCos Patches
[-- 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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: DHCP option parsing bug
2009-09-22 16:29 ` Jay Foster
@ 2009-09-23 14:29 ` Gary Thomas
0 siblings, 0 replies; 4+ messages in thread
From: Gary Thomas @ 2009-09-23 14:29 UTC (permalink / raw)
To: Jay Foster; +Cc: eCos Patches
On 09/22/2009 10:29 AM, Jay Foster wrote:
> 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
>
Nicely done, checked in.
Thanks
--
------------------------------------------------------------
Gary Thomas | Consulting for the
MLB Associates | Embedded world
------------------------------------------------------------
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2009-09-23 14:29 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-09-17 23:08 DHCP option parsing bug Jay Foster
2009-09-18 14:49 ` Gary Thomas
2009-09-22 16:29 ` Jay Foster
2009-09-23 14:29 ` Gary Thomas
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).