[PATCH] Multiple timer issues

[IrÃnyossy Knoblauch ArtÃr <ikartur at gmail dot com>]

Dear Cygwin Maintainers,

First of all, thank you for your work, I really enjoy using this software!

However, I have noticed that adjusting the system time can cause some
programs to misbehave. I have found bugs in the POSIX timer
implementation and a bug in the select() function's timeout handling.

Please find the proposed patches attached.


Regarding POSIX timers:

I have also created a small test application (see timer_test.c and the
Makefile) to demonstrate the issue. Please try to run it on both Linux
and Cygwin!

The test tries to set the system time back and forth to see the effect
on different kinds of timers. Please note, that for setting the system
time, the test has to be run with the necessary administrative rights
provided.


Regarding select():

The timeout shall be immune to adjustments to the system clock in all
cases; so the 'gtod' clock shouldn't be used, because it is not
monotonic.


I have tried to keep the changes as minimal as possible.
I hope that signing a legal agreement is not necessary, since these
are just bugfixes; if you think otherwise, please let me know.

Best Regards,
ArtÃr

From 4e5655a3d4381643ede3ddc758bfa05a4f1d40f0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ir=C3=A1nyossy=20Knoblauch=20Art=C3=BAr?=
 <ikartur@gmail.com>
Date: Mon, 15 Feb 2016 22:24:36 +0100
Subject: [PATCH 1/3] POSIX timer fixes

 * Make relative timers immune to system clock changes
 * Add CLOCK_MONOTONIC support

According to the Linux Programmer's Manual (man page of timer_settime):

"If the value of the CLOCK_REALTIME clock is adjusted while an absolute
timer based on that clock is armed, then the expiration of the timer
will be appropriately adjusted. Adjustments to the CLOCK_REALTIME clock
have no effect on relative timers based on that clock."

Only timers based on CLOCK_REALTIME created with the TIMER_ABSTIME flag
shall be synchronized to the system clock.
Other timers must be immune to system clock adjustments.
---
 winsup/cygwin/hires.h  |  4 +++-
 winsup/cygwin/timer.cc | 26 ++++++++++++++++++++++----
 2 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/winsup/cygwin/hires.h b/winsup/cygwin/hires.h
index 23b2391..38a1e9e 100644
--- a/winsup/cygwin/hires.h
+++ b/winsup/cygwin/hires.h
@@ -48,8 +48,9 @@ class hires_ns : public hires_base
   double freq;
   void prime ();
  public:
-  LONGLONG nsecs (bool monotonic = false);
+  LONGLONG nsecs (bool monotonic = true);
   LONGLONG usecs () {return nsecs () / 1000LL;}
+  LONGLONG msecs () {return nsecs () / 1000000LL;}
   LONGLONG resolution();
 };
 
@@ -62,5 +63,6 @@ class hires_ms : public hires_base
   UINT resolution ();
 };
 
+extern hires_ns ntod;
 extern hires_ms gtod;
 #endif /*__HIRES_H__*/
diff --git a/winsup/cygwin/timer.cc b/winsup/cygwin/timer.cc
index bfa1495..dfbd2ec 100644
--- a/winsup/cygwin/timer.cc
+++ b/winsup/cygwin/timer.cc
@@ -28,6 +28,8 @@ struct timer_tracker
   HANDLE syncthread;
   long long interval_us;
   long long sleepto_us;
+  bool is_monotonic;
+  long long usecs();
   bool cancel ();
   struct timer_tracker *next;
   int settime (int, const itimerspec *, itimerspec *);
@@ -59,6 +61,15 @@ lock_timer_tracker::~lock_timer_tracker ()
   protect.release ();
 }
 
+inline long long
+timer_tracker::usecs()
+{
+  if (is_monotonic)
+    return ntod.usecs();
+  else
+    return gtod.usecs();
+}
+
 bool
 timer_tracker::cancel ()
 {
@@ -99,6 +110,7 @@ timer_tracker::timer_tracker (clockid_t c, const sigevent *e)
   clock_id = c;
   magic = TT_MAGIC;
   hcancel = NULL;
+  is_monotonic = true;
   if (this != &ttstart)
     {
       lock_timer_tracker here;
@@ -128,7 +140,7 @@ timer_thread (VOID *x)
       LONG sleep_ms;
       /* Account for delays in starting thread
 	and sending the signal */
-      now = gtod.usecs ();
+      now = tt->usecs ();
       sleep_us = sleepto_us - now;
       if (sleep_us > 0)
 	{
@@ -232,7 +244,13 @@ timer_tracker::settime (int in_flags, const itimerspec *value, itimerspec *ovalu
       if (it_bad (value->it_value) || it_bad (value->it_interval))
 	__leave;
 
-      long long now = in_flags & TIMER_ABSTIME ? 0 : gtod.usecs ();
+      if ((in_flags & TIMER_ABSTIME) && (clock_id == CLOCK_REALTIME))
+        is_monotonic = false;
+      else
+        is_monotonic = true;
+
+
+      long long now = in_flags & TIMER_ABSTIME ? 0 : usecs ();
 
       lock_timer_tracker here;
       cancel ();
@@ -272,7 +290,7 @@ timer_tracker::gettime (itimerspec *ovalue)
   else
     {
       ovalue->it_interval = it_interval;
-      long long now = gtod.usecs ();
+      long long now = usecs ();
       long long left_us = sleepto_us - now;
       if (left_us < 0)
        left_us = 0;
@@ -317,7 +335,7 @@ timer_create (clockid_t clock_id, struct sigevent *__restrict evp,
 	  return -1;
 	}
 
-      if (clock_id != CLOCK_REALTIME)
+      if (clock_id != CLOCK_REALTIME && clock_id != CLOCK_MONOTONIC)
 	{
 	  set_errno (EINVAL);
 	  return -1;
-- 
1.9.1

From 98ce78460bdb9b81313d3342c74f24484373034c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ir=C3=A1nyossy=20Knoblauch=20Art=C3=BAr?=
 <ikartur@gmail.com>
Date: Mon, 15 Feb 2016 22:55:06 +0100
Subject: [PATCH 2/3] Make select() immune to system clock adjustments

---
 winsup/cygwin/select.cc | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/winsup/cygwin/select.cc b/winsup/cygwin/select.cc
index e1d48a3..5eb3417 100644
--- a/winsup/cygwin/select.cc
+++ b/winsup/cygwin/select.cc
@@ -133,7 +133,7 @@ select (int maxfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
   int ret = 0;
 
   /* Record the current time for later use. */
-  LONGLONG start_time = gtod.msecs ();
+  LONGLONG start_time = ntod.msecs ();
 
   select_stuff sel;
   sel.return_on_signal = 0;
@@ -212,7 +212,7 @@ select (int maxfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
       if (wait_state == select_stuff::select_loop && ms != INFINITE)
 	{
 	  select_printf ("recalculating ms");
-	  LONGLONG now = gtod.msecs ();
+	  LONGLONG now = ntod.msecs ();
 	  if (now > (start_time + ms))
 	    {
 	      select_printf ("timed out after verification");
-- 
1.9.1

From bdf9f52d4c56a9b7c3db46f2a134b54c75701308 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ir=C3=A1nyossy=20Knoblauch=20Art=C3=BAr?=
 <ikartur@gmail.com>
Date: Mon, 15 Feb 2016 23:00:28 +0100
Subject: [PATCH 3/3] Eliminate dead code

---
 winsup/cygwin/hires.h  |  3 +--
 winsup/cygwin/times.cc | 18 +++---------------
 2 files changed, 4 insertions(+), 17 deletions(-)

diff --git a/winsup/cygwin/hires.h b/winsup/cygwin/hires.h
index 38a1e9e..8311ad1 100644
--- a/winsup/cygwin/hires.h
+++ b/winsup/cygwin/hires.h
@@ -44,11 +44,10 @@ class hires_base
 
 class hires_ns : public hires_base
 {
-  LARGE_INTEGER primed_pc;
   double freq;
   void prime ();
  public:
-  LONGLONG nsecs (bool monotonic = true);
+  LONGLONG nsecs ();
   LONGLONG usecs () {return nsecs () / 1000LL;}
   LONGLONG msecs () {return nsecs () / 1000000LL;}
   LONGLONG resolution();
diff --git a/winsup/cygwin/times.cc b/winsup/cygwin/times.cc
index f0359bf..cb498d7 100644
--- a/winsup/cygwin/times.cc
+++ b/winsup/cygwin/times.cc
@@ -483,23 +483,12 @@ hires_ns::prime ()
       return;
     }
 
-  int priority = GetThreadPriority (GetCurrentThread ());
-
-  SetThreadPriority (GetCurrentThread (), THREAD_PRIORITY_TIME_CRITICAL);
-  if (!QueryPerformanceCounter (&primed_pc))
-    {
-      SetThreadPriority (GetCurrentThread (), priority);
-      inited = -1;
-      return;
-    }
-
   freq = (double) ((double) 1000000000. / (double) ifreq.QuadPart);
   inited = true;
-  SetThreadPriority (GetCurrentThread (), priority);
 }
 
 LONGLONG
-hires_ns::nsecs (bool monotonic)
+hires_ns::nsecs ()
 {
   if (!inited)
     prime ();
@@ -517,8 +506,7 @@ hires_ns::nsecs (bool monotonic)
     }
 
   // FIXME: Use round() here?
-  now.QuadPart = (LONGLONG) (freq * (double)
-		 (now.QuadPart - (monotonic ? 0LL : primed_pc.QuadPart)));
+  now.QuadPart = (LONGLONG) (freq * (double)(now.QuadPart));
   return now.QuadPart;
 }
 
@@ -605,7 +593,7 @@ clock_gettime (clockid_t clk_id, struct timespec *tp)
 
       case CLOCK_MONOTONIC:
 	{
-	  LONGLONG now = ntod.nsecs (true);
+	  LONGLONG now = ntod.nsecs ();
 	  if (now == (LONGLONG) -1)
 	    return -1;
 
-- 
1.9.1

Attachment: Makefile
Description: Binary data

#include <stdlib.h>
#include <stdio.h>
#include <time.h>
#include <signal.h>
#include <string.h>
#include <unistd.h>

struct test_case {

	clockid_t clock;
	timer_t timer_id;
	int flags;
	struct itimerspec end;
	int error;
	int result_ms;

	// is the result shall be altered by setting the system clock?
	int is_altered;

};

static struct test_case tests[] = {
	{ .clock = CLOCK_REALTIME, .flags = TIMER_ABSTIME, .is_altered = 1 },
	{ .clock = CLOCK_MONOTONIC, .flags = TIMER_ABSTIME},
	{ .clock = CLOCK_REALTIME},
	{ .clock = CLOCK_MONOTONIC}
};

static const int num_tests = sizeof(tests)/sizeof(tests[0]);

void shift_time(int t)
{
	int error = 0;

	struct timespec tp;
	error |= clock_gettime(CLOCK_REALTIME, &tp);
	if (!error) {
		tp.tv_sec += t;
		error |= clock_settime(CLOCK_REALTIME, &tp);
	}
	if (error) {
		printf("Couldn't set the system clock.\n"
		       "(Please try again with the necessary administrative rights.)\n");
		exit(1);
	}
}

int main()
{

	// Alter the system clock
	// (will be set back to the correct value later)
	shift_time(-2);
	
	// Setup
	for(int i=0; i < num_tests; ++i) {

		struct sigevent evt = { .sigev_notify = SIGEV_NONE };
		
		tests[i].error |= timer_create(tests[i].clock, &evt, &tests[i].timer_id);

		if (tests[i].flags & TIMER_ABSTIME) {
			tests[i].error |= clock_gettime(tests[i].clock, &tests[i].end.it_value);
		}

		tests[i].end.it_value.tv_sec += 5;
		tests[i].error |= timer_settime(tests[i].timer_id, tests[i].flags, &tests[i].end, NULL);
	}

	sleep(1);

	// Shift time forward
	// (correcting the system time as a result)
	shift_time(2);

	// get remaining times
	for(int i=0; i < num_tests; ++i) {
		struct itimerspec remaining;
		tests[i].error |= timer_gettime(tests[i].timer_id, &remaining);
		tests[i].result_ms = remaining.it_value.tv_sec*1000;
		tests[i].result_ms += (remaining.it_value.tv_nsec+500*1000)/(1000*1000);
	}

	// Report results
	int overall_ok = 1;
	for(int i=0; i < num_tests; ++i) {
		if (tests[i].clock == CLOCK_REALTIME) { printf("CLOCK_REALTIME,  "); }
		if (tests[i].clock == CLOCK_MONOTONIC) { printf("CLOCK_MONOTONIC, "); }
		if (tests[i].flags & TIMER_ABSTIME) {
			printf("absolute: ");
		}
		else {
			printf("relative: ");
		}
		if (tests[i].error) {
			overall_ok = 0;
			puts("ERROR");
			continue;
		}
		printf("%d ms, ", tests[i].result_ms);

		int expected_ms = tests[i].is_altered ? 2000 : 4000;
		if (abs(tests[i].result_ms - expected_ms) < 500) {
			puts("OK");
		}
		else {
			puts("Failed");
			overall_ok = 0;
		}
	}

	printf("\nResult: %s\n", overall_ok ? "OK" : "Failed");
	return 0;
}