[PATCH] udhcpd: Handle auto_time timeout overflow

[Tim Hentenaar]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.busybox.net/pipermail/busybox/attachments/20150127/80db234c/attachment.asc>