Opened 7 weeks ago

Closed 6 weeks ago

#31343 closed defect (fixed)

appveyor: labs(time_t) is not allowed

Reported by: nickm Owned by: nickm
Priority: Very High Milestone: Tor: 0.2.9.x-final
Component: Core Tor/Tor Version: Tor: 0.2.4.4-alpha
Severity: Major Keywords: tor-ci-fail 029-backport 035-backport 040-backport 041-backport
Cc: Actual Points: .1
Parent ID: Points: .1
Reviewer: mikeperry, teor Sponsor:

Description

Mike points out that appveyor is failing with

../src/core/or/channeltls.c:1768:7: error: absolute value function 'labs' given an argument of type 'time_t' {aka 'long long int'} but has parameter of type 'long int' which may cause truncation of value [-Werror=absolute-value]
 1768 |   if (labs(now - chan->conn->handshake_state->sent_versions_at) < 180) {
      |       ^~~~

That code has been there for ages, though. Looks like this is a new compiler warning rather than a new bug. It is a c issue, though, anywhere that time_t is bigger than long.

Child Tickets

TicketStatusOwnerSummaryComponent
#31374closednickmAppveyor: cast between incompatible function types in compat_timeCore Tor/Tor

Change History (17)

comment:1 Changed 7 weeks ago by nickm

On examination, this labs() doesn't belong here at all. The code here is testing whether we are getting a NETINFO cell fairly soon after we sent a VERSIONS cell. Using labs() makes it also test whether we are getting a NETINFO cell no earlier than 3 minutes before sending the VERSION cell, which is silly.

comment:2 Changed 7 weeks ago by nickm

Keywords: 041-must tor-ci 029-backport 035-backport 040-backport 041-backport added

Fixes for review.

0.2.9: ticket31343_029 with PR in https://github.com/torproject/tor/pull/1209
0.3.5 (fixes conflicts): ticket31343_035 with PR in https://github.com/torproject/tor/pull/1210
0.4.0 (fixes conflicts): ticket31343_040 with PR in https://github.com/torproject/tor/pull/1211
0.4.2 (no conflicts, just for CI): ticket31343_042 with PR in https://github.com/torproject/tor/pull/1212

Tagging for full backports, since this breaks CI.

comment:3 Changed 7 weeks ago by nickm

Owner: set to nickm
Status: newaccepted

comment:4 Changed 7 weeks ago by nickm

Status: acceptedneeds_review

comment:5 Changed 7 weeks ago by nickm

Actual Points: .1

comment:6 Changed 6 weeks ago by nickm

Milestone: Tor: 0.4.2.x-finalTor: 0.4.1.x-final

comment:7 Changed 6 weeks ago by nickm

Reviewer: mikeperry

comment:8 Changed 6 weeks ago by teor

Priority: MediumVery High
Severity: NormalCritical
Status: needs_reviewneeds_revision

I did a quick review here, because the appveyor failures are blocking backport merges.

I think the new code warns on any skew, but the old code only warned on skews over 1 hour. That change is not documented in the changes file, and seems to be a mistake?

Also, because the old code only warned on skews over 1 hour, the changes file has a confusing description of the old behaviour.

See my comments on the PR for details.

Last edited 6 weeks ago by teor (previous) (diff)

comment:9 Changed 6 weeks ago by teor

Severity: CriticalMajor

comment:10 Changed 6 weeks ago by teor

Reviewer: mikeperrymikeperry, teor

comment:11 Changed 6 weeks ago by nickm

Status: needs_revisionneeds_review

Okay, I've fixed the bug here. Apparently I missed it because it only existed in the 0.2.9 branch, and I had fixed it in the others when merging. :/

comment:12 Changed 6 weeks ago by asn

Status: needs_reviewneeds_revision

Seems like there is at least one more such issue that is not resolved by the above branches and appveyor still fails:

../src/feature/nodelist/routerlist.c:2923:21: error: absolute value function 'labs' given an argument of type 'time_t' {aka 'long long int'} but has parameter of type 'long int' which may cause truncation of value 
[-Werror=absolute-value]
 2923 |   time_difference = labs(r2->uptime - (r1->uptime + (r2pub - r1pub)));
      |                     ^~~~
depbase=`echo src/feature/nodelist/fmt_routerstatus.o | sed 's|[^/]*$|.deps/&|;s|\.o$||'`;\

We should probably fix that one too, right?

comment:13 Changed 6 weeks ago by teor

Keywords: tor-ci-fail added; tor-ci removed

comment:14 Changed 6 weeks ago by nickm

Keywords: tor-ci added; 041-must tor-ci-fail removed
Milestone: Tor: 0.4.1.x-finalTor: 0.4.0.x-final

Fixed that and ran tests. There is still an appveyor issue wrt function casting, but that isn't this.

These branches are now merged to maint-0.4.1 and forward; marking for backport.

comment:15 Changed 6 weeks ago by nickm

Keywords: tor-ci-fail added; tor-ci removed

comment:16 Changed 6 weeks ago by teor

Keywords: backports? removed

comment:17 Changed 6 weeks ago by teor

Milestone: Tor: 0.4.0.x-finalTor: 0.2.9.x-final
Points: .1
Resolution: fixed
Status: needs_revisionclosed
Version: Tor: 0.2.4.4-alpha

This issue breaks Appveyor CI: backporting immediately.

It looks like #31343 and #31374 each fail due to the other's error: I'll do a combined test branch to check.

Merged #31343 and #31374 together.

Note: See TracTickets for help on using tickets.