From: Juergen Perlinger <perlinger@ntp.org>
Date: Wed, 10 Jan 2018 16:48:26 +0000 (+0100)
Subject: [Bug 3450] Dubious error messages from plausibility checks in get_systime()
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=6e3f7adb28473f33c33c63abdac6f7383b1306d0;p=thirdparty%2Fntp.git

[Bug 3450] Dubious error messages from plausibility checks in get_systime()

bk: 5a5643daDmC9pmH8GNVfSsTyKdphqA
---

diff --git a/ChangeLog b/ChangeLog
index b399fc835..ccbe39653 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -4,6 +4,8 @@
   - initial patch by <stenn@ntp.org>, extended by <perlinger@ntp.org>
 * [Sec 3412] ctl_getitem(): Don't compare names past NUL. <perlinger@ntp.org>
 * [Sec 3012] Sybil vulnerability: noepeer support.  HStenn, JPerlinger.
+* [Bug 3450] Dubious error messages from plausibility checks in get_systime()
+ - removed error log caused by rounding/slew, ensured postcondition <perlinger@ntp.org>
 * [Bug 3447] AES-128-CMAC (fixes) <perlinger@ntp.org>
   - refactoring the MAC code, too
 * [Bug 3441] Validate the assumption that AF_UNSPEC is 0.  stenn@ntp.org
diff --git a/libntp/systime.c b/libntp/systime.c
index d0a5359d0..18528438a 100644
--- a/libntp/systime.c
+++ b/libntp/systime.c
@@ -5,8 +5,10 @@
  *
  */
 #include <config.h>
+#include <math.h>
 
 #include "ntp.h"
+#include "ntpd.h"
 #include "ntp_syslog.h"
 #include "ntp_stdlib.h"
 #include "ntp_random.h"
@@ -111,7 +113,10 @@ set_sys_fuzz(
 	sys_fuzz = fuzz_val;
 	INSIST(sys_fuzz >= 0);
 	INSIST(sys_fuzz <= 1.0);
-	sys_fuzz_nsec = (long)(sys_fuzz * 1e9 + 0.5);
+	/* [Bug 3450] ensure nsec fuzz >= sys_fuzz to reduce chance of
+	 * short falling fuzz advance
+	 */
+	sys_fuzz_nsec = (long)ceil(sys_fuzz * 1e9);
 }
 
 
@@ -169,13 +174,10 @@ get_systime(
         static struct timespec  ts_last;        /* last sampled os time */
 	static struct timespec	ts_prev;	/* prior os time */
 	static l_fp		lfp_prev;	/* prior result */
-	static double		dfuzz_prev;	/* prior fuzz */
 	struct timespec ts;	/* seconds and nanoseconds */
 	struct timespec ts_min;	/* earliest permissible */
 	struct timespec ts_lam;	/* lamport fictional increment */
-	struct timespec ts_prev_log;	/* for msyslog only */
 	double	dfuzz;
-	double	ddelta;
 	l_fp	result;
 	l_fp	lfpfuzz;
 	l_fp	lfpdelta;
@@ -217,21 +219,16 @@ get_systime(
 			if (!lamport_violated)
 				ts = ts_min;
 		}
-		ts_prev_log = ts_prev;
 		ts_prev = ts;
-	} else {
-		/*
-		 * Quiet "ts_prev_log.tv_sec may be used uninitialized"
-		 * warning from x86 gcc 4.5.2.
-		 */
-		ZERO(ts_prev_log);
 	}
 
 	/* convert from timespec to l_fp fixed-point */
 	result = tspec_stamp_to_lfp(ts);
 
 	/*
-	 * Add in the fuzz.
+	 * Add in the fuzz. 'ntp_random()' returns [0..2**31-1] so we
+	 * must scale up the result by 2.0 to cover the full fractional
+	 * range.
 	 */
 	dfuzz = ntp_random() * 2. / FRAC * sys_fuzz;
 	DTOLFP(dfuzz, &lfpfuzz);
@@ -241,30 +238,33 @@ get_systime(
 	 * Ensure result is strictly greater than prior result (ignoring
 	 * sys_residual's effect for now) once sys_fuzz has been
 	 * determined.
+	 *
+	 * [Bug 3450] Rounding errors and time slew can lead to a
+	 * violation of the expected postcondition. This is bound to
+	 * happen from time to time (depending on state of the random
+	 * generator, the current slew and the closeness of system time
+	 * stamps drawn) and does not warrant a syslog entry. Instead it
+	 * makes much more sense to ensure the postcondition and hop
+	 * along silently.
 	 */
 	if (!USING_SIGIO()) {
-		if (!L_ISZERO(&lfp_prev) && !lamport_violated) {
-			if (!L_ISGTU(&result, &lfp_prev) &&
-			    sys_fuzz > 0.) {
-				msyslog(LOG_ERR, "ts_prev %s ts_min %s",
-					tspectoa(ts_prev_log),
-					tspectoa(ts_min));
-				msyslog(LOG_ERR, "ts %s", tspectoa(ts));
-				msyslog(LOG_ERR, "sys_fuzz %ld nsec, prior fuzz %.9f",
-					sys_fuzz_nsec, dfuzz_prev);
-				msyslog(LOG_ERR, "this fuzz %.9f",
-					dfuzz);
-				lfpdelta = lfp_prev;
-				L_SUB(&lfpdelta, &result);
-				LFPTOD(&lfpdelta, ddelta);
-				msyslog(LOG_ERR,
-					"prev get_systime 0x%x.%08x is %.9f later than 0x%x.%08x",
-					lfp_prev.l_ui, lfp_prev.l_uf,
-					ddelta, result.l_ui, result.l_uf);
+		if (!L_ISZERO(&lfp_prev) &&
+		    !lamport_violated    &&
+		    (sys_fuzz > 0.0)      )
+		{
+			lfpdelta = result;
+			L_SUB(&lfpdelta, &lfp_prev);
+			L_SUBUF(&lfpdelta, 1);
+			if (lfpdelta.l_i < 0)
+			{
+				L_NEG(&lfpdelta);
+				DPRINTF(1, ("get_systime: postcond failed by %s secs, fixed\n",
+					    lfptoa(&lfpdelta, 9)));
+				result = lfp_prev;
+				L_ADDUF(&result, 1);
 			}
 		}
 		lfp_prev = result;
-		dfuzz_prev = dfuzz;
 		if (lamport_violated) 
 			lamport_violated = FALSE;
 	}