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?=
<ikar...@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?=
<ikar...@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?=
<ikar...@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
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;
}
