Date: Sun, 19 Mar 2017 15:12:08 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Dmitry Chagin <dchagin@freebsd.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r315498 - head/sys/compat/linux Message-ID: <20170319142933.I994@besplex.bde.org> In-Reply-To: <201703181814.v2IIEHxx066428@repo.freebsd.org> References: <201703181814.v2IIEHxx066428@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, 18 Mar 2017, Dmitry Chagin wrote:
> Log:
> Check for negative nanoseconds.
> Linux do that in timespec_valid().
This uses the unsigned hack to further obfuscate the code. No check is
necessary on any supported arch, since there are no representability
problems and FreeBSD checks for invalid nanoseconds natively.
> Modified: head/sys/compat/linux/linux_time.c
> ==============================================================================
> --- head/sys/compat/linux/linux_time.c Sat Mar 18 18:12:09 2017 (r315497)
> +++ head/sys/compat/linux/linux_time.c Sat Mar 18 18:14:17 2017 (r315498)
> @@ -142,7 +142,7 @@ linux_to_native_timespec(struct timespec
>
> LIN_SDT_PROBE2(time, linux_to_native_timespec, entry, ntp, ltp);
>
> - if (ltp->tv_sec < 0 || ltp->tv_nsec > (l_long)999999999L) {
> + if (ltp->tv_sec < 0 || (l_ulong)ltp->tv_nsec > 999999999L) {
> LIN_SDT_PROBE1(time, linux_to_native_timespec, return, EINVAL);
> return (EINVAL);
> }
Checking tv_sec < 0 breaks cases where negative times are valid. They
were valid in nanosleep() in FreeBSD, and the recent addition of
clock_nanosleep() doesn't seem to have broken this.
There is no check that tv_sec is representable. None is needed, since
native time_t is at least as large as linux time_t on all supported arches.
There is no check that large tv_nsec is representable, but the check that
large tv_nsec is valid not quite accidentally alo checks for
representability None is needed, since native long is at least as large
as linux l_long on all supported arches.
The check that large tv_nsec is valid had 2 type errors: explicit long
constant and bogus cast to undo this. These are only small style bug.
Now it still has the first error, and is obfuscated using an unsigned
hack. It takes a longer type analysis to see that this is just a style
bug (since l_long == int32_t == int on 32-bit systems, but the constant
is long so the expression doesn't have everything unsigned).
There was no check that large negative tv_nsec is representable, but no
check is necessary, as above. Now there is an unnecessary check, using
the unsigned hack, etc. It takes a MUCH longer type analysis to see
that this is just a style bug. E.g., consider the following subtle
variation:
if (ltp->tv_sec < 0 || (l_ulong)ltp->tv_nsec > 9999999999L) {
The only change is to add a 9. Of course, the constant is now wrong
for nansoseconds, but understanding why the unsigned hack is just
an obfuscation with the correct constant requires the same analysis.
Suppose l_ulong is uint32_t and tv_nsec is -1 and native is 64 bits.
Then on 32-bit systems, the cast gives UINT32_MAX = 0xFFFFFFFF (u_int,
not u_long, even on 32-bit systems). 9999999999L is larger than this,
so the comparison is tautologically false and the bug might be reported
by a compiler warning. 999999999L is not larger than this, so the
comparison works as intended, just like if the constant were correctly
written without an L suffix to obfuscate it -- then all the apparent
longs are actually ints, and the unsigned hack works as intended.
The L suffix doesn't make any difference here, except to obfuscate the
code and possibly cause or prevent compiler warnings. On 32-bit hosts,
the constant with the extra 9 is too large for long, so the compiler
can reasonably warn that the constant has type long long. gcc with
-std before 1999 warns whenever a constant is too large for long and
unless it has a [U]LL suffix, and compilers could unreasonbly have
the same sort of warnings for unsuffixed long constants. Anyway,
with such a large constant, you have to be careful about its type.
With no suffix, it might be either long or long long constant. The
constant for nanoseconds is about 1/2 of INT32_MAX = LONG_MAX on 32-bit
systems, so it is just below the limit for needing much care on 32-bit
systems.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20170319142933.I994>