MIME-Version: 1.0
In-Reply-To: <rt-3.8.6-62597-1290641910-1422.22033-5-0@isc.org>
References: <RT-Ticket-22033@isc.org> <4C812D59.50503@redhat.com> <rt-3.8.6-62597-1290641910-1422.22033-5-0@isc.org>
Message-ID: <4D05EFAF.9090905@redhat.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Subject: Re: [ISC-Bugs #22033] dhcp-4.2.0 uses int for handling time values
User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.13) Gecko/20101209 Fedora/3.1.7-0.35.b3pre.fc14 Thunderbird/3.1.7
Date: Mon, 13 Dec 2010 11:04:31 +0100
To: dhcp-bugs@isc.org
Content-Transfer-Encoding: 7bit
From: Jiri Popelka <jpopelka@redhat.com>
RT-Message-ID: <rt-3.8.6-55504-1292234683-283.22033-0-0@isc.org>
Content-Length: 4335

Hi Shawn,

see my small remark lower in the code.

Jiri

On 11/25/2010 12:38 AM, Shawn Routhier via RT wrote:
> Thank you for the bug report. We've chose a different solution in order to
> limit what gets passed into the isc_interval_set call. I'm attaching a copy of
> the diff to the ticket. I would appreciate it if you can verify the fix
> (assuming the testbed is still available).
>
> Cut here for diff
>
> --- DHCP/RELNOTES:1.405 Thu Nov 18 19:47:36 2010
> +++ DHCP/RELNOTES Wed Nov 24 22:25:02 2010
> @@ -144,6 +144,9 @@
> a setsockopt() call. The signal is already being ignored as part
> of the ISC library. [ISC-Bugs #22269]
>
> +- Limit the timeout period allowed in the dispatch code to 2^^32-1 seconds.
> + Thanks to a report from Jiri Popelka at Red Hat.
> + [ISC-Bugs #22033], [Red Hat Bug #628258]
>
> Changes since 4.2.0b2
>
> Index: DHCP/common/dispatch.c
> diff -u DHCP/common/dispatch.c:1.75 DHCP/common/dispatch.c:1.75.46.1
> --- DHCP/common/dispatch.c:1.75 Tue May 25 00:22:29 2010
> +++ DHCP/common/dispatch.c Wed Nov 24 22:25:02 2010
> @@ -174,6 +174,7 @@
>
> /* maximum value for usec */
> #define USEC_MAX 1000000
> +#define DHCP_SEC_MAX 0xFFFFFFFF
>
> void add_timeout (when, where, what, ref, unref)
> struct timeval *when;
> @@ -185,7 +186,8 @@
> struct timeout *t, *q;
> int usereset = 0;
> isc_result_t status;
> - int sec, usec;
> + int64_t sec;
> + int usec;
> isc_interval_t interval;
> isc_time_t expires;
>
> @@ -227,9 +229,49 @@
> q->what = what;
> }
>
> - /* We don't really need this, but keep it for now */
> - q->when.tv_sec = when->tv_sec;
> - q->when.tv_usec = when->tv_usec;
> + /*
> + * The value passed in is a time from an epoch but we need a relative
> + * time so we need to do some math to try and recover the period.
> + * This is complicated by the fact that not all of the calls cared
> + * about the usec value, if it's zero we assume the caller didn't care.
> + *
> + * The ISC timer library doesn't seem to like negative values
> + * and can't accept any values above 4G-1 seconds so we limit
> + * the values to 0<= value<  4G-1. We do it before
> + * checking the trace option so that both the trace code and
> + * the working code use the same values.
> + */
> +
> + sec = when->tv_sec - cur_tv.tv_sec;
> + usec = when->tv_usec - cur_tv.tv_usec;
> +
> + if ((when->tv_usec != 0)&&  (usec<  0)) {
> + sec--;
> + usec += USEC_MAX;
> + }
> +
> + if (sec<  0) {
> + sec = 0;
> + usec = 0;
> + } else if (sec>  DHCP_SEC_MAX) {
> + log_error("Timeout requested too large %lld "
> + "reducing to 2^^32-1", sec);
> + sec = DHCP_SEC_MAX;
> + usec = 0;
> + }
> + else if (usec<  0) {
> + usec = 0;
> + } else if (usec>= USEC_MAX) {
> + usec = USEC_MAX - 1;
> + }
> +
> + /*
> + * This is necessary for the tracing code but we put it
> + * here in case we want to compare timing information
> + * for some reason, like debugging.
> + */
> + q->when.tv_sec = cur_tv.tv_sec + (sec&  DHCP_SEC_MAX);
> + q->when.tv_usec = usec;
>
> #if defined (TRACING)
> if (trace_playback()) {
> @@ -279,38 +321,7 @@
> q->next = timeouts;
> timeouts = q;
>
> - /*
> - * Set up the interval values - The previous timers allowed
> - * negative values to be set, the ISC timer library doesn't like
> - * that so we make any negative values 0 which sould amount to
> - * the same thing.
> - */
> -
> - /*
> - * The value passed in is a time from an epoch but we need a relative
> - * time so we need to do some math to try and recover the period.
> - * This is complicated by the fact that not all of the calls cared
> - * about the usec value, if it's zero we assume the caller didn't care.
> - */
> -
> - sec = when->tv_sec - cur_tv.tv_sec;
> - usec = when->tv_usec - cur_tv.tv_usec;
> -
> - if ((when->tv_usec != 0)&&  (usec<  0)) {
> - sec--;
> - usec += USEC_MAX;
> - }
> -
> - if (sec<  0) {
> - sec = 0;
> - usec = 0;
> - } else if (usec<  0) {
> - usec = 0;
> - } else if (usec>= USEC_MAX) {
> - usec = USEC_MAX - 1;
> - }
> -
> - isc_interval_set(&interval, sec, usec * 1000);
> + isc_interval_set(&interval, sec&  0xFFFFFFFF, usec * 1000);
Probably should be

+ isc_interval_set(&interval, sec&  DHCP_SEC_MAX, usec * 1000);
instead of
+ isc_interval_set(&interval, sec&  0xFFFFFFFF, usec * 1000);


> status = isc_time_nowplusinterval(&expires,&interval);
> if (status != ISC_R_SUCCESS) {
> /*
>