[PATCH] udhcpd: Handle auto_time timeout overflow

Rich Felker dalias at libc.org
Tue Jan 27 18:48:42 UTC 2015


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


More information about the busybox mailing list