Opened 22 months ago

Closed 5 months ago

#24490 closed defect (fixed)

Stop setting bridges running in networkstatus_getinfo_by_purpose()

Reported by: teor Owned by: neel
Priority: Medium Milestone: Tor: 0.4.1.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: easy, intro, refactor, code-correctness
Cc: ffmancera, isis, neel Actual Points:
Parent ID: Points: 1
Reviewer: ahf Sponsor:

Description

A GETINFO controller function shouldn't modify vital data structures.

Instead, we should make sure the list is up to date regularly, or that it is updated before we call this function.

This bug was introduced in commit 74d05f4 in tor-0.2.0.13-alpha.

Child Tickets

Change History (16)

comment:1 Changed 22 months ago by teor

Cc: ffmancera added

Replying to ffmancera from #tor-dev:

No, I am not working on this.
It's a bug komlo and I discovered at the recent Tor meeting.

comment:2 Changed 22 months ago by teor

Cc: isis added

What we need to do to fix this bug is to move the call that sets bridges running somewhere else, so that this function doesn't have any side effects.

This might mean that we need to make the call happen as a scheduled task, because none of the controller GETINFO code should change state.

Maybe isis will have an opinion on how best to fix this?

comment:3 Changed 6 months ago by neel

Cc: neel added
Owner: set to neel
Status: newassigned

comment:4 Changed 6 months ago by neel

Status: assignedneeds_review

My PR is here: https://github.com/torproject/tor/pull/889

Setting as needs_review.

comment:5 Changed 6 months ago by asn

Reviewer: ahf

comment:6 Changed 5 months ago by ahf

I think the code looks good, but I'm not marking it as ready for merge yet. I've just asked the other network team members about how we should review the exceptions file for practracker. I've also asked the current bridge auth administrator how they handle this currently.

comment:7 Changed 5 months ago by neel

I noticed that teor gave reviews to my PR. I have pushed the revisions. Still needs to pass CI, however.

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

Hi neel, thanks, but I wasn't finished with my review.

I have numbered the changes that we need to make.

Replying to ahf:

I think the code looks good, but I'm not marking it as ready for merge yet. I've just asked the other network team members about how we should review the exceptions file for practracker.

  1. I think we can put most of the new flag-setting code in voteflags.c:

https://github.com/torproject/tor/pull/889/files#r274725520

In fact, we might not even need a new callback:

I've also asked the current bridge auth administrator how they handle this currently.

I bet it just works for them, and they don't know how it works :-)

v3 (non-bridge) directory authorities update the Running flag when they vote.

But for bridges, the Running flag gets updated in the getinfo function.

  1. For consistency, let's update the bridge Running flag in networkstatus_dump_bridge_status_to_file(), before calling networkstatus_getinfo_by_purpose().

Authorities also update the running flag in list_server_status_v1(), another control port function.

  1. For consistency, let's stop updating the Running flag in list_server_status_v1(), and just let it be updated when the vote/bridge-status is written.
  1. For each function we modify, let's update the function documentation.
  1. Let's also update the dir-spec, to document the old and new behaviour. The first version with the new behaviour will be 0.4.1.1-alpha, let's put that in the spec.

comment:9 Changed 5 months ago by neel

I have made the changes.

The torspec PR is here: https://github.com/torproject/torspec/pull/76

I believe the changes had to be made in control-spec, not dir-spec.

comment:10 Changed 5 months ago by teor

Milestone: Tor: unspecifiedTor: 0.4.1.x-final

comment:11 Changed 5 months ago by teor

comment:12 Changed 5 months ago by ahf

Status: needs_reviewmerge_ready

I think the spec change looks good and as far as I can tell you addressed the concerns teor had with the code.

comment:13 Changed 5 months ago by nickm

The code changes and the spec changes look like they do what they're supposed to do. Quick question, though: is this actually an improvement? We're making a GETINFO result less accurate. Maybe doing this on a timer would be more sensible? If so I'll merge this, then do another ticket to add a timer.

comment:14 in reply to:  13 ; Changed 5 months ago by teor

Replying to nickm:

The code changes and the spec changes look like they do what they're supposed to do. Quick question, though: is this actually an improvement? We're making a GETINFO result less accurate. Maybe doing this on a timer would be more sensible? If so I'll merge this, then do another ticket to add a timer.

Sure, I think that doing it on a timer is sensible.

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

Replying to teor:

Sure, I think that doing it on a timer is sensible.

I've opened #30301 for this.

comment:16 Changed 5 months ago by nickm

Resolution: fixed
Status: merge_readyclosed

Branches merged to tor master and to torspec.

Note: See TracTickets for help on using tickets.