From 4eabc6db47fe64b6757f7a5f0e651a41f02efca3 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 14 Sep 2018 08:30:24 -0400 Subject: [PATCH 1/4] Use a slightly more accurate formula for OSX 32-bit msec conversion We use an optimized but less accurate formula for converting coarse time differences to milliseconds on 32-bit OSX platforms, so that we can avoid 64-bit division. The old numbers were off by 0.4%. The new numbers are off by .006%. This should make the unit tests a bit cleaner, and our tolerances a bit closer. --- src/common/compat_time.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/common/compat_time.c b/src/common/compat_time.c index 40847a8442..f92dc09c41 100644 --- a/src/common/compat_time.c +++ b/src/common/compat_time.c @@ -299,11 +299,11 @@ monotime_init_internal(void) } { // For converting ticks to milliseconds in a 32-bit-friendly way, we - // will first right-shift by 20, and then multiply by 20/19, since - // (1<<20) * 19/20 is about 1e6. We precompute a new numerate and + // will first right-shift by 20, and then multiply by 2048/1953, since + // (1<<20) * 1953/2048 is about 1e6. We precompute a new numerator and // denominator here to avoid multiple multiplies. - mach_time_info_msec_cvt.numer = mach_time_info.numer * 20; - mach_time_info_msec_cvt.denom = mach_time_info.denom * 19; + mach_time_info_msec_cvt.numer = mach_time_info.numer * 2048; + mach_time_info_msec_cvt.denom = mach_time_info.denom * 1953; } } From f02e8b5944c238979c0c0cf3ce4a8911026b2210 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 14 Sep 2018 08:36:33 -0400 Subject: [PATCH 2/4] Avoid integer overflow on fast 32-bit millisecond conversion. Multiply-then-divide is more accurate, but it runs into trouble when our input is above INT32_MAX/numerator. So when our value is too large, do divide-then-multiply instead. Fixes part of bug 27139; bugfix on 0.3.4.1-alpha. --- src/common/compat_time.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/src/common/compat_time.c b/src/common/compat_time.c index f92dc09c41..93b527def0 100644 --- a/src/common/compat_time.c +++ b/src/common/compat_time.c @@ -280,6 +280,7 @@ monotime_reset_ratchets_for_testing(void) */ static struct mach_timebase_info mach_time_info; static struct mach_timebase_info mach_time_info_msec_cvt; +static int32_t mach_time_msec_cvt_threshold; static int monotime_shift = 0; static void @@ -304,6 +305,10 @@ monotime_init_internal(void) // denominator here to avoid multiple multiplies. mach_time_info_msec_cvt.numer = mach_time_info.numer * 2048; mach_time_info_msec_cvt.denom = mach_time_info.denom * 1953; + // For any value above this amount, we should divide before multiplying, + // to avoid overflow. For a value below this, we should multiply + // before dividing, to improve accuracy. + mach_time_msec_cvt_threshold = INT32_MAX / mach_time_info_msec_cvt.numer; } } @@ -366,8 +371,13 @@ monotime_coarse_diff_msec32_(const monotime_coarse_t *start, /* We already require in di_ops.c that right-shift performs a sign-extend. */ const int32_t diff_microticks = (int32_t)(diff_ticks >> 20); - return (diff_microticks * mach_time_info_msec_cvt.numer) / - mach_time_info_msec_cvt.denom; + if (diff_microticks >= mach_time_msec_cvt_threshold) { + return (diff_microticks / mach_time_info_msec_cvt.denom) * + mach_time_info_msec_cvt.numer; + } else { + return (diff_microticks * mach_time_info_msec_cvt.numer) / + mach_time_info_msec_cvt.denom; + } } uint32_t From 6e5e1be7375e6e8de38af1f8c2c13e35d745edea Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 14 Sep 2018 08:40:10 -0400 Subject: [PATCH 3/4] Make circuitmux ewma timing test more tolerant on 32bit osx Since we use a 32-bit approximation for millisecond conversion here, we can't expect so much precision. Fixes part of bug 27139; bugfix on 0.3.4.1-alpha. --- src/common/compat_time.h | 1 + src/test/test_circuitmux.c | 13 +++++++++---- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/src/common/compat_time.h b/src/common/compat_time.h index 57ab20ab11..f241aa5eba 100644 --- a/src/common/compat_time.h +++ b/src/common/compat_time.h @@ -196,6 +196,7 @@ monotime_coarse_diff_msec32(const monotime_coarse_t *start, // on a 64-bit platform, let's assume 64/64 division is cheap. return (int32_t) monotime_coarse_diff_msec(start, end); #else +#define USING_32BIT_MSEC_HACK return monotime_coarse_diff_msec32_(start, end); #endif } diff --git a/src/test/test_circuitmux.c b/src/test/test_circuitmux.c index 14c7598703..c81d53ae51 100644 --- a/src/test/test_circuitmux.c +++ b/src/test/test_circuitmux.c @@ -13,6 +13,8 @@ #include "scheduler.h" #include "test.h" +#include + /* XXXX duplicated function from test_circuitlist.c */ static channel_t * new_fake_channel(void) @@ -103,16 +105,19 @@ test_cmux_compute_ticks(void *arg) monotime_coarse_set_mock_time_nsec(now); tick = cell_ewma_get_current_tick_and_fraction(&rem); tt_uint_op(tick, OP_EQ, tick_zero); - tt_double_op(rem, OP_GT, .149999999); - tt_double_op(rem, OP_LT, .150000001); +#ifdef USING_32BIT_MSEC_HACK + const double tolerance = .0005; +#else + const double tolerance = .00000001; +#endif + tt_double_op(fabs(rem - .15), OP_LT, tolerance); /* 25 second later and we should be in another tick. */ now = START_NS + NS_PER_S * 25; monotime_coarse_set_mock_time_nsec(now); tick = cell_ewma_get_current_tick_and_fraction(&rem); tt_uint_op(tick, OP_EQ, tick_zero + 2); - tt_double_op(rem, OP_GT, .499999999); - tt_double_op(rem, OP_LT, .500000001); + tt_double_op(fabs(rem - .5), OP_LT, tolerance); done: ; From 9a90f4c6b42cb2c251dae74f92d4192e171442e1 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 14 Sep 2018 08:49:27 -0400 Subject: [PATCH 4/4] Changes file for the 32-bit msec conversion fixes of #27139 --- changes/bug27139 | 14 ++++++++++++++ 1 file changed, 14 insertions(+) create mode 100644 changes/bug27139 diff --git a/changes/bug27139 b/changes/bug27139 new file mode 100644 index 0000000000..0d1e3b4329 --- /dev/null +++ b/changes/bug27139 @@ -0,0 +1,14 @@ + o Minor bugfixes (32-bit OSX and iOS, timing): + - Fix an integer overflow bug in our optimized 32-bit millisecond- + difference algorithm for 32-bit Apple platforms. Previously, it + would overflow when calculating the difference between two times + more than 47 days apart. Fixes part of bug 27139; bugfix on + 0.3.4.1-alpha. + - Improve the precision of our 32-bit millisecond difference + algorithm for 32-bit Apple platforms. Fixes part of bug 27139; + bugfix on 0.3.4.1-alpha. + - Relax the tolerance on the mainloop/update_time_jumps test + when running on 32-bit Apple platforms. Fixes part of bug 27139; + bugfix on 0.3.4.1-alpha. + +