Opened 3 years ago

Closed 3 years ago

#17751 closed defect (fixed)

Use router_get_my_routerinfo() rather than desc_routerinfo

Reported by: teor Owned by: hkannan
Priority: Medium Milestone: Tor: 0.2.9.x-final
Component: Core Tor/Tor Version:
Severity: Minor Keywords: easy
Cc: Actual Points:
Parent ID: Points: small
Reviewer: Sponsor:

Description

The following functions use desc_routerinfo where they could use router_get_my_routerinfo():

  • check_descriptor_ipaddress_changed
  • router_compare_to_my_exit_policy
  • router_my_exit_policy_is_reject_star
  • router_get_my_descriptor
  • check_descriptor_bandwidth_changed

This could cause issues if desc_routerinfo needs to be rebuilt, because some of these functions don't call router_get_my_routerinfo() first.

Others have comments about desc_routerinfo that might need to be updated:

  • dirserv_get_routerdescs
  • router_compare_to_my_exit_policy
    • Also change "IPv6 and IPv6 entries" to "IPv4 and IPv6 entries"

Child Tickets

Change History (10)

comment:1 Changed 3 years ago by nickm

Milestone: Tor: 0.2.8.x-finalTor: 0.2.9.x-final

It is impossible that we will fix all 226 currently open 028 tickets before 028 releases. Time to move some out. This is my second pass through the "new" and tickets, looking for things to move to 0.2.9.

comment:2 Changed 3 years ago by nickm

Points: small

easy to do, but it will be important to check the logic carefully.

comment:3 Changed 3 years ago by hkannan

Owner: set to hkannan
Status: newaccepted

comment:4 Changed 3 years ago by hkannan

Status: acceptedneeds_review

comment:5 Changed 3 years ago by hkannan

Change on https://github.com/harini-kannan/tor, on branch Ticket17751.

comment:6 Changed 3 years ago by nickm

Looks good at first glance. Before we merge, I'm going to have to take a look at all the code surrounding this to make sure there are no subtle interactions. But that's on me. I hope I get to it soon.

comment:7 Changed 3 years ago by teor

The only behavioural change in this patch is that the following checks:

  if (!server_mode(get_options()))
    return NULL;
  if (router_rebuild_descriptor(0))
    return NULL;

are now performed before accessing router_get_my_routerinfo() in:

  • check_descriptor_bandwidth_changed()
  • check_descriptor_ipaddress_changed()

I think this is slightly better behaviour than the old code.
(All the other changes result in the same behaviour: we were already calling router_get_my_routerinfo() in most of these functions before accessing desc_routerinfo.)

comment:8 Changed 3 years ago by cypherpunks

I would store the result of calls to router_get_my_routerinfo() in a variable first (and check for NULL ofcourse) instead of using the result directly.

comment:9 Changed 3 years ago by nickm

Thanks for looking at it, teor! merged, and tweaked per cypherpunks's suggestion

comment:10 Changed 3 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed
Note: See TracTickets for help on using tickets.