[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