From owner-svn-src-head@freebsd.org Sun Mar 19 04:12:10 2017 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id C9361D137D6; Sun, 19 Mar 2017 04:12:10 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail107.syd.optusnet.com.au (mail107.syd.optusnet.com.au [211.29.132.53]) by mx1.freebsd.org (Postfix) with ESMTP id 53997FC7; Sun, 19 Mar 2017 04:12:09 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from besplex.bde.org (c122-106-153-191.carlnfd1.nsw.optusnet.com.au [122.106.153.191]) by mail107.syd.optusnet.com.au (Postfix) with ESMTPS id BAB34D47D73; Sun, 19 Mar 2017 15:12:08 +1100 (AEDT) Date: Sun, 19 Mar 2017 15:12:08 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Dmitry Chagin cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r315498 - head/sys/compat/linux In-Reply-To: <201703181814.v2IIEHxx066428@repo.freebsd.org> Message-ID: <20170319142933.I994@besplex.bde.org> References: <201703181814.v2IIEHxx066428@repo.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.2 cv=AYLBJzfG c=1 sm=1 tr=0 a=Tj3pCpwHnMupdyZSltBt7Q==:117 a=Tj3pCpwHnMupdyZSltBt7Q==:17 a=kj9zAlcOel0A:10 a=sawmzL94VX9KUcPa1PAA:9 a=CjuIK1q_8ugA:10 X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 19 Mar 2017 04:12:10 -0000 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