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
