Date: Sun, 20 Mar 2016 16:26:37 +0300 From: Chagin Dmitry <dchagin@freebsd.org> To: Bruce Evans <brde@optusnet.com.au> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r296543 - head/sys/compat/linux Message-ID: <20160320132637.GA88466@chd.heemeyer.club> In-Reply-To: <20160309160342.P1249@besplex.bde.org> References: <201603081920.u28JKvbM088851@repo.freebsd.org> <20160309160342.P1249@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, Mar 09, 2016 at 07:16:27PM +1100, Bruce Evans wrote:
> On Tue, 8 Mar 2016, Dmitry Chagin wrote:
>
> > Log:
> > Put a commit message from r296502 about Linux alarm() system call
> > behaviour to the source.
> > ...
> > Modified: head/sys/compat/linux/linux_misc.c
> > ==============================================================================
> > --- head/sys/compat/linux/linux_misc.c Tue Mar 8 19:08:55 2016 (r296542)
> > +++ head/sys/compat/linux/linux_misc.c Tue Mar 8 19:20:57 2016 (r296543)
> > @@ -206,6 +206,11 @@ linux_alarm(struct thread *td, struct li
> > it.it_value.tv_usec = 0;
> > it.it_interval.tv_sec = 0;
> > it.it_interval.tv_usec = 0;
> > + /*
> > + * According to POSIX and Linux implementation
> > + * the alarm() system call is always successfull.
> > + * Ignore errors and return 0 as a Linux do.
> > + */
>
> Why does this need a comment referring to external sources? FreeBSD's
> own man page also says that there is no error return for alarm(3).
> However, the man page and the implementation are quite broken. The
> implementation in libc does have an error return, but this is
> undocumented. The documentation says that that the maximum number of
> seconds is 100000000 (100 million), but doesn't say what happens when
> this limit is exceeded. The implementation of this is broken too.
> What actually happens for the libc version is:
> - secs <= 100 million works in all kernel versions
> - in old kernel versions, secs > 100 million fails to set the alarm;
> it sets errno to EINVAL and returns (u_int)-1 to misindicate the error
> - in FreeBSD-~[6-9], secs > 100 million works with 64-bit time_t. With
> 32-bit time_t, secs > INT_MAX fails; secs between INT_MAX and about
> 1000 million (= the time from now until overflow) causes undefined
> behaviour due to overflow, but this might simulate working; secs between
> about 1000 million and 100 million work.
> - in FreeBSD-~[10-current], secs > INT32_MAX / 2 fail and other cases have
> a chance of working. In the failing cases, the error is misindicated as
> in old versions except for the different threshold.
>
> > kern_setitimer(td, ITIMER_REAL, &it, &old_it);
>
> Removing the error check broke this completely.
>
> > if (timevalisset(&old_it.it_value)) {
> > if (old_it.it_value.tv_usec != 0)
>
> old_it is stack garbage when kern_setitimer() fails. The value in
> this stack garbage is now returned and errno is not set. When the
> error was checked, the consistent garbage value (u_int)-1 was returned,
> and errno was not set.
>
> This function worked almost correctly in FreeBSD-5, by duplicating
> most of the internals of setitimer() including its limit of 100 million
> which was not broken then. This limit was to avoid overflow with 32-bit
> time_t unless the current time is after year 2035. It was broken in
> 2005. It remained just broken with 32-bit time_t until FreeBSD-9.
> Starting in FreeBSD-10, sbintime_t is used. This reduces the brokenness
> with 32-bit time_t but increases it with 64-bit time_t. sbintime_t has
> the same signed 32-bit limit for seconds as 32-bit time_t. New ittimer
> code using sbintime_t is more careful about overflow, but it cannot
> support alarm() or large times in setitimer() even with 64-bit time_t.
> It actually enforces a limit of INT32_MAX / 2 (~ 1000 million) and
> doesn't documement this, where old code enforces a limit of 100 million
> and does document this. Man pages still document the old limit, but
> no implmentations of alarm() except the old one for linux are aware
> of this.
>
> Complete description of bugs in this function:
>
> X int
> X linux_alarm(struct thread *td, struct linux_alarm_args *args)
> X {
> X struct itimerval it, old_it;
> X u_int secs;
> X
> X #ifdef DEBUG
> X if (ldebug(alarm))
> X printf(ARGS(alarm, "%u"), args->secs);
> X #endif
> X
> X secs = args->secs;
> X
>
> Style bug: extra blank line to separate related code. Strict KNF doesn't
> even allow blank lines to separate unrelated code. The one after the
> DEBUG ifdef is an example. But bugs in indent(1) give extra blank lines
> for ifdefs.
>
> X if (secs > INT_MAX)
> X secs = INT_MAX;
>
> A bounds check is needed, but this one is very wrong. We are going to
> assign secs to tv_sec, and need to ensure that this doesn't overflow.
> tv_sec used to have type long, so the correct limit for this was LONG_MAX.
> POSIX broke this type, and FreeBSD was broken to conform in 2005. Then
> the correct limit became TIME_T_MAX, but this is unavailable under that
> spelling. INT_MAX accidentally works as well as possible for its intended
> purpose it time_t is 32 bits and int is also 32 bits, but if time_t is
> 64-bits then it unnecessarily breaks alarm() in versions before sbintime_t.
>
> However, the limit of INT_MAX doesn't work for the purpose of breaking
> alarm() as little as possible. It just ensured that kern_setitimer()
> and thus this function always fails if kern_settimer() enforced its
> documented limit of 100 million. I think the change to use this limit
> was made after setitimer()'s limit was broken. Then this limit worked
> for a while with 64-bit time_t, but with 32-bit time_t, using it always
> gave overflow by adding INT_MAX to the current time. Now with
> sbintime_t and its limit of INT32_MAX / 2, using INT_MAX here ensures
> that kern_settimer() and thus this function always fails...
>
> The best limit to use here is 100 million, or perhaps INT32_MAX / 2 to
> match kern_setitimer()s new limit, after checking that this works
> (sbintime_t must be careful not to add INT32_MAX / 2 to either the
> current time or more than 1 other value near INT_MAX32 / 2).
>
> X
>
> Style bug: extra blank line.
>
> X it.it_value.tv_sec = (long) secs;
>
> Style bugs: bogus cast, and space before cast. The cast was not incorrect
> before POSIX broke the type of tv_sec, but it should never have been
> necessary (for avoiding compiler warnings) since compilers should see
> that the limited value fits in tv_sec.
>
> X it.it_value.tv_usec = 0;
> X it.it_interval.tv_sec = 0;
> X it.it_interval.tv_usec = 0;
>
> Style bug in previous 2 lines: the corresponding libc code uses
> timevalclear(). (Normally I prefer explicit code, but this function uses
> a macro later.)
>
> X /*
> X * According to POSIX and Linux implementation
> X * the alarm() system call is always successfull.
> X * Ignore errors and return 0 as a Linux does.
> X */
>
> Style bugs: dubious commit, and formatting of this comment for ~60 column
> terminals. There are many delicate points in the error (mis)handling,
> but the comment only describes an uninteresting one. Comments should
> probably be formatted narrow terminals, but that is an unusual style.
> Other comment in this file use random right margins but many go out to
> nearly column 89.
>
> X kern_setitimer(td, ITIMER_REAL, &it, &old_it);
>
> Bug: lost error handling.
>
> After fixing the above limit of INT_MAX, the error here should never
> occur, and you could assert that, but the error always occurs now
> if secs > INT32_MAX / 2. secs <= INT32_MAX / 2 is so sure to work with
> the current sbintime_t code that this is not worth asserting. (Perhaps
> it will fail due to overflow, but kern_setitimer() will succeed.)
> secs <= 100000000 is even more sure to work (until 2035).
>
> X if (timevalisset(&old_it.it_value)) {
>
> Bug: beginning of accesses to the uninitialized variable old_it in the
> error case.
>
> Style bugs: unnecessary test, and inconsistent use of macros. libc
> doesn't do this test. All it does is extra work in the test to avoid
> doing anything more in the usual case where the old timeout has expired.
>
> X if (old_it.it_value.tv_usec != 0)
> X old_it.it_value.tv_sec++;
> X td->td_retval[0] = old_it.it_value.tv_sec;
>
> Bug: return of garbage in the error case.
>
> X }
> X return (0);
> X }
>
> Bruce
Thank you very match, Bruce for you pont! Should I correct setitimer/alarm man pages
about maximum seconds?
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20160320132637.GA88466>