0000976: udhcpc doesn't work if built for 64bit an big endian

(0001554)
creese
08-01-06 09:48
edited on: 08-01-06 09:50

My last patch missed the lease time. It also needs to be a uint32_t instead of unsigned long on 64bit archs.

--- networking/udhcp/dhcpc.c.orig 2006-08-01 09:34:14.000000000 -0700
+++ networking/udhcp/dhcpc.c 2006-08-01 09:35:18.000000000 -0700
@@ -31,9 +31,9 @@
#include "signalpipe.h"

static int state;
-static unsigned long requested_ip; /* = 0 */
-static unsigned long server_addr;
-static unsigned long timeout;
+static uint32_t requested_ip; /* = 0 */
+static uint32_t server_addr;
+static uint32_t timeout;
static int packet_num; /* = 0 */
static int fd = -1;

@@ -138,7 +138,8 @@
{
        uint8_t *temp, *message;
        unsigned long t1 = 0, t2 = 0, xid = 0;
- unsigned long start = 0, lease;
+ unsigned long start = 0;
+ uint32_t lease;
        fd_set rfds;
        int retval;
        struct timeval tv;

(0001617)
vda
09-03-06 09:58

Well... what is the problem? _How exactly_ it doesn't work?

-static unsigned long timeout;
+static uint32_t timeout;

I'd understand if you turn it into uint, but uint32? Why?

(0001618)
creese
09-03-06 11:24

uint would work as well as uint32_t since they are the same for gcc. The basic problem is that a memcpy is used to read the fields out of the dhcp response. The existing code works fine for 32bit and 64bit little endian. For 64bit big endian, the memcpy put the data into the upper 32bits of the 64bit long. I used uint32_t to fix this since that makes it clear that these fields are defined as 32bit from the dhcp spec.

(0001622)
vda
09-07-06 09:30

Sorry. I do not fully understand, and would like to.
Let's take for example requested_ip.

# grep -r requested_ip . | grep -F .c:
./dhcpc.c:static unsigned long requested_ip; /* = 0 */
./dhcpc.c: temp_addr.s_addr = requested_ip;
./dhcpc.c: send_release(server_addr, requested_ip); /* unicast */
./dhcpc.c: requested_ip = inet_addr(optarg);
./dhcpc.c: send_discover(xid, requested_ip); /* broadcast */
./dhcpc.c: send_renew(xid, server_addr, requested_ip); /* unicast */
./dhcpc.c: else send_selecting(xid, server_addr, requested_ip); /* broadcast */
./dhcpc.c: send_renew(xid, server_addr, requested_ip); /* unicast */
./dhcpc.c: send_renew(xid, 0, requested_ip); /* broadcast */
./dhcpc.c: requested_ip = packet.yiaddr;
./dhcpc.c: requested_ip = packet.yiaddr;
./dhcpc.c: requested_ip = 0;

Which line is causing the problem with this variable?

(0001623)
vda
09-07-06 10:07

Committed a minimal fix. Should work now.
If you want to do more intrusive audit - it is welcomed. Thanks.

Issue History
Date Modified	Username	Field	Change
07-31-06 18:27	creese	New Issue
07-31-06 18:27	creese	Status	new => assigned
07-31-06 18:27	creese	Assigned To	=> BusyBox
07-31-06 18:27	creese	File Added: busybox-udhcpcd-64bit.patch
08-01-06 09:48	creese	Note Added: 0001554
08-01-06 09:50	creese	Note Edited: 0001554
09-03-06 09:58	vda	Note Added: 0001617
09-03-06 11:24	creese	Note Added: 0001618
09-07-06 09:30	vda	Note Added: 0001622
09-07-06 10:07	vda	Note Added: 0001623
09-07-06 10:08	vda	Status	assigned => closed
09-07-06 10:08	vda	Resolution	open => fixed

Notes
(0001554) creese 08-01-06 09:48 edited on: 08-01-06 09:50	My last patch missed the lease time. It also needs to be a uint32_t instead of unsigned long on 64bit archs. --- networking/udhcp/dhcpc.c.orig 2006-08-01 09:34:14.000000000 -0700 +++ networking/udhcp/dhcpc.c 2006-08-01 09:35:18.000000000 -0700 @@ -31,9 +31,9 @@ #include "signalpipe.h" static int state; -static unsigned long requested_ip; /* = 0 / -static unsigned long server_addr; -static unsigned long timeout; +static uint32_t requested_ip; / = 0 / +static uint32_t server_addr; +static uint32_t timeout; static int packet_num; / = 0 / static int fd = -1; @@ -138,7 +138,8 @@ { uint8_t temp, *message; unsigned long t1 = 0, t2 = 0, xid = 0; - unsigned long start = 0, lease; + unsigned long start = 0; + uint32_t lease; fd_set rfds; int retval; struct timeval tv;

(0001617) vda 09-03-06 09:58	Well... what is the problem? _How exactly_ it doesn't work? -static unsigned long timeout; +static uint32_t timeout; I'd understand if you turn it into uint, but uint32? Why?

(0001618) creese 09-03-06 11:24	uint would work as well as uint32_t since they are the same for gcc. The basic problem is that a memcpy is used to read the fields out of the dhcp response. The existing code works fine for 32bit and 64bit little endian. For 64bit big endian, the memcpy put the data into the upper 32bits of the 64bit long. I used uint32_t to fix this since that makes it clear that these fields are defined as 32bit from the dhcp spec.

(0001622) vda 09-07-06 09:30	Sorry. I do not fully understand, and would like to. Let's take for example requested_ip. # grep -r requested_ip . \| grep -F .c: ./dhcpc.c:static unsigned long requested_ip; /* = 0 / ./dhcpc.c: temp_addr.s_addr = requested_ip; ./dhcpc.c: send_release(server_addr, requested_ip); / unicast / ./dhcpc.c: requested_ip = inet_addr(optarg); ./dhcpc.c: send_discover(xid, requested_ip); / broadcast / ./dhcpc.c: send_renew(xid, server_addr, requested_ip); / unicast / ./dhcpc.c: else send_selecting(xid, server_addr, requested_ip); / broadcast / ./dhcpc.c: send_renew(xid, server_addr, requested_ip); / unicast / ./dhcpc.c: send_renew(xid, 0, requested_ip); / broadcast */ ./dhcpc.c: requested_ip = packet.yiaddr; ./dhcpc.c: requested_ip = packet.yiaddr; ./dhcpc.c: requested_ip = 0; Which line is causing the problem with this variable?

(0001623) vda 09-07-06 10:07	Committed a minimal fix. Should work now. If you want to do more intrusive audit - it is welcomed. Thanks.

Relationships