Re: [PATCH] udhcpd: Handle auto_time timeout overflow
On Tue, Jan 27, 2015 at 04:51:33PM -0500, Cathey, Jim wrote: > At the bottom, some of C's arithmetical rules > are 'stupid'. Especially as regards type > promotion. At least they're well-defined. No, unsigned types are just modular arithmetic. There's nothing 'stupid' or unexpected about how they behave unless you're not aware of that, at least not any more stupid than = and == (and even === in some langs :) having different meanings or left-shifting "cout" in C++... > Absolutely true in a mathematical sense is that > the difference between two unsigned numbers is > SIGNED! It depends on what number system you're working in. Objects of a fixed size cannot represent the integers so you have to either pick a number system that can be represented (C's unsigned types) or work with an approximation of the integers (C's signed types). > But that's not what C does. You can get > around this, easy enough, but you do have to > understand exactly what is going on. It helps > if you are working on a 2's-complement machine. Twos complement is irrelevant to the operation of unsigned arithmetic. Rich ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH] udhcpd: Handle auto_time timeout overflow
On Tue, Jan 27, 2015 at 09:51:29PM +0100, Denys Vlasenko wrote: > Hmm, I think it's a sign-extension bug. Can you try replacing > > tv.tv_sec = timeout_end - monotonic_sec(); > > with > > tv.tv_sec = (int)(timeout_end - monotonic_sec()); > > I suspect this will fix the behavior. In fact, I'm going to smack myself now for not having noticed this earlier on... I noticed also that Denys has already committed a patch for this, so I'm not going to drag this thread out any longer than it already is. Thanks all! Tim pgp3LcCVigjMj.pgp Description: PGP signature ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH] udhcpd: Handle auto_time timeout overflow
El martes, 27 de enero de 2015, Cathey, Jim escribió: > Because mathematically, differences are *always* signed! > > > > 2 - 1 == 1 > > 1 - 2 == -1 > In modulo arithmetic , addition and substraction are the same for both unsigned and signed operands. Assume you have two 32-bit operands a and b. Let's assume a=1 and b=2. The result of a-b is always 0x regardless of whether a and b are signed or unsigned. Signedness *will* be relevant when you need to compare with another value: If the operand is unsigned, 0x > 0 If the operand is signed, 0x (-1) < 0 Guillermo -- Guillermo Rodriguez Garcia guille.rodrig...@gmail.com ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
RE: [PATCH] udhcpd: Handle auto_time timeout overflow
Oh, and carry bits. Addition/subtraction are also precision-changing! -- Jim From: busybox [mailto:busybox-boun...@busybox.net] On Behalf Of Cathey, Jim Sent: Tuesday, January 27, 2015 2:19 PM To: Guillermo Rodriguez Garcia; Tim Hentenaar Cc: busybox; Rich Felker Subject: RE: [PATCH] udhcpd: Handle auto_time timeout overflow Because mathematically, differences are always signed! 2 - 1 == 1 1 - 2 == -1 Doesn't matter what the size, or sign-ability of the LHS operands are. C, however, absolutely stinks at mathematical relations that change the nature of the calculation. Precision changing (like integer multiplication and division), sign invention (like subtraction), etc. You can work with it, but you must understand what is going on. -- Jim From: busybox [mailto:busybox-boun...@busybox.net] On Behalf Of Guillermo Rodriguez Garcia Sent: Tuesday, January 27, 2015 2:10 PM To: Tim Hentenaar Cc: busybox; Rich Felker Subject: Re: [PATCH] udhcpd: Handle auto_time timeout overflow El martes, 27 de enero de 2015, Tim Hentenaar mailto:t...@hentenaar.com>> escribió: Perhaps it wrongly assumes that since the operands for the subtraction are 32-bit unsigned integers, that the result will be also unsigned. Uhm, why would that be a wrong assumption ? Guillermo -- Guillermo Rodriguez Garcia guille.rodrig...@gmail.com<mailto:guille.rodrig...@gmail.com> ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
RE: [PATCH] udhcpd: Handle auto_time timeout overflow
Because mathematically, differences are always signed! 2 - 1 == 1 1 - 2 == -1 Doesn't matter what the size, or sign-ability of the LHS operands are. C, however, absolutely stinks at mathematical relations that change the nature of the calculation. Precision changing (like integer multiplication and division), sign invention (like subtraction), etc. You can work with it, but you must understand what is going on. -- Jim From: busybox [mailto:busybox-boun...@busybox.net] On Behalf Of Guillermo Rodriguez Garcia Sent: Tuesday, January 27, 2015 2:10 PM To: Tim Hentenaar Cc: busybox; Rich Felker Subject: Re: [PATCH] udhcpd: Handle auto_time timeout overflow El martes, 27 de enero de 2015, Tim Hentenaar mailto:t...@hentenaar.com>> escribió: Perhaps it wrongly assumes that since the operands for the subtraction are 32-bit unsigned integers, that the result will be also unsigned. Uhm, why would that be a wrong assumption ? Guillermo -- Guillermo Rodriguez Garcia guille.rodrig...@gmail.com<mailto:guille.rodrig...@gmail.com> ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH] udhcpd: Handle auto_time timeout overflow
El martes, 27 de enero de 2015, Tim Hentenaar escribió: > Perhaps it wrongly assumes that since the operands for the subtraction > are 32-bit unsigned integers, that the result will be also unsigned. Uhm, why would that be a wrong assumption ? Guillermo -- Guillermo Rodriguez Garcia guille.rodrig...@gmail.com ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
RE: [PATCH] udhcpd: Handle auto_time timeout overflow
At the bottom, some of C's arithmetical rules are 'stupid'. Especially as regards type promotion. At least they're well-defined. Absolutely true in a mathematical sense is that the difference between two unsigned numbers is SIGNED! But that's not what C does. You can get around this, easy enough, but you do have to understand exactly what is going on. It helps if you are working on a 2's-complement machine. Off the cuff, I think this works: unsigned int a, b; int delta; ... delta = (int)a - (int)b; if (delta < 0) delta = (int)b - (int)a; -or maybe- if (delta < 0) delta = -delta; -- Jim ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH] udhcpd: Handle auto_time timeout overflow
On Tue, Jan 27, 2015 at 10:28:26PM +0100, Tim Hentenaar wrote: > On Tue, Jan 27, 2015 at 09:51:29PM +0100, Denys Vlasenko wrote: > > Hmm, I think it's a sign-extension bug. Can you try replacing > > > > tv.tv_sec = timeout_end - monotonic_sec(); > > > > with > > > > tv.tv_sec = (int)(timeout_end - monotonic_sec()); > > > > I suspect this will fix the behavior. > > When I make that change, I get: > > movq$0, -872(%rbp) #, tv.tv_usec > subl%eax, %ecx # D.8486, D.8486 > testl %r14d, %r14d# > movslq %ecx, %rax # D.8486, > movq%rax, -880(%rbp)# D.8494, tv.tv_sec > je .L101 #, > testq %rax, %rax # D.8494 > jle .L192 #, > > Hmm... Looking at the assembly before the change, it's moving eax -> edx > instead of sign-extending, while here (with the explicit cast) it > sign-extends the result. It then generates the proper jump instruction > to boot. > > Perhaps it wrongly assumes that since the operands for the subtraction > are 32-bit unsigned integers, that the result will be also unsigned. Then, > the sign-extension gets optimized away. Then, making the cast explicit > forces gcc to sign-extend the result. This is not a wrong assumption. The result of unsigned subtraction is unsigned, and it's computed via modular arithmetic. The problem is that the semantics of the C code as written are wrong, not anything weird done by the compiler in translating it. Rich ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
RE: [PATCH] udhcpd: Handle auto_time timeout overflow
>Because we don't expect machines to run for 68 years without reboot. Certain nameless-but-large customers out in the world, running certain nameless Linux/busybox-based products, have experienced abject system failures after longer runtimes. Two years is one such number. These are generally sign-overflow types of bugs. Kernel bugs, libc bugs, etc. Some products are intended to run for years, if not decades, at a time. -- Jim ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH] udhcpd: Handle auto_time timeout overflow
On Tue, Jan 27, 2015 at 09:51:29PM +0100, Denys Vlasenko wrote: > Hmm, I think it's a sign-extension bug. Can you try replacing > > tv.tv_sec = timeout_end - monotonic_sec(); > > with > > tv.tv_sec = (int)(timeout_end - monotonic_sec()); > > I suspect this will fix the behavior. When I make that change, I get: movq$0, -872(%rbp) #, tv.tv_usec subl%eax, %ecx # D.8486, D.8486 testl %r14d, %r14d# movslq %ecx, %rax # D.8486, movq%rax, -880(%rbp)# D.8494, tv.tv_sec je .L101 #, testq %rax, %rax # D.8494 jle .L192 #, Hmm... Looking at the assembly before the change, it's moving eax -> edx instead of sign-extending, while here (with the explicit cast) it sign-extends the result. It then generates the proper jump instruction to boot. Perhaps it wrongly assumes that since the operands for the subtraction are 32-bit unsigned integers, that the result will be also unsigned. Then, the sign-extension gets optimized away. Then, making the cast explicit forces gcc to sign-extend the result. Tim pgpC1kEA4qcCz.pgp Description: PGP signature ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
RE: [PATCH] udhcpd: Handle auto_time timeout overflow
While 68 years does seem a stretch, who in their right mind wants to code implicit time-bombs into their stuff? A vendor wants to be able to say "I'm game if you are." (I.e., totally NOT a microsoft mentality! Which is to say, reboot every few days else it stops working right.) Handling monobased timer overflow is well understood, and has been since computing was invented. If the difference looks negative (a physical impossibility, so any such is an arithmetical artifact) you just flip it around and proceed with the (correct) positive difference. The only time this breaks down is if the true timeout interval approaches the wraparound time. Very few bits of code will try to schedule a non-calendrical event 68 years into the future! -- Jim ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH] udhcpd: Handle auto_time timeout overflow
We ran a 486 for some 5 years pretty much non stop in one of our server rooms. By the time we shut it off, the switch broke and later some other part just crumbled when we tried starting it back up. Had we left it alone, it probably would have continued running for many more years. - Original Message - From: "Denys Vlasenko" To: "Jim Cathey" Cc: "busybox" , "Rich Felker" Sent: Tuesday, January 27, 2015 2:13:32 PM Subject: Re: [PATCH] udhcpd: Handle auto_time timeout overflow On Tue, Jan 27, 2015 at 10:11 PM, Cathey, Jim wrote: >>Because we don't expect machines to run for 68 years without reboot. > > Certain nameless-but-large customers out in the world, running certain > nameless Linux/busybox-based products, have experienced abject system > failures after longer runtimes. Two years is one such number. I _sincerely_ doubt anyone in their right mind would insist on running a machine, without reboots, for 68 years. Years, not days. ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH] udhcpd: Handle auto_time timeout overflow
On Tue, Jan 27, 2015 at 10:11 PM, Cathey, Jim wrote: >>Because we don't expect machines to run for 68 years without reboot. > > Certain nameless-but-large customers out in the world, running certain > nameless Linux/busybox-based products, have experienced abject system > failures after longer runtimes. Two years is one such number. I _sincerely_ doubt anyone in their right mind would insist on running a machine, without reboots, for 68 years. Years, not days. ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH] udhcpd: Handle auto_time timeout overflow
On Tue, Jan 27, 2015 at 4:02 PM, Tim Hentenaar wrote:
> if (server_config.auto_time) {
> - tv.tv_sec = timeout_end - monotonic_sec();
> tv.tv_usec = 0;
> + tv.tv_sec = 0;
> +
> + msec = monotonic_sec();
> + if (msec < timeout_end)
> + tv.tv_sec = timeout_end - msec;
> }
This is in fact buggy, with a non-obvious reason.
monotonic_sec() returns a monotonically increasing second count,
_with unknown base_. It may well start counting from a value
close to overflowing, such as 0xff00.
Imagine that timeout_end = monotonic_sec() + timeout; happened to
overflow and evaluate to 0.
The (monotonic_sec() < timeout_end) condition after this will always be false,
which means that at *any* moment, code thinks that "timeout_end"
has been reached. Wrong.
if you want to ask "did we reach timeout_end?",
you should calculate (monotonic_sec() - timeout_end)
and look at its sign.
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH] udhcpd: Handle auto_time timeout overflow
>> + tv.tv_sec = 0; >> + >> + msec = monotonic_sec(); >> + if (msec < timeout_end) >> + tv.tv_sec = timeout_end - msec; > > Why are we using a 32-bit type for seconds in 2015? Because we don't expect machines to run for 68 years without reboot. We only ever use a difference between monotonic_sec() taken at two different moments in time. Such a difference overflows 2^31 after 68 years of uptime. ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH] udhcpd: Handle auto_time timeout overflow
Hmm, I think it's a sign-extension bug. Can you try replacing tv.tv_sec = timeout_end - monotonic_sec(); with tv.tv_sec = (int)(timeout_end - monotonic_sec()); I suspect this will fix the behavior. ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH] udhcpd: Handle auto_time timeout overflow
On Tue, Jan 27, 2015 at 04:02:16PM +0100, Tim Hentenaar wrote:
> On Tue, Jan 27, 2015 at 08:41:30AM -0500, Rich Felker wrote:
> > On Wed, Jan 21, 2015 at 11:00:03PM +0100, Tim Hentenaar wrote:
> > > ---
> > > networking/udhcp/dhcpd.c | 3 +++
> > > 1 file changed, 3 insertions(+)
> > >
> > > diff --git a/networking/udhcp/dhcpd.c b/networking/udhcp/dhcpd.c
> > > index 4b3ed24..d56763f 100644
> > > --- a/networking/udhcp/dhcpd.c
> > > +++ b/networking/udhcp/dhcpd.c
> > > @@ -415,6 +415,9 @@ int udhcpd_main(int argc UNUSED_PARAM, char **argv)
> > > if (server_config.auto_time) {
> > > tv.tv_sec = timeout_end - monotonic_sec();
> > > tv.tv_usec = 0;
> > > +
> > > + if ((unsigned)tv.tv_sec > server_config.auto_time)
> > > + tv.tv_sec = 0;
> >
> > I don't think this is a valid fix. If overflow occurs, there has
> > already been an invocation of undefined behavior (assuming it's
> > actually an overflow and not an implicit conversion into a signed type
> > from legal unsigned arithmetic, but in that case the result would
> > still be nonsense and might not look negative!). The check belongs
> > before the arithmetic that would invoke UB (or just give a
> > non-meaningful result) rather than after the computation.
> >
>
> Apologies for the double-response, but I revised the
> patch to use a stack variable for safety, and to save 5 bytes.
>
> To be honest, I think this particular bit of code could benefit from
> some refactoring...
>
> Better?
>
> ---
> networking/udhcp/dhcpd.c | 7 ++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/networking/udhcp/dhcpd.c b/networking/udhcp/dhcpd.c
> index 4b3ed24..75ddc12 100644
> --- a/networking/udhcp/dhcpd.c
> +++ b/networking/udhcp/dhcpd.c
> @@ -399,6 +399,7 @@ int udhcpd_main(int argc UNUSED_PARAM, char **argv)
> fd_set rfds;
> struct dhcp_packet packet;
> int bytes;
> + unsigned int msec;
> struct timeval tv;
> uint8_t *server_id_opt;
> uint8_t *requested_ip_opt;
> @@ -413,8 +414,12 @@ int udhcpd_main(int argc UNUSED_PARAM, char **argv)
>
> max_sock = udhcp_sp_fd_set(&rfds, server_socket);
> if (server_config.auto_time) {
> - tv.tv_sec = timeout_end - monotonic_sec();
> tv.tv_usec = 0;
> + tv.tv_sec = 0;
> +
> + msec = monotonic_sec();
> + if (msec < timeout_end)
> + tv.tv_sec = timeout_end - msec;
Why are we using a 32-bit type for seconds in 2015? msec should be
time_t, and monotonic_sec should be returning time_t, or they could
just use int64_t or something instead (but I feel like there's not
much point in supporting 64-bit time if the underlying system's time_t
isn't 64-bit anyway, and it's more costly, so time_t is probably
best).
Rich
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
Re: [RESEND] Re: [PATCH] udhcpd: Handle auto_time timeout overflow
On Tue, Jan 27, 2015 at 02:13:38PM +0100, Denys Vlasenko wrote:
> On Tue, Jan 27, 2015 at 1:28 PM, Tim Hentenaar wrote:
> > callmonotonic_sec #
> > movl24(%rsp), %ecx # %sfp, D.9844
> > movl52+bb_common_bufsiz1(%rip), %r14d # MEM[(struct
> > server_config_t * {ref-all})&bb_common_bufsiz1].auto_time,
> > movq$0, 88(%rsp)#, tv.tv_usec
> > subl%eax, %ecx # D.9836, D.9844
> > testl %r14d, %r14d#
> > movq%rcx, 80(%rsp) # D.9844, tv.tv_sec
> > je .L88#,
> > testq %rcx, %rcx # D.9844
> > je .L179 #,
> > leaq80(%rsp), %r8 #, iftmp.12
> > jmp .L87#
> >
> > Are you using a 32-bit x86 arch? If not, what do you get if you compile
> > it in 64-bit mode?
>
> With gcc 4.7.2:
>
> calludhcp_sp_fd_set #
> cmpl$0, bb_common_bufsiz1+52(%rip) #, MEM[(struct
> server_config_t * {ref-all})&bb_common_bufsiz1].auto_time
> movl%eax, %ebx #, max_sock
> je .L71#,
> callmonotonic_sec #
> movl24(%rsp), %edx # %sfp,
> movq$0, 56(%rsp)#, tv.tv_usec
> subl%eax, %edx # D.8343,
> movl%edx, %eax #,
> movq%rax, 48(%rsp) #, tv.tv_sec
> .L71:
> xorl%r8d, %r8d # iftmp.11
> cmpl$0, bb_common_bufsiz1+52(%rip) #, MEM[(struct
> server_config_t * {ref-all})&bb_common_bufsiz1].auto_time
> je .L72#,
> cmpq$0, 48(%rsp)#, tv.tv_sec
> jg .L110 #,
> jmp .L166 #
> .L110:
> leaq48(%rsp), %r8 #, iftmp.11
> .L72:
> leal1(%rbx), %edi #, tmp216
> leaq104(%rsp), %rsi #,
> xorl%ecx, %ecx #
> xorl%edx, %edx #
> callselect #
With gcc 4.7.4:
callmonotonic_sec #
movl16(%rsp), %edx # %sfp,
movl52+bb_common_bufsiz1(%rip), %r13d # MEM[(struct server_config_t *
{ref-all})&bb_common_bufsiz1].auto_time,
movq$0, 72(%rsp)#, tv.tv_usec
subl%eax, %edx # D.8069,
testl %r13d, %r13d#
movq%rdx, 64(%rsp) # D.8071, tv.tv_sec
je .L100 #,
testq %rdx, %rdx # D.8071
je .L196 #,
leaq64(%rsp), %r8 #, iftmp.13
jmp .L99#
With gcc 4.9.2:
callmonotonic_sec #
movl-948(%rbp), %edx# %sfp, D.8524
movl52+bb_common_bufsiz1(%rip), %r15d # MEM[(struct server_config_t *
{ref-all})&bb_common_bufsiz1].auto_time,
movq$0, -872(%rbp) #, tv.tv_usec
subl%eax, %edx # D.8513, D.8524
testl %r15d, %r15d#
movq%rdx, -880(%rbp)# D.8524, tv.tv_sec
je .L98#,
testq %rdx, %rdx # D.8524
je .L190 #,
leaq-880(%rbp), %r8 #, D.8510
jmp .L97#
Basically identical to what I got initially. I also double-checked that
the file was freshly regenerated each time. I'm at a loss to explain
gcc's choice of opcode in this case. I could be missing something,
though. I've attached the full .s file.
Here's the md5sum of the source file I'm using (from git rev.
9de69c024c7c47f3f8733dbc7c9522966fcd73a9), in case there's any
difference:
6497bc6e9038ad587e557a8ee59214d2 networking/udhcp/dhcpd.c
Tim
.file "dhcpd.c"
# GNU C (Gentoo Hardened 4.7.4 p1.2, pie-0.5.5) version 4.7.4 (x86_64-gentoo-linux-uclibc)
# compiled by GNU C version 4.7.4, GMP version 5.1.3, MPFR version 3.1.2, MPC version 1.0.1
# GGC heuristics: --param ggc-min-expand=30 --param ggc-min-heapsize=4096
# options passed: -I include -I libbb -D _GNU_SOURCE -D NDEBUG
# -D _LARGEFILE_SOURCE -D _LARGEFILE64_SOURCE -D _FILE_OFFSET_BITS=64
# -D BB_VER=KBUILD_STR(1.23.0) -D BB_BT=AUTOCONF_TIMESTAMP
# -D KBUILD_STR(s)=#s -D KBUILD_BASENAME=KBUILD_STR(dhcpd)
# -D KBUILD_MODNAME=KBUILD_STR(dhcpd) -include include/autoconf.h
# -MD networking/udhcp/.dhcpd.s.d networking/udhcp/dhcpd.c
# -fno-strict-overflow -mtune=generic -march=x86-64
# -auxbase-strip networking/udhcp/dhcpd.s -Os -O2 -Wall -Wshadow
# -Wwrite-strings -Wundef -Wstrict-prototypes -Wunused -Wunused-parameter
# -Wunused-function -Wunused-value -Wmissing-prototypes
# -Wmissing-declarations -Wno-format-security -Wdeclaration-after-statement
# -Wold-style-definition -std=gnu99 -fno-builtin-strlen -finline-limit=0
# -fomit-frame-pointer -ffunction-sections -fdata-sections
# -fno-guess-branch-probability -funsigned-char -falign-functions=1
# -falign-jumps=1 -falign-labels=1 -falign-loops=1 -fno-unwind-tables
# -fno-asynchronous-unwind-tables -fpie -fPIE -fverbose-asm
# -fstack-protector-all
# options enabled: -fPIC -fPIE -fauto-inc-dec -fbranch-count-reg
# -fcaller-saves -fcombine-stack-adjustments -fcommon -fcompare-elim
# -fcprop-registers -fcrossjumping -fcse-follow-jumps -fdata-sections
# -fdebug-types-section -fdefer-pop -fdelete-null-pointer-checks
# -fdevirtualize -fdwarf2-cfi-asm -fearly-inlinin
Re: [PATCH] udhcpd: Handle auto_time timeout overflow
On Tue, Jan 27, 2015 at 08:41:30AM -0500, Rich Felker wrote:
> On Wed, Jan 21, 2015 at 11:00:03PM +0100, Tim Hentenaar wrote:
> > ---
> > networking/udhcp/dhcpd.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/networking/udhcp/dhcpd.c b/networking/udhcp/dhcpd.c
> > index 4b3ed24..d56763f 100644
> > --- a/networking/udhcp/dhcpd.c
> > +++ b/networking/udhcp/dhcpd.c
> > @@ -415,6 +415,9 @@ int udhcpd_main(int argc UNUSED_PARAM, char **argv)
> > if (server_config.auto_time) {
> > tv.tv_sec = timeout_end - monotonic_sec();
> > tv.tv_usec = 0;
> > +
> > + if ((unsigned)tv.tv_sec > server_config.auto_time)
> > + tv.tv_sec = 0;
>
> I don't think this is a valid fix. If overflow occurs, there has
> already been an invocation of undefined behavior (assuming it's
> actually an overflow and not an implicit conversion into a signed type
> from legal unsigned arithmetic, but in that case the result would
> still be nonsense and might not look negative!). The check belongs
> before the arithmetic that would invoke UB (or just give a
> non-meaningful result) rather than after the computation.
>
Apologies for the double-response, but I revised the
patch to use a stack variable for safety, and to save 5 bytes.
To be honest, I think this particular bit of code could benefit from
some refactoring...
Better?
---
networking/udhcp/dhcpd.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/networking/udhcp/dhcpd.c b/networking/udhcp/dhcpd.c
index 4b3ed24..75ddc12 100644
--- a/networking/udhcp/dhcpd.c
+++ b/networking/udhcp/dhcpd.c
@@ -399,6 +399,7 @@ int udhcpd_main(int argc UNUSED_PARAM, char **argv)
fd_set rfds;
struct dhcp_packet packet;
int bytes;
+ unsigned int msec;
struct timeval tv;
uint8_t *server_id_opt;
uint8_t *requested_ip_opt;
@@ -413,8 +414,12 @@ int udhcpd_main(int argc UNUSED_PARAM, char **argv)
max_sock = udhcp_sp_fd_set(&rfds, server_socket);
if (server_config.auto_time) {
- tv.tv_sec = timeout_end - monotonic_sec();
tv.tv_usec = 0;
+ tv.tv_sec = 0;
+
+ msec = monotonic_sec();
+ if (msec < timeout_end)
+ tv.tv_sec = timeout_end - msec;
}
retval = 0;
if (!server_config.auto_time || tv.tv_sec > 0) {
--
2.0.4
pgpItXvfuU5MT.pgp
Description: PGP signature
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
Re: [RESEND] Re: [PATCH] udhcpd: Handle auto_time timeout overflow
On Tue, Jan 27, 2015 at 02:13:38PM +0100, Denys Vlasenko wrote:
> With gcc 4.7.2:
>
> calludhcp_sp_fd_set #
> cmpl$0, bb_common_bufsiz1+52(%rip) #, MEM[(struct
> server_config_t * {ref-all})&bb_common_bufsiz1].auto_time
> movl%eax, %ebx #, max_sock
> je .L71#,
> callmonotonic_sec #
> movl24(%rsp), %edx # %sfp,
> movq$0, 56(%rsp)#, tv.tv_usec
> subl%eax, %edx # D.8343,
> movl%edx, %eax #,
> movq%rax, 48(%rsp) #, tv.tv_sec
> .L71:
> xorl%r8d, %r8d # iftmp.11
> cmpl$0, bb_common_bufsiz1+52(%rip) #, MEM[(struct
> server_config_t * {ref-all})&bb_common_bufsiz1].auto_time
> je .L72#,
> cmpq$0, 48(%rsp)#, tv.tv_sec
> jg .L110 #,
> jmp .L166 #
> .L110:
> leaq48(%rsp), %r8 #, iftmp.11
> .L72:
> leal1(%rbx), %edi #, tmp216
> leaq104(%rsp), %rsi #,
> xorl%ecx, %ecx #
> xorl%edx, %edx #
> callselect #
Interesting. I've sent an updated patch based on Rich's suggestion, and
I examined the generated assembly after applying it as well. GCC
emits the expected instruction in that case as well:
.L88:
movl52+bb_common_bufsiz1(%rip), %r14d # MEM[(struct
server_config_t * {ref-all})&bb_common_bufsiz1].auto_time,
testl %r14d, %r14d#
je .L89#,
cmpq$0, 80(%rsp)#, tv.tv_sec
jle .L180 #,
leaq80(%rsp), %r8 #, iftmp.12
jmp .L87#
I'm at a loss as to why this behavior occurs. I've got gcc-4.7.4 and
gcc 4.9.7 and 4.9.2 building. I'll try to reproduce it with those with
the problem code, and see what I get, just for the hell of it.
Tim
pgpR8GA64fWeO.pgp
Description: PGP signature
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH] udhcpd: Handle auto_time timeout overflow
Hi Rich,
On Tue, Jan 27, 2015 at 08:41:30AM -0500, Rich Felker wrote:
> On Wed, Jan 21, 2015 at 11:00:03PM +0100, Tim Hentenaar wrote:
> > ---
> > networking/udhcp/dhcpd.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/networking/udhcp/dhcpd.c b/networking/udhcp/dhcpd.c
> > index 4b3ed24..d56763f 100644
> > --- a/networking/udhcp/dhcpd.c
> > +++ b/networking/udhcp/dhcpd.c
> > @@ -415,6 +415,9 @@ int udhcpd_main(int argc UNUSED_PARAM, char **argv)
> > if (server_config.auto_time) {
> > tv.tv_sec = timeout_end - monotonic_sec();
> > tv.tv_usec = 0;
> > +
> > + if ((unsigned)tv.tv_sec > server_config.auto_time)
> > + tv.tv_sec = 0;
>
> I don't think this is a valid fix. If overflow occurs, there has
> already been an invocation of undefined behavior (assuming it's
> actually an overflow and not an implicit conversion into a signed type
> from legal unsigned arithmetic, but in that case the result would
> still be nonsense and might not look negative!). The check belongs
> before the arithmetic that would invoke UB (or just give a
> non-meaningful result) rather than after the computation.
Technically it's unsigned arithmetic underflowing (since an unsigned
integer cannot be negative) then being converted to a signed
integer (since tv_sec is signed) which is coincidentally negative. You're
absolutely right that it could be possible in this case to get a
non-sensical result that appears positive.
What happens is that timeout_end isn't reset by all paths through the
loop, and it can happen that timeout_end is smaller than the value
returned by monotonic_sec(), thus the unexpected negative result.
This looks better to me. What do you think?
diff --git a/networking/udhcp/dhcpd.c b/networking/udhcp/dhcpd.c
index 4b3ed24..f801ef2 100644
--- a/networking/udhcp/dhcpd.c
+++ b/networking/udhcp/dhcpd.c
@@ -413,8 +413,10 @@ int udhcpd_main(int argc UNUSED_PARAM, char **argv)
max_sock = udhcp_sp_fd_set(&rfds, server_socket);
if (server_config.auto_time) {
- tv.tv_sec = timeout_end - monotonic_sec();
tv.tv_usec = 0;
+ tv.tv_sec = 0;
+ if (monotonic_sec() < timeout_end)
+ tv.tv_sec = timeout_end - monotonic_sec();
}
retval = 0;
if (!server_config.auto_time || tv.tv_sec > 0) {
Tim
pgpMglWTw6G2N.pgp
Description: PGP signature
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH] udhcpd: Handle auto_time timeout overflow
On Wed, Jan 21, 2015 at 11:00:03PM +0100, Tim Hentenaar wrote:
> Howdy y'all,
>
> I've noticed an interesting issue with udhcpd and auto_time.
>
> Some paths within the while loop don't go through continue_with_autotime.
> Thus, if it takes a bit too long to reset timeout_end, the monotonic
> timer may be far enough along that the subtraction which sets tv.tv_sec
> will overflow, like so:
>
> Jan 21 19:38:13 10.0.0.1 udhcpd[75]: Waking from select()
> Jan 21 19:38:13 10.0.0.1 udhcpd[75]: tv_sec = 10
> Jan 21 19:38:21 10.0.0.1 udhcpd[75]: Waking from select()
> Jan 21 19:38:23 10.0.0.1 udhcpd[75]: Sending OFFER of 10.0.0.2
> Jan 21 19:38:23 10.0.0.1 udhcpd[75]: tv_sec = -1
> Jan 21 19:38:23 10.0.0.1 udhcpd[75]: Waking from select()
> Jan 21 19:38:23 10.0.0.1 udhcpd[75]: Sending ACK to 10.0.0.2
> Jan 21 19:38:23 10.0.0.1 udhcpd[75]: tv_sec = -1
> Jan 21 19:38:43 10.0.0.2 udhcpc[47]: Sending renew...
> Jan 21 19:38:43 10.0.0.2 udhcpc[47]: Lease of 10.0.0.2 obtained, lease time 30
> Jan 21 19:38:43 10.0.0.1 udhcpd[75]: Waking from select()
> Jan 21 19:38:43 10.0.0.1 udhcpd[75]: Sending ACK to 10.0.0.2
> Jan 21 19:38:43 10.0.0.1 udhcpd[75]: tv_sec = -21
> Jan 21 19:39:03 10.0.0.2 udhcpc[47]: Sending renew...
> Jan 21 19:39:03 10.0.0.1 udhcpd[75]: Waking from select()
> Jan 21 19:39:03 10.0.0.1 udhcpd[75]: Sending ACK to 10.0.0.2
> Jan 21 19:39:03 10.0.0.2 udhcpc[47]: Lease of 10.0.0.2 obtained, lease time 30
> Jan 21 19:39:03 10.0.0.1 udhcpd[75]: tv_sec = -41
>
> This patch adds a quick and easy check for it, resetting tv_sec to 0,
> which should fall through to write_leases() and continue_with_autotime,
> resetting timeout_end again.
>
> Tim
>
> ---
> networking/udhcp/dhcpd.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/networking/udhcp/dhcpd.c b/networking/udhcp/dhcpd.c
> index 4b3ed24..d56763f 100644
> --- a/networking/udhcp/dhcpd.c
> +++ b/networking/udhcp/dhcpd.c
> @@ -415,6 +415,9 @@ int udhcpd_main(int argc UNUSED_PARAM, char **argv)
> if (server_config.auto_time) {
> tv.tv_sec = timeout_end - monotonic_sec();
> tv.tv_usec = 0;
> +
> + if ((unsigned)tv.tv_sec > server_config.auto_time)
> + tv.tv_sec = 0;
I don't think this is a valid fix. If overflow occurs, there has
already been an invocation of undefined behavior (assuming it's
actually an overflow and not an implicit conversion into a signed type
from legal unsigned arithmetic, but in that case the result would
still be nonsense and might not look negative!). The check belongs
before the arithmetic that would invoke UB (or just give a
non-meaningful result) rather than after the computation.
Rich
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
Re: [RESEND] Re: [PATCH] udhcpd: Handle auto_time timeout overflow
On Tue, Jan 27, 2015 at 1:28 PM, Tim Hentenaar wrote:
> callmonotonic_sec #
> movl24(%rsp), %ecx # %sfp, D.9844
> movl52+bb_common_bufsiz1(%rip), %r14d # MEM[(struct
> server_config_t * {ref-all})&bb_common_bufsiz1].auto_time,
> movq$0, 88(%rsp)#, tv.tv_usec
> subl%eax, %ecx # D.9836, D.9844
> testl %r14d, %r14d#
> movq%rcx, 80(%rsp) # D.9844, tv.tv_sec
> je .L88#,
> testq %rcx, %rcx # D.9844
> je .L179 #,
> leaq80(%rsp), %r8 #, iftmp.12
> jmp .L87#
>
> Are you using a 32-bit x86 arch? If not, what do you get if you compile
> it in 64-bit mode?
With gcc 4.7.2:
calludhcp_sp_fd_set #
cmpl$0, bb_common_bufsiz1+52(%rip) #, MEM[(struct
server_config_t * {ref-all})&bb_common_bufsiz1].auto_time
movl%eax, %ebx #, max_sock
je .L71#,
callmonotonic_sec #
movl24(%rsp), %edx # %sfp,
movq$0, 56(%rsp)#, tv.tv_usec
subl%eax, %edx # D.8343,
movl%edx, %eax #,
movq%rax, 48(%rsp) #, tv.tv_sec
.L71:
xorl%r8d, %r8d # iftmp.11
cmpl$0, bb_common_bufsiz1+52(%rip) #, MEM[(struct
server_config_t * {ref-all})&bb_common_bufsiz1].auto_time
je .L72#,
cmpq$0, 48(%rsp)#, tv.tv_sec
jg .L110 #,
jmp .L166 #
.L110:
leaq48(%rsp), %r8 #, iftmp.11
.L72:
leal1(%rbx), %edi #, tmp216
leaq104(%rsp), %rsi #,
xorl%ecx, %ecx #
xorl%edx, %edx #
callselect #
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
Re: [RESEND] Re: [PATCH] udhcpd: Handle auto_time timeout overflow
On Tue, Jan 27, 2015 at 11:15:06AM +0100, Denys Vlasenko wrote:
> On Mon, Jan 26, 2015 at 6:13 PM, Tim Hentenaar wrote:
> > Hi Denys,
> >
> > On Sun, Jan 25, 2015 at 09:58:50PM +0100, Denys Vlasenko wrote:
> >> I don't understand why current code does not reach write_leases().
> >> If tv.tv_sec is negative, we should not reach select() here -
> >>
> >> retval = 0;
> >> if (!server_config.auto_time || tv.tv_sec > 0) {
> >> retval = select(max_sock + 1, &rfds, NULL, NULL,
> >> server_config.auto_time ? &tv :
> >> NULL);
> >> }
> >> if (retval == 0) {
> >> write_leases();
> >> goto continue_with_autotime;
> >> }
> >>
> >> and therefore, retval == 0 and we should reach write_leases().
> >>
> >> What am I missing?
> >
> > I wondered the same thing at first. Here's the relevent code as
> > generated by GCC. Interestingly, the "tv_sec > 0" check seems to get
> > optimized out (I'm compiling with -O2), and select() gets called as long
> > as tv_sec is non-zero. Looks almost as if it's treating tv_sec as
> > unsigned...
>
> Can't be, "v > 0" is a valid test even for unsigned types.
>
> Can you run "make networking/udhcp/dhcpd.s" and post the resulting file?
> I'm seeing this code with gcc 4.3.1:
Of course. In your case, with 32-bit code it seems fine.
But, I still see the same incorrect comparison:
.L88:
xorl%r8d, %r8d # iftmp.12
.L87:
movq(%rsp), %rsi# %sfp,
leal1(%rbx), %edi #, D.9843
xorl%ecx, %ecx #
xorl%edx, %edx #
callselect@PLT #
testl %eax, %eax # retval
jne .L182 #,
.L179:
callwrite_leases#
.p2align 4,,6
jmp .L83#
.L86:
.p2align 4,,8
callmonotonic_sec #
movl24(%rsp), %ecx # %sfp, D.9844
movl52+bb_common_bufsiz1(%rip), %r14d # MEM[(struct
server_config_t * {ref-all})&bb_common_bufsiz1].auto_time,
movq$0, 88(%rsp)#, tv.tv_usec
subl%eax, %ecx # D.9836, D.9844
testl %r14d, %r14d#
movq%rcx, 80(%rsp) # D.9844, tv.tv_sec
je .L88#,
testq %rcx, %rcx # D.9844
je .L179 #,
leaq80(%rsp), %r8 #, iftmp.12
jmp .L87#
Are you using a 32-bit x86 arch? If not, what do you get if you compile
it in 64-bit mode?
I wonder why at -O2 it generates je here instead of jle. The ops would
be the same size and speed. At -Os, and -O0 it generates the correct
instruction.
At -Os:
.L78:
xorl%r8d, %r8d # iftmp.12
cmpl$0, 52+bb_common_bufsiz1(%rip) #, MEM[(struct server_config_t
* {ref-all})&bb_common_bufsiz1].auto_time
je .L79#,
cmpq$0, 80(%rsp)#, tv.tv_sec
jle .L170 #,
At -O0:
.L59:
movl$0, 32(%rsp)#, retval
leaqbb_common_bufsiz1(%rip), %rax #, bb_common_bufsiz1.62
movl52(%rax), %eax # MEM[(struct server_config_t *
{ref-all})bb_common_bufsiz1.62_111].auto_time, D.8648
testl %eax, %eax # D.8648
je .L60#,
movq144(%rsp), %rax # tv.tv_sec, D.8656
testq %rax, %rax # D.8656
jle .L61#,
I even wrote a small test case to see if I could reproduce this behavior
with a smaller body of code, to no avail.
Tim
pgprs0ZtBHMOV.pgp
Description: PGP signature
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
Re: [RESEND] Re: [PATCH] udhcpd: Handle auto_time timeout overflow
On Mon, Jan 26, 2015 at 6:13 PM, Tim Hentenaar wrote:
> Hi Denys,
>
> On Sun, Jan 25, 2015 at 09:58:50PM +0100, Denys Vlasenko wrote:
>> I don't understand why current code does not reach write_leases().
>> If tv.tv_sec is negative, we should not reach select() here -
>>
>> retval = 0;
>> if (!server_config.auto_time || tv.tv_sec > 0) {
>> retval = select(max_sock + 1, &rfds, NULL, NULL,
>> server_config.auto_time ? &tv :
>> NULL);
>> }
>> if (retval == 0) {
>> write_leases();
>> goto continue_with_autotime;
>> }
>>
>> and therefore, retval == 0 and we should reach write_leases().
>>
>> What am I missing?
>
> I wondered the same thing at first. Here's the relevent code as
> generated by GCC. Interestingly, the "tv_sec > 0" check seems to get
> optimized out (I'm compiling with -O2), and select() gets called as long
> as tv_sec is non-zero. Looks almost as if it's treating tv_sec as
> unsigned...
Can't be, "v > 0" is a valid test even for unsigned types.
>
> In my .config, I've got:
>
> CONFIG_EXTRA_CFLAGS="-O2 -fPIE -pie"
>
> Code generated with -O2:
> ; bb_common_bufsiz1+0x34 = server_config.auto_time -> %r15d
>177d7: 44 8b 3d 96 ec 26 00mov0x26ec96(%rip),%r15d
>177de: 89 c3 mov%eax,%ebx
>177e0: 45 85 fftest %r15d,%r15d
>177e3: 75 26 jne1780b
>
>; if (!server_config.auto_time) { select(...); }
>177e5: 45 31 c0xor%r8d,%r8d
>177e8: 48 8b b5 60 fc ff ffmov-0x3a0(%rbp),%rsi
>177ef: 8d 7b 01lea0x1(%rbx),%edi
>177f2: 31 c9 xor%ecx,%ecx
>177f4: 31 d2 xor%edx,%edx
>177f6: e8 b5 08 ff ff callq 80b0
Can you run "make networking/udhcp/dhcpd.s" and post the resulting file?
I'm seeing this code with gcc 4.3.1:
calludhcp_sp_fd_set #
movl%eax, %ebx #, max_sock
cmpl$0, bb_common_bufsiz1+44#, .auto_time
je .L61#,
callmonotonic_sec #
movl12(%esp), %edx # timeout_end,
subl%eax, %edx # D.7265,
movl%edx, 816(%esp) # tmp124, tv.tv_sec
movl$0, 820(%esp) #, tv.tv_usec
.L61:
xorl%eax, %eax # iftmp.50
cmpl$0, bb_common_bufsiz1+44#, .auto_time
je .L63#,
cmpl$0, 816(%esp) #, tv.tv_sec
jle .L64#,
leal816(%esp), %eax #, iftmp.50
.L63:
pushl %eax# iftmp.50
pushl $0 #
pushl $0 #
leal664(%esp), %eax #, tmp125
pushl %eax# tmp125
leal1(%ebx), %eax #, tmp126
pushl %eax# tmp126
callselect #
addl$20, %esp #,
cmpl$0, %eax#, retval.323
jne .L65#,
.L64:
callwrite_leases#
jmp .L102 #
.L65:
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
[RESEND] Re: [PATCH] udhcpd: Handle auto_time timeout overflow
Hi Denys,
On Sun, Jan 25, 2015 at 09:58:50PM +0100, Denys Vlasenko wrote:
> I don't understand why current code does not reach write_leases().
> If tv.tv_sec is negative, we should not reach select() here -
>
> retval = 0;
> if (!server_config.auto_time || tv.tv_sec > 0) {
> retval = select(max_sock + 1, &rfds, NULL, NULL,
> server_config.auto_time ? &tv : NULL);
> }
> if (retval == 0) {
> write_leases();
> goto continue_with_autotime;
> }
>
> and therefore, retval == 0 and we should reach write_leases().
>
> What am I missing?
I wondered the same thing at first. Here's the relevent code as
generated by GCC. Interestingly, the "tv_sec > 0" check seems to get
optimized out (I'm compiling with -O2), and select() gets called as long
as tv_sec is non-zero. Looks almost as if it's treating tv_sec as
unsigned...
In my .config, I've got:
CONFIG_EXTRA_CFLAGS="-O2 -fPIE -pie"
Code generated with -O2:
; bb_common_bufsiz1+0x34 = server_config.auto_time -> %r15d
177d7: 44 8b 3d 96 ec 26 00mov0x26ec96(%rip),%r15d
177de: 89 c3 mov%eax,%ebx
177e0: 45 85 fftest %r15d,%r15d
177e3: 75 26 jne1780b
; if (!server_config.auto_time) { select(...); }
177e5: 45 31 c0xor%r8d,%r8d
177e8: 48 8b b5 60 fc ff ffmov-0x3a0(%rbp),%rsi
177ef: 8d 7b 01lea0x1(%rbx),%edi
177f2: 31 c9 xor%ecx,%ecx
177f4: 31 d2 xor%edx,%edx
177f6: e8 b5 08 ff ff callq 80b0
; if (retval == 0)
; write leases
177fb: 85 c0 test %eax,%eax
177fd: 75 4a jne17849
177ff: e8 e5 88 03 00 callq 500e9
17804: eb 85 jmp1778b
17806: e8 85 09 ff ff callq 8190 <__stack_chk_fail@plt>
; if (server_config.auto_time) {
1780b: 0f 1f 44 00 00 nopl 0x0(%rax,%rax,1)
17810: e8 3b 2c ff ff callq a450
; timeout_end -> %ecx
17815: 8b 8d 68 fc ff ff mov-0x398(%rbp),%ecx
; server_config.auto_time -> %r14d
1781b: 44 8b 35 52 ec 26 00mov0x26ec52(%rip),%r14d
; tv.tv_usec = 0
17822: 48 c7 85 98 fc ff ffmovq $0x0,-0x368(%rbp)
17829: 00 00 00 00
; tv.tv_sec = timeout_end - monotonic_time()
1782d: 29 c1 sub%eax,%ecx
1782f: 45 85 f6test %r14d,%r14d
17832: 48 89 8d 90 fc ff ffmov%rcx,-0x370(%rbp)
; if (!server_config.auto_time)
; go do select() with NULL timeout
17839: 74 aa je 177e5
; if (!tv.tv_sec)
; go write leases
1783b: 48 85 c9test %rcx,%rcx
1783e: 74 bf je 177ff
; Setup timeout param for select() and go do it
17840: 4c 8d 85 90 fc ff fflea-0x370(%rbp),%r8
17847: eb 9f jmp177e8
Recompiling with -O0 yields:
; If (!server_config.auto_time)
3055f: 48 8d 05 da 4e 28 00lea0x284eda(%rip),%rax
30566: 8b 40 34mov0x34(%rax),%eax
30569: 85 c0 test %eax,%eax
3056b: 74 0c je 30579
; if !server_config.auto_time && tv.tv_sec <= 0 then write leases
3056d: 48 8b 85 b0 fc ff ffmov-0x350(%rbp),%rax
30574: 48 85 c0test %rax,%rax
30577: 7e 44 jle305bd
; if (!server_config.auto_time)
;go set timeout to NULL
30579: 48 8d 05 c0 4e 28 00lea0x284ec0(%rip),%rax
30580: 8b 40 34mov0x34(%rax),%eax
30583: 85 c0 test %eax,%eax
30585: 74 09 je 30590
; else: use tv as timeout for select()
30587: 48 8d 85 b0 fc ff fflea-0x350(%rbp),%rax
3058e: eb 05 jmp30595
; timeout = NULL
30590: b8 00 00 00 00 mov$0x0,%eax
; Setup select parameters
30595: 8b 95 58 fc ff ff mov-0x3a8(%rbp),%edx
3059b: 8d 7a 01lea0x1(%rdx),%edi
3059e: 48 8d b5 f0 fc ff fflea-0x310(%rbp),%rsi
305a5: 49 89 c0mov%rax,%r8
305a8: b9 00 00 00 00 mov$0x0,%ecx
305ad: ba 00 00 00 00 mov$0x0,%edx
305b2: e8 99 7b fd ff callq 8150
305b7: 89 85 40 fc ff ff mov%eax,-0x3c0(%rbp)
; if (reval == 0)
;write leases
305bd: 83 bd 40 fc ff ff 0
Re: [PATCH] udhcpd: Handle auto_time timeout overflow
On Wed, Jan 21, 2015 at 11:00 PM, Tim Hentenaar wrote:
> Howdy y'all,
>
> I've noticed an interesting issue with udhcpd and auto_time.
>
> Some paths within the while loop don't go through continue_with_autotime.
> Thus, if it takes a bit too long to reset timeout_end, the monotonic
> timer may be far enough along that the subtraction which sets tv.tv_sec
> will overflow, like so:
>
> Jan 21 19:38:13 10.0.0.1 udhcpd[75]: Waking from select()
> Jan 21 19:38:13 10.0.0.1 udhcpd[75]: tv_sec = 10
> Jan 21 19:38:21 10.0.0.1 udhcpd[75]: Waking from select()
> Jan 21 19:38:23 10.0.0.1 udhcpd[75]: Sending OFFER of 10.0.0.2
> Jan 21 19:38:23 10.0.0.1 udhcpd[75]: tv_sec = -1
> Jan 21 19:38:23 10.0.0.1 udhcpd[75]: Waking from select()
> Jan 21 19:38:23 10.0.0.1 udhcpd[75]: Sending ACK to 10.0.0.2
> Jan 21 19:38:23 10.0.0.1 udhcpd[75]: tv_sec = -1
> Jan 21 19:38:43 10.0.0.2 udhcpc[47]: Sending renew...
> Jan 21 19:38:43 10.0.0.2 udhcpc[47]: Lease of 10.0.0.2 obtained, lease time 30
> Jan 21 19:38:43 10.0.0.1 udhcpd[75]: Waking from select()
> Jan 21 19:38:43 10.0.0.1 udhcpd[75]: Sending ACK to 10.0.0.2
> Jan 21 19:38:43 10.0.0.1 udhcpd[75]: tv_sec = -21
> Jan 21 19:39:03 10.0.0.2 udhcpc[47]: Sending renew...
> Jan 21 19:39:03 10.0.0.1 udhcpd[75]: Waking from select()
> Jan 21 19:39:03 10.0.0.1 udhcpd[75]: Sending ACK to 10.0.0.2
> Jan 21 19:39:03 10.0.0.2 udhcpc[47]: Lease of 10.0.0.2 obtained, lease time 30
> Jan 21 19:39:03 10.0.0.1 udhcpd[75]: tv_sec = -41
>
> This patch adds a quick and easy check for it, resetting tv_sec to 0,
> which should fall through to write_leases() and continue_with_autotime,
> resetting timeout_end again.
I don't understand why current code does not reach write_leases().
If tv.tv_sec is negative, we should not reach select() here -
retval = 0;
if (!server_config.auto_time || tv.tv_sec > 0) {
retval = select(max_sock + 1, &rfds, NULL, NULL,
server_config.auto_time ? &tv : NULL);
}
if (retval == 0) {
write_leases();
goto continue_with_autotime;
}
and therefore, retval == 0 and we should reach write_leases().
What am I missing?
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
[PATCH] udhcpd: Handle auto_time timeout overflow
Howdy y'all,
I've noticed an interesting issue with udhcpd and auto_time.
Some paths within the while loop don't go through continue_with_autotime.
Thus, if it takes a bit too long to reset timeout_end, the monotonic
timer may be far enough along that the subtraction which sets tv.tv_sec
will overflow, like so:
Jan 21 19:38:13 10.0.0.1 udhcpd[75]: Waking from select()
Jan 21 19:38:13 10.0.0.1 udhcpd[75]: tv_sec = 10
Jan 21 19:38:21 10.0.0.1 udhcpd[75]: Waking from select()
Jan 21 19:38:23 10.0.0.1 udhcpd[75]: Sending OFFER of 10.0.0.2
Jan 21 19:38:23 10.0.0.1 udhcpd[75]: tv_sec = -1
Jan 21 19:38:23 10.0.0.1 udhcpd[75]: Waking from select()
Jan 21 19:38:23 10.0.0.1 udhcpd[75]: Sending ACK to 10.0.0.2
Jan 21 19:38:23 10.0.0.1 udhcpd[75]: tv_sec = -1
Jan 21 19:38:43 10.0.0.2 udhcpc[47]: Sending renew...
Jan 21 19:38:43 10.0.0.2 udhcpc[47]: Lease of 10.0.0.2 obtained, lease time 30
Jan 21 19:38:43 10.0.0.1 udhcpd[75]: Waking from select()
Jan 21 19:38:43 10.0.0.1 udhcpd[75]: Sending ACK to 10.0.0.2
Jan 21 19:38:43 10.0.0.1 udhcpd[75]: tv_sec = -21
Jan 21 19:39:03 10.0.0.2 udhcpc[47]: Sending renew...
Jan 21 19:39:03 10.0.0.1 udhcpd[75]: Waking from select()
Jan 21 19:39:03 10.0.0.1 udhcpd[75]: Sending ACK to 10.0.0.2
Jan 21 19:39:03 10.0.0.2 udhcpc[47]: Lease of 10.0.0.2 obtained, lease time 30
Jan 21 19:39:03 10.0.0.1 udhcpd[75]: tv_sec = -41
This patch adds a quick and easy check for it, resetting tv_sec to 0,
which should fall through to write_leases() and continue_with_autotime,
resetting timeout_end again.
Tim
---
networking/udhcp/dhcpd.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/networking/udhcp/dhcpd.c b/networking/udhcp/dhcpd.c
index 4b3ed24..d56763f 100644
--- a/networking/udhcp/dhcpd.c
+++ b/networking/udhcp/dhcpd.c
@@ -415,6 +415,9 @@ int udhcpd_main(int argc UNUSED_PARAM, char **argv)
if (server_config.auto_time) {
tv.tv_sec = timeout_end - monotonic_sec();
tv.tv_usec = 0;
+
+ if ((unsigned)tv.tv_sec > server_config.auto_time)
+ tv.tv_sec = 0;
}
retval = 0;
if (!server_config.auto_time || tv.tv_sec > 0) {
--
2.0.4
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
