Re: [tor-bugs] #9729 [Tor]: Make bridges publish additional ORPort addresses in their descriptor

["Tor Bug Tracker & Wiki" <blackhole@xxxxxxxxxxxxxx>]

#9729: Make bridges publish additional ORPort addresses in their descriptor
----------------------------+----------------------------------------------
     Reporter:  sqrt2       |      Owner:
         Type:              |     Status:  needs_revision
  enhancement               |  Milestone:
     Priority:  normal      |    Version:  Tor: 0.2.5.1-alpha
    Component:  Tor         |   Keywords:  ORPort bridge multiple addresses
   Resolution:              |  Parent ID:
Actual Points:              |
       Points:              |
----------------------------+----------------------------------------------

Comment (by ln5):

 Lacking the brains for a high-level review, I'm going for a partial
 look-at-details review for now, in the hope that it might help shake
 out some issues.

 Also, note that most of router*.[ch] and test/test_addr.c have _not_
 been reviewed yet.

 - are we leaking memory in get_interface_address6() in "smart way"
   when return_addrs is empty after the loop (returning NULL)?

 - the construct with r = return_addrs in get_interface_address6() is a
   bit odd IMO; the test !r will never be true f.ex.

 - are we leaking memory (the list in r) in get_interface_address()?

 - slight code duplication in dirserv_orconn_tls_done() when calling
   node_set_last_reachability()

 - is the check for the ipv6 address in
   dirserv_single_reachability_test() backwards? i.e. should check for
   tor_addr_compare() == 0

 - did router_get_active_listener_port_by_addr_type_af() get the
   tor_addr_compare() test backwards too?

 - the call to tor_free() in router_get_advertised_or_port_by_af() (at
   router.c:1508) is never reached and we're leaking that memory

 - needs a ChangeLog entry (i.e. a file in changes/)

 - documentation for last_discovered_ipv*_address refers to a function
   that does not exist (change to get_stable_interface_address6?)

 Formatting and style.
 - remove trailing whitespace
 - no need to break a line like
   "port_cfg_t * router_get_main_ipv6_listener_address(const smartlist_t
 *ports);"
 - i'd add curly braces to constructs like
     else
       /* We got here from options_validate(), and we're just checking if
 we're
        * globally reachable and can be a directory authority. This
 function will
        * be called again when ports have been configured and we can pick
 the right
        * address then. */
       log_fn(notice_severity, LD_CONFIG,
              "We have at least one usable address, that's enough right
 now.");
 - indentation is wrong in a couple places (like address.c:1479)
 - long lines here and there (see 'Wide' when doing "make check-spaces")
 - no need for '\' to continue a line (like dirserv.c:2966)
 - i'd break lines like this in two:
         if (chan) command_setup_channel(chan);

-- 
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/9729#comment:17>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs