Created attachment 182197 [details] Contents of typical dhclient.leases.INTERFACE file If the DHCP server is set to hand out a long lease time (effectively infinite) then dhclient calculates an expiry time (and a renew time) in the past. Repeat-by: 1) set DHCP server to hand out a lease of (say) 2147483600 2) boot a relevant client running latest dhclient Observed: DHCP server assigns an IP address, and dhclient gets it. It doesn't appear on the interface, and dhclient tries continually to get another one (look in /var/log/messages). Also observed: /var/db/dhclient.leases.$INTERFACE contains a valid rebind date, but invalid renew and expire dates, in the past. It appears that any assigned address is immediately expired!
Are you using 386 (ia32) or ppc32, by any chance?
The DHCP lease time is a uint32, but expiry is stored as a time_t. On 32-bit Intel and PPC platforms, time_t is a (signed) int32. That (or a 32-bit int) is probably involved somewhere.
Confirmed that this is on 386 (ia32). This worked correctly in 11.0-RELEASE, but yesterday I updated to 11-STABLE and it broke. Looks like the recent change to handle large values - didn't!
Bob, thanks for including me. I've touched that code path, so I'll check tonight what's up with this.
I'll take this one.
Ok, I've confirmed that this is an issue through dnsmasq with a lease time of sudo dnsmasq --no-daemon --port=0 --dhcp-range=10.38.26.100,10.38.26.200,255.255.255.0,2147483600 --interface vboxnet4 renew 0 2025/11/2 17:57:36; rebind 0 2025/11/2 17:57:37; expire 3 1949/4/13 17:19:08; However, I think it is a compiler optimisation issue. I can 'fix' it by compiling with '-O0'. I think the compiler optimises away the if statements checking for overflow. Would you be so kind to try the attached patch/patched version of dhclient.c and let me know?
Created attachment 182240 [details] Patch for dhclient.c
Created attachment 182241 [details] patched version of dhclient.c
That's insane, I was chasing this for weeks. My connection was going up and down under high CPU load. I have even tried three different NICs because of this (bfe, xl, rl). My server has a static DHCP IP address from my router and writes this: > lease { > interface "bfe0"; > fixed-address 192.168.1.5; > option subnet-mask 255.255.255.0; > option routers 192.168.1.1; > option domain-name-servers 192.168.1.1; > option dhcp-lease-time -1; > option dhcp-message-type 5; > option dhcp-server-identifier 192.168.1.1; > option dhcp-renewal-time 2147483647; > option dhcp-rebinding-time -644245096; > renew 4 1915/4/8 02:55:45; > rebind 2 2038/1/19 03:14:07; > expire 6 1949/4/16 16:32:49; > } I am on: FreeBSD bsd1home 11.0-STABLE FreeBSD 11.0-STABLE #1 r317706: Wed May 3 18:24:43 CEST 2017 root@bsd1home:/usr/obj/usr/src/sys/BSD1HOME i386 The expiry time 1073741823 seconds. I will try the patch the next couple of days and let you know. The typedef of time_t likely won't change for i386 for 12, will it?
(In reply to Michael Osipov from comment #9) > The typedef of time_t likely won't change for i386 for 12, will it? No, IIRC it's unlikely it will ever change on i386 due to ABI considerations.
(In reply to Conrad Meyer from comment #10) Fair enough. I read you patch and you have restructured the code. Isn't that enough or do you still need to turn off optimization?
(In reply to Michael Osipov from comment #11) I don't have any patch, you want Nick :-). As far as I know it should fix the issue. It would be good if you can test it.
(In reply to Conrad Meyer from comment #12) Right, my fingers were to fast. Will report by the end of the week.
I've confirmed the bug, confirmed that it is defined as 'unspecified behaviour in the C spec.' After the original poster confirms the fix (take your time Bob!) I'll commit a fix into CURRENT, then STABLE after 2 weeks. Any confirmation that the patch fixes other people's issues as well would of course be appreciated.
I confirm that I have tested the patched version and the problem has gone! Thanks very much. Apologies for the delay in testing; I was in hospital for two days and then recovering after elective surgery.
Tested the patch, though it looks better. There is still an integer overflow in rebind: lease { interface "bfe0"; fixed-address 192.168.1.7; option subnet-mask 255.255.255.0; option routers 192.168.1.1; option domain-name-servers 192.168.1.1; option dhcp-lease-time -1; option dhcp-message-type 5; option dhcp-server-identifier 192.168.1.1; option dhcp-renewal-time 2147483647; option dhcp-rebinding-time -644245096; renew 2 2038/1/19 03:14:07; rebind 5 1996/12/6 21:30:54; expire 2 2038/1/19 03:14:07; } What harm will cause this for me?
I think that I made a mistake in that patch (reviewing on my phone so not sure). I will create a final version tonight and have you test that instead. Thanks for the feedback!
(In reply to Nick Hibma from comment #17) OK thanks but why did you then close the issue?
Why I closed the issue. Ah, that's called incompetence! (I must have hit the button accidentally on my phone).
Created attachment 182372 [details] Second patch for dhclient.c
Created attachment 182373 [details] Second patched version for dhclient.c
New patch attached to this bug which should no longer suffer from any integer overflows. Please test and let me know.
Looks way better now: lease { interface "bfe0"; fixed-address 192.168.1.7; option subnet-mask 255.255.255.0; option routers 192.168.1.1; option domain-name-servers 192.168.1.1; option dhcp-lease-time -1; option dhcp-message-type 5; option dhcp-server-identifier 192.168.1.1; option dhcp-renewal-time 2147483647; option dhcp-rebinding-time -644245096; renew 1 2027/9/13 11:15:01; rebind 1 2035/6/18 23:14:17; expire 2 2038/1/19 03:14:07; } But dhcp-rebinding-time still looks wrong, manpage says: "option dhcp-rebinding-time uint32;". I think it should rather be 1503238551.
This look wrong indeed. It is the literal value as found in the packet received from the server though, and that might overflow the time_t while being printed I am going to commit the fix as it is though for now as it fixes dhclient's erratic behaviour for large values as stated in the PR.
A commit references this bug: Author: n_hibma Date: Sun May 7 19:59:38 UTC 2017 New revision: 317915 URL: https://svnweb.freebsd.org/changeset/base/317915 Log: Fix handling of large DHCP expiry values. They would overflow a signed 32-bit time_t on 32 bit architectures. This was taken care of, but a compiler optimisation makes this behave erratically. This could be resolved by adding a -fwrapv flag, but instead we can check the value before adding the current timestamp to it. In the lease file values are still wrong though: option dhcp-rebinding-time -644245096; PR: 218980 Reported by: Bob Eager MFC after: 2 weeks Changes: head/sbin/dhclient/dhclient.c
Created attachment 182375 [details] Patched version of options.c
Created attachment 182376 [details] Patch for options.c
Michael, could you try attached version of options.c / the patch? It fixes printing of unsigned long and unsigned short values. The formatting was correct, but the wrong printf format specification was being used.
(In reply to Nick Hibma from comment #28) Already on the way...
Patch applied: lease { interface "bfe0"; fixed-address 192.168.1.7; option subnet-mask 255.255.255.0; option routers 192.168.1.1; option domain-name-servers 192.168.1.1; option dhcp-lease-time 4294967295; option dhcp-message-type 5; option dhcp-server-identifier 192.168.1.1; option dhcp-renewal-time 2147483647; option dhcp-rebinding-time 3650722200; renew 1 2027/9/13 11:53:23; rebind 1 2035/6/18 23:23:56; expire 2 2038/1/19 03:14:07; } Looks good now. Go ahead and commit it.
Comitted in rev. 317915 (forgot to mention the PR no in the svn log, sorry). Will be MFC-ed in two weeks time. I'll close the bug report after that.
A commit references this bug: Author: n_hibma Date: Mon May 22 10:28:17 UTC 2017 New revision: 318630 URL: https://svnweb.freebsd.org/changeset/base/318630 Log: MFC: ------------------------------------------------------------------------ r317923 | n_hibma | 2017-05-07 23:11:28 +0200 (Sun, 07 May 2017) | 8 lines Fix the output of very large rebind, renew and lease time options in lease file. Some routers set very large values for rebind time (Netgear) and these are erroneously reported as negative in the leasefile. This was due to a wrong printf format specification of %ld for an unsigned long on 32-bit platforms. ------------------------------------------------------------------------ r317915 | n_hibma | 2017-05-07 21:59:37 +0200 (Sun, 07 May 2017) | 16 lines Fix handling of large DHCP expiry values. They would overflow a signed 32-bit time_t on 32 bit architectures. This was taken care of, but a compiler optimisation makes this behave erratically. This could be resolved by adding a -fwrapv flag, but instead we can check the value before adding the current timestamp to it. In the lease file values are still wrong though: option dhcp-rebinding-time -644245096; PR: 218980 Changes: stable/11/sbin/dhclient/dhclient.c stable/11/sbin/dhclient/options.c
Committed to current and MFC-ed. If there are any remaining issues w.r.t. time in dhclient and time_t, please open a new bug report and feel free to include me in the CC: list.