[PATCH] Multiple timer issues

[PATCH] Multiple timer issues

[Irányossy Knoblauch Artúr]

[-- Attachment #1: Type: text/plain, Size: 1166 bytes --]

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

[-- Attachment #2: 0001-POSIX-timer-fixes.patch --]
[-- Type: text/x-patch, Size: 3813 bytes --]

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


[-- Attachment #3: 0002-Make-select-immune-to-system-clock-adjustments.patch --]
[-- Type: text/x-patch, Size: 1176 bytes --]

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


[-- Attachment #4: 0003-Eliminate-dead-code.patch --]
[-- Type: text/x-patch, Size: 2136 bytes --]

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 #5: Makefile --]
[-- Type: application/octet-stream, Size: 126 bytes --]

all: timer_test
	
timer_test: timer_test.c
	gcc -std=gnu99 -g -Wall timer_test.c -lrt -o timer_test

clean:
	rm -f timer_test

[-- Attachment #6: timer_test.c --]
[-- Type: text/x-csrc, Size: 2590 bytes --]

#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;
}

Re: [PATCH] Multiple timer issues

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 2103 bytes --]

Hi Artúr,

On Feb 17 23:30, Irányossy Knoblauch Artúr wrote:
> Dear Cygwin Maintainers,
> 
> First of all, thank you for your work, I really enjoy using this software!

Thank you :)

> 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.

I checked your patches and they look good.  I like it that this weird
priming can go away.

> 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.

Patches 2 can go in as trivial, patch 3 too with a little bit of a
stretch.  Unfortunately your first patch is too big to go in as trivial.
Would you mind terribly to send a copyright assignment per
https://cygwin.com/contrib.html?  If you send it as PDF by mail it takes
usually just a few days to be countersigned.

I would apply patch 2 immediately, but as far as I can see it relies
on patch 1.  Without patch 1, exchanging gtod with ntod will not change
anything since it's still a non-monotonic timer.  Or am I missing
something?


Thanks a lot,
Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] Multiple timer issues

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 2532 bytes --]

Artúr,

Ping?  Any news on your CA?  Did you get my reply to your mail to
ges-info?


Thanks,
Corinna

On Feb 18 12:28, Corinna Vinschen wrote:
> Hi Artúr,
> 
> On Feb 17 23:30, Irányossy Knoblauch Artúr wrote:
> > Dear Cygwin Maintainers,
> > 
> > First of all, thank you for your work, I really enjoy using this software!
> 
> Thank you :)
> 
> > 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.
> 
> I checked your patches and they look good.  I like it that this weird
> priming can go away.
> 
> > 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.
> 
> Patches 2 can go in as trivial, patch 3 too with a little bit of a
> stretch.  Unfortunately your first patch is too big to go in as trivial.
> Would you mind terribly to send a copyright assignment per
> https://cygwin.com/contrib.html?  If you send it as PDF by mail it takes
> usually just a few days to be countersigned.
> 
> I would apply patch 2 immediately, but as far as I can see it relies
> on patch 1.  Without patch 1, exchanging gtod with ntod will not change
> anything since it's still a non-monotonic timer.  Or am I missing
> something?
> 
> 
> Thanks a lot,
> Corinna
> 
> -- 
> Corinna Vinschen                  Please, send mails regarding Cygwin to
> Cygwin Maintainer                 cygwin AT cygwin DOT com
> Red Hat



-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]