Opened 3 years ago

Last modified 17 months ago

#22739 needs_revision defect

Make routerinfo_t and routerstatus_t addresses immutable; store overrides in node_t

Reported by: nickm Owned by:
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: review-group-31, 034-triage-20180328, 034-removed-20180328
Cc: catalyst, teor Actual Points:
Parent ID: #27248 Points:
Reviewer: Sponsor:


See this comment in rewrite_node_address_for_bridge:

  /* XXXX overridden addresses should really live in the node_t, so that the
   *   routerinfo_t and the microdesc_t can be immutable.  But we can only
   *   do that safely if we know that no function that connects to an OR
   *   does so through an address from any source other than node_get_addr().

Here's how we can do that, in several phases.

1) Add an "override orport" tor_addrport_t in node_t which, if set, overrides the advertised ports. Make rewrite_node_address_for_bridge() overrwrite that in addition to the stuff it already overwrites.

2) Make the various node_get*_addr() look at that field.

3) Rename ri->addr, ri->*port, md->addr, and md->*port, possibly combining them to use the tor_addrport_t structure. This will break everything that uses them. As we fix those compilation errors, make sure that everything using them to decide where to connect uses node_get*_addr() instead -- specifically, one of the functions modified in 2 above.

4) Remove the extra logic in rewrite_node_address_for_bridge().

Child Tickets

#20531newrewrite_node_address_for_bridge and networkstatus_set_current_consensus can conflictCore Tor/Tor

Change History (11)

comment:1 Changed 3 years ago by catalyst

Cc: catalyst added

comment:2 Changed 3 years ago by nickm

Milestone: Tor: 0.3.2.x-finalTor: 0.3.3.x-final
Owner: set to nickm
Status: newaccepted

comment:3 Changed 3 years ago by nickm

Cc: teor added
Milestone: Tor: 0.3.3.x-finalTor: 0.3.4.x-final
Status: acceptedneeds_review

I've started a branch here as feature22739. I've taken the approach above, but only used the "rename the field" trick to identify places that might need a change. I've marked those with XXXX comments.

This isn't done, but I wouldn't mind some feedback on this approach.

Adding teor to cc because he's looked at this code a lot.

comment:4 Changed 3 years ago by nickm

Keywords: review-group-31 added

comment:5 Changed 3 years ago by ahf

Status: needs_reviewneeds_revision

I think the overall approach, in these two patches, is sensible and should clean-up this code a lot. The removed code in the first patch is quite some spaghetti.

Small nit:

tor_addr_port_t override_orport;

in or.h is missing a documentation string.

I think this code is good, but I'm marking this ticket as needing revision because of the many XXX's in the second patch.

Maybe Teor have some additional feedback here too?

comment:6 Changed 3 years ago by nickm

Keywords: 034-triage-20180328 added

comment:7 Changed 3 years ago by nickm

Keywords: 034-removed-20180328 added

Per our triage process, these tickets are pending removal from 0.3.4.

comment:8 Changed 3 years ago by nickm

Milestone: Tor: 0.3.4.x-finalTor: unspecified

These needs_revision, tickets, tagged with 034-removed-*, are no longer in-scope for 0.3.4. We can reconsider any of them, if somebody does the necessary revision.

comment:9 Changed 23 months ago by teor

Parent ID: #27248

#27248 would obsolete or solve this issue.

comment:10 Changed 17 months ago by nickm

Owner: nickm deleted
Status: needs_revisionassigned

comment:11 Changed 17 months ago by nickm

Status: assignedneeds_revision

None of these revisions are in my near-term plans.

Note: See TracTickets for help on using tickets.