Unit test util/time is broken on OpenBSD

This is in master, I think we should check if it's in 0.2.8 and fix it before the stable release:

util/time: 
  FAIL src/test/test_util.c:302: assert(LONG_MAX OP_EQ tv_udiff(&start, &end)): 2147483647 vs -6005000
  [time FAILED]

https://buildbot.pixelminers.net/builders/OpenBSD/builds/980/steps/shell_3/logs/stdio

changed milestone to %Tor: 0.2.9.x-final

added TorCoreTeam201607 actualpoints::1.0 component::core tor/tor integer-safety milestone::Tor: 0.2.9.x-final points::0.5 priority::medium resolution::fixed severity::normal status::closed type::defect labels

I'd love to fix this, but I really need an OpenBSD box to test the fix works for all the cases, or someone to test my proposed fixes on their OpenBSD box.

There are a few different issues here:

Some BSDs have a 64-bit tv_sec, but a 32-bit time_t. Source: https://www.gnu.org/software/gnulib/manual/html_node/sys_002ftime_002eh.html The unit test failure on those BSDs doesn't affect 0.2.8, because the failing test was only introduced in 0.2.9.

However, the integer truncation / overflow issue when time_t and tv_sec are different sizes still affects BSDs on 0.2.8. But we've changed the code in tv_udiff and tv_mdiff to fix a different bug between 0.2.8 and master, so it's not worth going back to change it.

There have been integer overflow errors in tv_udiff and tv_mdiff on all platforms in every released version of tor. I think this is ok, because we typically compare internal clocks with these functions. But I didn't check to see if we ever use times that come from the network. And now we are compiling with -ftrapv, we really need to fix this issue up.

I modified tv_udiff and tv_mdiff to use 64-bit integers and check for overflow internally, but they still output a long. We can fix the interfaces to all the time functions to be 64-bit clean in #18480 (moved).

Please see my branch bug19483-v2 on https://github.com/teor2345/tor.git It's based on master (0.2.9). It comes with comprehensive unit tests, that exercise several different kinds of overflow, and check the rounding behaviour of tv_mdiff. (They don't test 64-bit tv_sec values on 32-bit BSDs, because that would have meant finding the size of tv_sec at compile time.)

I have tested it on OS X x86_64 and i386, and Linux x86_64. It needs testing on multiple platforms (especially Windows and 32-bit BSDs with 64-bit tv_sec), but I think that's best done by merging to master.)

Trac:
Actualpoints: N/A to 1.0
Keywords: N/A deleted, integer-safety, TorCoreTeam201607 added
Milestone: Tor: 0.2.8.x-final to Tor: 0.2.9.x-final
Status: new to needs_review

ok, merging. (Wow, math in C is hard.)

Trac:
Resolution: N/A to fixed
Status: needs_review to closed

Trac:
Status: closed to reopened
Resolution: fixed to N/A

gcc on amd64 (and only amd64) is smart enough to elide some of my overflow checks:

../src/common/util.c: In function 'tv_udiff':
../src/common/util.c:1454:3: error: logical 'or' of collectively exhaustive tests is always true [-Werror=logical-op]
../src/common/util.c: In function 'tv_mdiff':
../src/common/util.c:1510:3: error: logical 'or' of collectively exhaustive tests is always true [-Werror=logical-op]
cc1: all warnings being treated as errors

Please see my branch bug19483-fix-v2, which skips the range check on L64 platforms, because it's unreachable.

I also added some unit tests with negative tv_sec values. Let's hope no-one has declared tv_sec to be unsigned.

Trac:
Status: reopened to needs_review

lgtm; merged to master; thanks!

Trac:
Status: needs_review to closed
Resolution: N/A to fixed

closed

changed time estimate to 4h

added 8h of time spent

moved to tpo/core/tor#19483 (closed)