Opened 6 months ago

Closed 4 months ago

#29018 closed enhancement (fixed)

Make all statistics depend on ExtraInfoStatistics

Reported by: teor Owned by: teor
Priority: Medium Milestone: Tor: 0.4.1.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: asn-merge, security-low, 040-deferred-20190220, privcount, dgoulet-merge, network-team-roadmap-2019-Q1Q2, merge-after-0403-alpha
Cc: Actual Points: 2
Parent ID: Points: 0.1
Reviewer: nickm Sponsor: SponsorV-can

Description

Like #29017, when a user sets ExtraInfoStatistics 0, they probably don't want any statistics in their extra-info document.

Child Tickets

Change History (25)

comment:1 Changed 6 months ago by teor

Status: assignedneeds_review

See my pull request https://github.com/torproject/tor/pull/639

Notes:

GeoIP file hashes might not be sensitive, but the other statistics are reasonably sensitive, particularly for private bridges.

ServerTransportPlugin lines aren't strictly statistics, but they are still potentially sensitive.

We turn off statistics in the extra-info file if we ever fail to parse the file. So we should really include all optional chunks under the write_stats_to_extrainfo check.

comment:2 Changed 6 months ago by teor

Sponsor: SponsorV-can

Improving the reliability and usability of statistics can fall under Sponsor V.

comment:3 Changed 6 months ago by dgoulet

Reviewer: nickm

comment:4 Changed 6 months ago by nickm

Status: needs_reviewmerge_ready

This looks fine to me. A quick question before I merge: I see that coverage has fallen here. Do you think you would time to add a test for this stuff?

comment:5 in reply to:  4 ; Changed 6 months ago by teor

Status: merge_readyneeds_revision

Replying to nickm:

This looks fine to me. A quick question before I merge: I see that coverage has fallen here. Do you think you would time to add a test for this stuff?

I could tweak the existing tests to run with ExtraInfoStatistics 0 and 1. That should improve coverage compared to master.

comment:6 in reply to:  5 Changed 5 months ago by teor

Replying to teor:

Replying to nickm:

This looks fine to me. A quick question before I merge: I see that coverage has fallen here. Do you think you would time to add a test for this stuff?

I could tweak the existing tests to run with ExtraInfoStatistics 0 and 1. That should improve coverage compared to master.

Actually, there are no existing tests that build an extrainfo document. Instead, the existing tests mock the relevant functions, replacing them with static descriptor strings.

These static descriptor strings are also out of date. I opened #29521 to address this technical debt, because I don't have time to do it in Sponsor V.

comment:7 Changed 5 months ago by teor

Actual Points: 0.12
Status: needs_revisionneeds_review

Branch

I tried to merge the changes from 0.3.3 forward to master, but the merge was really complicated.

So I opened a new pull request https://github.com/torproject/tor/pull/709
It contains the:

Tests

The code passes make test-network-all with ExtraInfoStatistics 1 and ExtraInfoStatistics 0 set in chutney's common.i.

I discovered some bugs on my local machine while writing this code.
I don't know if they'll affect CI:

  • #29529 util/map_anon_nofork test fails on macOS
  • #29527 Division by zero: undefined behaviour in circuitpadding/circuitpadding_sample_distribution test
  • #29530 Some address/get_if_addrs* tests fail when the network is unreachable

TODO

I think there might be some memory leaks in the new unit tests, I'm not sure if I caught them all. I'll check in the morning once CI has finished.

I think the coverage should have increased a bit compared with #29017 and master. But I'll need to wait for CI to make sure.

comment:8 in reply to:  7 Changed 5 months ago by teor

Replying to teor:

I think there might be some memory leaks in the new unit tests, I'm not sure if I caught them all. I'll check in the morning once CI has finished.

I found and fixup'd one memory leak. Hopefully it's the last one. (I need to install a newer version of clang to get good leak detection locally.)

I think the coverage should have increased a bit compared with #29017 and master. But I'll need to wait for CI to make sure.

Coveralls stopped working about 20 hours ago. Maybe it's back now?

comment:9 Changed 5 months ago by nickm

Keywords: 040-deferred-20190220 added
Milestone: Tor: 0.4.0.x-finalTor: unspecified

Deferring 51 tickets from 0.4.0.x-final. Tagging them with 040-deferred-20190220 for visibility. These are the tickets that did not get 040-must, 040-can, or tor-ci.

comment:10 Changed 5 months ago by teor

Keywords: privcount added
Milestone: Tor: unspecifiedTor: 0.4.1.x-final

This task is on the 0.4.1 roadmap as part of Sponsor V.

comment:11 Changed 5 months ago by teor

The coveralls bot is back:
https://github.com/torproject/tor/pull/709#issuecomment-465408842

These unit tests increase the overall coverage by 0.2%.

I looked at the uncovered lines: many of them are error cases or mocking. Some of them are from other pull requests.

I decided not to unit test the new router_build_fresh_unsigned_routerinfo() function: it requires too much state. (See #29521.)

comment:12 Changed 5 months ago by nickm

Status: needs_reviewmerge_ready

This seems okay for 0.4.0. I'm not 100% sure on backporting any farther, but I wouldn't object much.

Last edited 5 months ago by nickm (previous) (diff)

comment:13 in reply to:  12 Changed 5 months ago by nickm

Status: merge_readyneeds_review

Replying to nickm:

This seems okay for 0.4.0. I'm not 100% sure on backporting any farther, but I wouldn't object much.

Oops, I meant to say that on another ticket.

comment:14 Changed 5 months ago by nickm

Keywords: dgoulet-merge added
Status: needs_reviewmerge_ready

LGTM. This can go in after #29017 has been merged.

comment:15 in reply to:  14 Changed 5 months ago by dgoulet

Replying to nickm:

LGTM. This can go in after #29017 has been merged.

Is this ACK for the PR 639 or 709. They are MASSIVELY different and the 709 one is huge. I just want to make sure what was reviewed.

comment:16 Changed 5 months ago by nickm

I'm acking 709, but only once 683 is merged as a part of #29017.

comment:17 Changed 4 months ago by gaba

Keywords: network-team-roadmap-2019-Q1Q2 added

comment:18 in reply to:  16 Changed 4 months ago by teor

Replying to nickm:

I'm acking 709, but only once 683 is merged as a part of #29017.

#29017 has been merged to master.
This ticket won't be backported, so we can merge now.

comment:19 Changed 4 months ago by teor

Keywords: asn-merge added

Oh right, asn has to merge this ticket.

comment:20 Changed 4 months ago by teor

Keywords: fast-fix removed

Fix keywords.

comment:21 Changed 4 months ago by teor

asn, please don't merge until next week, nickm is doing an alpha.

comment:22 Changed 4 months ago by teor

Keywords: merge-after-0403-alpha added

Tag these tickets for merge after 0.4.0.3-alpha, to make searches easier.

comment:23 Changed 4 months ago by teor

This ticket is a large refactor, it's not going to be backported to 0.4.0.

comment:24 Changed 4 months ago by teor

0.4.0.3-alpha was released on Friday, so these tickets can now be merged.

#29018 and #29806 can be merged by asn, and #29703 can be merged by teor as a backport.

comment:25 Changed 4 months ago by asn

Resolution: fixed
Status: merge_readyclosed

Pushed in master. comment:23 indicates that no 040 backport is needed.
Closing this.

Note: See TracTickets for help on using tickets.