appveyor: labs(time_t) is not allowed
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.
Activity
changed milestone to %Tor: 0.2.9.x-final
Trac:
Child Ticket(s): #31374 (moved)added 029-backport 035-backport 040-backport 041-backport actualpoints::.1 component::core tor/tor milestone::Tor: 0.2.9.x-final owner::nickm points::.1 priority::very high resolution::fixed reviewer::mikeperry reviewer::teor severity::major status::closed tor-ci-fail type::defect version::tor 0.2.4.4-alpha labels
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.
-
Fixes for review.
0.2.9:
ticket31343_029with PR in https://github.com/torproject/tor/pull/1209 0.3.5 (fixes conflicts):ticket31343_035with PR in https://github.com/torproject/tor/pull/1210 0.4.0 (fixes conflicts):ticket31343_040with PR in https://github.com/torproject/tor/pull/1211 0.4.2 (no conflicts, just for CI):ticket31343_042with PR in https://github.com/torproject/tor/pull/1212Tagging for full backports, since this breaks CI.
Trac:
Keywords: backports? deleted, backports? 041-must tor-ci 029-backport 035-backport 040-backport 041-backport added 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.
Trac:
Priority: Medium to Very High
Severity: Normal to Critical
Status: needs_review to needs_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?
Trac:
Status: needs_review to needs_revision-
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.
Trac:
Milestone: Tor: 0.4.1.x-final to Tor: 0.4.0.x-final
Keywords: backports? 041-must tor-ci-fail 029-backport 035-backport 040-backport 041-backport deleted, backports? tor-ci 029-backport 035-backport 040-backport 041-backport added This issue breaks Appveyor CI: backporting immediately.
It looks like #31343 (moved) and #31374 (moved) each fail due to the other's error: I'll do a combined test branch to check.
Merged #31343 (moved) and #31374 (moved) together.
Trac:
Resolution: N/A to fixed
Version: N/A to Tor: 0.2.4.4-alpha
Points: N/A to .1
Milestone: Tor: 0.4.0.x-final to Tor: 0.2.9.x-final
Status: needs_revision to closedmentioned in issue #31374 (moved)
moved to tpo/core/tor#31343 (closed)