Date: Fri, 19 Dec 1997 19:26:35 +1100 From: Bruce Evans <bde@zeta.org.au> To: jmb@FreeBSD.ORG, lars@fredriks-1.pr.mcs.net Cc: current@FreeBSD.ORG Subject: Re: EDOM and hz Message-ID: <199712190826.TAA03871@godzilla.zeta.org.au>
next in thread | raw e-mail | index | archive | help
>> if (tv->tv_sec > SHRT_MAX / hz - hz) {
>> error = EDOM;
>> goto bad;
>> }
>>...
> <hmmm....better grab stevens v2....ah, let's see on p 543,4>
>
>Rich Stevens recommends:
>
> if (tv->tv_sec*hz + tv_usec/tick > SHRT_MAX) {
> error = EDOM;
> goto bad;
> }
>
>
> where tick = 1,000,000 / hz
> and SHRT_MAX = 32767
As pointed out in Stevens, the original code should be changed because
it is wrong. I had comments in my mailbox about this bug, but seem to
have lost the mail, and I thought that -current has a fix from Lite2,
but it doesn't.
The suggested change is wronger. It fails when tv_sec*hz overflows,
and when the addition overflows.
Checking for overflow of the multiplication is not easy:
Method 1:
Convert everything to u_long and check that the values don't change, so
that the multiplication can't trap. Then after doing the multiplication
(m = a * b), check that ((m / a) == b && (m % a) == 0), except when a
== 0. This would be slower than doing the division in the original code,
especially if hz is constant.
Method 2:
Avoid possibile overflow by checking that (tv->tv_sec < LONG_MAX / hz).
When combined with other checks, this gives a check much like the one
in the original code.
A more correct version of the original code:
if (tv->tv_sec > SHRT_MAX / hz - 1) {
error = EDOM;
goto bad;
}
The -1 term is unnecessary in some cases, e.g., when tv->tv_usec == 0,
but it's probably not worth doing extra checks to avoid these cases.
The -1 term needs to be -2 in some cases where hz doesn't divide
1000000 evenly and tick = 1000000 / hz is rounded in a certain way.
The rounding becomes more significant if hz is a large fraction of
1000000 and can't work right if hz > 1000000.
Perhaps the best way is to combine the checks:
u_long val;
...
/* assert(hz > 0); */
if (tv->tv_sec < 0 || tv->tv_usec < 0 || tv->tv_usec >= 1000000 ||
tv->tv_sec > SHRT_MAX / hz) {
error = EDOM;
goto bad;
}
/* assert(tick > 0); */
/* assert(ULONG_MAX - SHRT_MAX >= 1000000); */
val = (u_long)(tv->tv_sec * hz) + tv_usec / tick;
if (val > SHRT_MAX) {
error = EDOM;
goto bad;
}
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?199712190826.TAA03871>