Opened 4 years ago

Closed 4 years ago

#19483 closed defect (fixed)

Unit test util/time is broken on OpenBSD

Reported by: teor Owned by:
Priority: Medium Milestone: Tor: 0.2.9.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: integer-safety, TorCoreTeam201607
Cc: Actual Points: 1.0
Parent ID: Points: 0.5
Reviewer: Sponsor:

Description

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

Child Tickets

Change History (7)

comment:1 Changed 4 years ago by teor

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.

comment:2 Changed 4 years ago by teor

Actual Points: 1.0
Keywords: integer-safety TorCoreTeam201607 added
Milestone: Tor: 0.2.8.x-finalTor: 0.2.9.x-final
Status: newneeds_review

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.

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

comment:3 Changed 4 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

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

comment:4 Changed 4 years ago by teor

Resolution: fixed
Status: closedreopened

comment:5 Changed 4 years ago by teor

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

comment:6 Changed 4 years ago by teor

Status: reopenedneeds_review

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.

comment:7 Changed 4 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

lgtm; merged to master; thanks!

Note: See TracTickets for help on using tickets.