From 169980337054fdc414d496e6b8e7c4dda9c53be4 Mon Sep 17 00:00:00 2001 From: tylerp Date: Wed, 31 Jan 2024 09:15:48 -0800 Subject: [PATCH 1/3] Resolved rollover bug in alarm.c gettimeasticks Previous implementation overflowed an intermediate calculation when computing the time in microseconds. This implementation resolves the overflow and also provides comments and assertion statements to guard against future overflow bugs. --- libtock/alarm_timer.c | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/libtock/alarm_timer.c b/libtock/alarm_timer.c index 27ef8b572..c1d06c7c0 100644 --- a/libtock/alarm_timer.c +++ b/libtock/alarm_timer.c @@ -3,6 +3,9 @@ #include "timer.h" #include #include +#include +#include +#define MAX_UINT32 ((uint32_t)(-1)) // Returns 0 if a <= b < c, 1 otherwise static int within_range(uint32_t a, uint32_t b, uint32_t c) { @@ -219,11 +222,29 @@ int gettimeasticks(struct timeval *tv, __attribute__ ((unused)) void *tzvp) alarm_internal_frequency(&frequency); alarm_internal_read(&now); + // The microsecond calculation will overflow in the intermediate scaling of + // (remainder * 1000) if the remainder is approximately greater than 4e6. Because + // remainder is calculated as now % frequency, we can define 0 <= remainder < frequency. + // This implies that the tv_usec must be of type uint64_t if frequency > 4MHz to avoid + // an overflow from occurring. We check this in the below assertion statement. + const uint32_t MAX_VALID_FREQ = 4000000; + assert(frequency < MAX_VALID_FREQ || sizeof(tv->tv_usec) == sizeof(uint64_t)); + + // Confirm frequency assumption + assert(frequency > 0); + seconds = now / frequency; remainder = now % frequency; + // NOTE: the drawback to this microsecond calculation is the potential loss of precision + // when scaling frequency / 1000 (lose 3 degrees of precision). At the time of this + // implementation (1/31/24), the Tock timer frequency struct provides support for + // frequencies such as 1KHz, 16KHz, 1MHz, etc. With such frequencies, there is not a loss + // of precision as the 3 least significant digits do not encode data. The only case of a lose + // in precision is for the frequency 32.768KHz. In this case, the loss of precision introduces ~1us + // of error. tv->tv_sec = seconds; - tv->tv_usec = (remainder * 1000 * 1000) / frequency; + tv->tv_usec = (remainder * 1000) / (frequency / 1000); return 0; } From 60123262e98fad9f5c4f85810138c6757dbc69db Mon Sep 17 00:00:00 2001 From: tylerp Date: Wed, 31 Jan 2024 09:30:45 -0800 Subject: [PATCH 2/3] Removed unnecessary header files used for testing --- libtock/alarm_timer.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/libtock/alarm_timer.c b/libtock/alarm_timer.c index c1d06c7c0..8cd116648 100644 --- a/libtock/alarm_timer.c +++ b/libtock/alarm_timer.c @@ -3,9 +3,6 @@ #include "timer.h" #include #include -#include -#include -#define MAX_UINT32 ((uint32_t)(-1)) // Returns 0 if a <= b < c, 1 otherwise static int within_range(uint32_t a, uint32_t b, uint32_t c) { From a938d0392d11b4214f1909696e3b14dff9a971f4 Mon Sep 17 00:00:00 2001 From: tylerp Date: Fri, 2 Feb 2024 16:30:50 -0800 Subject: [PATCH 3/3] updated formatting --- libtock/alarm_timer.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/libtock/alarm_timer.c b/libtock/alarm_timer.c index 8cd116648..eedbbb003 100644 --- a/libtock/alarm_timer.c +++ b/libtock/alarm_timer.c @@ -220,10 +220,10 @@ int gettimeasticks(struct timeval *tv, __attribute__ ((unused)) void *tzvp) alarm_internal_read(&now); // The microsecond calculation will overflow in the intermediate scaling of - // (remainder * 1000) if the remainder is approximately greater than 4e6. Because + // (remainder * 1000) if the remainder is approximately greater than 4e6. Because // remainder is calculated as now % frequency, we can define 0 <= remainder < frequency. - // This implies that the tv_usec must be of type uint64_t if frequency > 4MHz to avoid - // an overflow from occurring. We check this in the below assertion statement. + // This implies that the tv_usec must be of type uint64_t if frequency > 4MHz to avoid + // an overflow from occurring. We check this in the below assertion statement. const uint32_t MAX_VALID_FREQ = 4000000; assert(frequency < MAX_VALID_FREQ || sizeof(tv->tv_usec) == sizeof(uint64_t)); @@ -234,12 +234,12 @@ int gettimeasticks(struct timeval *tv, __attribute__ ((unused)) void *tzvp) remainder = now % frequency; // NOTE: the drawback to this microsecond calculation is the potential loss of precision - // when scaling frequency / 1000 (lose 3 degrees of precision). At the time of this + // when scaling frequency / 1000 (lose 3 degrees of precision). At the time of this // implementation (1/31/24), the Tock timer frequency struct provides support for - // frequencies such as 1KHz, 16KHz, 1MHz, etc. With such frequencies, there is not a loss + // frequencies such as 1KHz, 16KHz, 1MHz, etc. With such frequencies, there is not a loss // of precision as the 3 least significant digits do not encode data. The only case of a lose - // in precision is for the frequency 32.768KHz. In this case, the loss of precision introduces ~1us - // of error. + // in precision is for the frequency 32.768KHz. In this case, the loss of precision introduces ~1us + // of error. tv->tv_sec = seconds; tv->tv_usec = (remainder * 1000) / (frequency / 1000);